-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
wildApprox: also approximate FunProto arguments #13361
Conversation
Previously, `wildApprox` delegated FunProto handling to `TypeMap#mapOver` which only maps the result type of the FunProto, but the result of `wildApprox` can be passed to `ImplicitRunInfo#computeIScope` which will end up calling `FunProto#typedArgs`. This caused two problems: - It forced us to use a `provisional` flag in `computeIScope` to account for uninstantiated type variables that might show up in the result of typedArgs. - It lead to an assertion failure (in i13340.scala) because it turns out that `typedArgs` doesn't always respect the pre-conditions of `mergeConstraintWith` (see added NOTE). By approximating FunProto arguments in wildApprox we can drop the `provisional` flag and avoid the issue with `typedArgs` (though we might need to actually fix `typedArgs` one day if the same issue shows up in other circumstances). Fixes scala#13340.
test performance please |
performance test scheduled: 7 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13361/ to see the changes. Benchmarks is based on merging with master (dd7a07a) |
Ping for review, it would be nice to get this in 3.1.0-RC1 since it fixes a crash. |
test performance please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restarted the performance tests to get a better picture.
Except for my comment on line 838 LGTM
if (args eq tp.args) && (resTp eq tp.resultType) then | ||
tp | ||
else | ||
FunProtoTyped(args, resTp)(ctx.typer, tp.applyKind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know FunProto
was not an UnapplyFunProto
? In that case it seems wrong to map to FunProtoTyped
. So we either should match on FunProtoTyped
, or implement a method that returns
the same kind of FunProto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wildApprox in master will call derivedFunProto
which is not overridden anywhere and therefore always returns a regular FunProto
even if we started with a FunProtoTyped
or an UnapplyFunProto
. As far as I can tell that doesn't matter for the stuf we do with wildApprox (which is collecting companion types from parts of the type). So while I agree we could try improving on this (and maybe making UnapplyFunProto a subtype of FunProtoTyped?), it seems like it could be done in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can merge this now then
test performance please |
Don't know what's going on with the bot but http://dotty-bench.epfl.ch/ seems to be working at least so we'll see if anything shows up there. |
Previously,
wildApprox
delegated FunProto handling toTypeMap#mapOver
which only maps the result type of the FunProto, butthe result of
wildApprox
can be passed toImplicitRunInfo#computeIScope
which will end up callingFunProto#typedArgs
. This caused two problems:provisional
flag incomputeIScope
to accountfor uninstantiated type variables that might show up in the result of
typedArgs.
that
typedArgs
doesn't always respect the pre-conditions ofmergeConstraintWith
(see added NOTE).By approximating FunProto arguments in wildApprox we can drop the
provisional
flag and avoid the issue withtypedArgs
(though we mightneed to actually fix
typedArgs
one day if the same issue shows up inother circumstances).
Fixes #13340.