Skip to content

Fix #11185: cache pretyped argument with adaptation in FunProto #12171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

liufengyun
Copy link
Contributor

Fix #11185: cache pretyped argument with adaptation in FunProto

@liufengyun liufengyun marked this pull request as ready for review April 21, 2021 13:03
class Test:
def foo(a: Int, b: Int) = a + b

Map(1 -> 2).map(foo _)
Copy link
Contributor

@Jasper-M Jasper-M Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a random thought, but if the collections ever get rewritten again, map might not be overloaded anymore. In which case this test no longer does what was intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll add another test with overloading.

@@ -2101,7 +2101,9 @@ trait Applications extends Compatibility {
else defn.FunctionOf(commonParamTypes, WildcardType)
overload.println(i"pretype arg $arg with expected type $commonFormal")
if (commonParamTypes.forall(isFullyDefined(_, ForceDegree.flipBottom)))
withMode(Mode.ImplicitsEnabled)(pt.typedArg(arg, commonFormal))
withMode(Mode.ImplicitsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance this looks too simplistic. We do a complicated dance not to overcommit by caching arguments that can be passed to several different expected types. How can we be sure that we can simply cache in this case? Maybe we can, but we'd need a detailed comment why.

Copy link
Contributor Author

@liufengyun liufengyun Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a comment is deserved here. Currently I see the rationale is this: the expected type for pretyping here come from all the overloading candidates and they (commandParamTypes) are all fully defined.

I just checked: it does not work with master anymore. I think #12138 is to blame.

I rebased against master, it seems to work.

@odersky
Copy link
Contributor

odersky commented Apr 23, 2021

I just checked: it does not work with master anymore. I think #12138 is to blame.

@odersky odersky merged commit bb806c5 into scala:master Apr 27, 2021
@odersky odersky deleted the fix-11185 branch April 27, 2021 07:40
@Kordyjan Kordyjan added this to the 3.0.1 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter untupling and automatic eta-expansion don't always mix
4 participants