Skip to content
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

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

smarter
Copy link
Member

@smarter smarter commented Aug 23, 2021

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 #13340.

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.
@smarter
Copy link
Member Author

smarter commented Aug 23, 2021

test performance please

@dottybot
Copy link
Member

performance test scheduled: 7 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/13361/ to see the changes.

Benchmarks is based on merging with master (dd7a07a)

@smarter
Copy link
Member Author

smarter commented Aug 27, 2021

Ping for review, it would be nice to get this in 3.1.0-RC1 since it fixes a crash.

@smarter smarter added this to the 3.1.0 milestone Aug 27, 2021
@odersky
Copy link
Contributor

odersky commented Aug 27, 2021

test performance please

Copy link
Contributor

@odersky odersky left a 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)
Copy link
Contributor

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.

Copy link
Member Author

@smarter smarter Aug 27, 2021

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.

Copy link
Contributor

@odersky odersky left a 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

@odersky
Copy link
Contributor

odersky commented Aug 27, 2021

test performance please

@smarter
Copy link
Member Author

smarter commented Aug 27, 2021

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.

@smarter smarter merged commit 427d313 into scala:master Aug 27, 2021
@smarter smarter deleted the proto-implicit branch August 27, 2021 19:46
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.

Crash: TyperState attempted to take ownership of Any which is already owned by committable TyperState
3 participants