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

Make simplify replace type parameters inside method types #15430

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jun 13, 2022

Simplify usually replaces a constrained TypeParamRef with the associated TypeVar, which is
necessary so that the TypeParamRef is properly instantiated afterwards. But it did not
recurse inside method types, since that might change signatures.

This commit adds an auxiliary TypeMap over method types that just instantiates
TypeParamRefs without applying any of the other transformations of simplify.

Fixes #15428

Simplify usually replaces a constrained TypeParamRef with the associated TypeVar, which is
necessary so that the TypeParamRef is properly instantiated afterwards. But it did not
recurse inside method types, since that might change signatures.

This commit adds an auxiliary TypeMap over method types that just instantiates
TypeParamRefs without applying any of the other transformations of simplify.
@odersky odersky requested a review from smarter June 13, 2022 09:42
@smarter
Copy link
Member

smarter commented Jun 14, 2022

I don't understand why this would be needed, the TypeVar should be permanently instantiated by the time we try to pickle it, if the instantiation was rolled back, that points to a bug somewhere (maybe a missing TyperState#commit() ?)

@smarter
Copy link
Member

smarter commented Jun 14, 2022

Ah I misunderstood, it's not the TypeVar that we replace, it's the TypeParamRef, and we get a TypeParamRef because the constraint is generated from the bounds of the method type. But then we have to be really careful about always calling simplify on every part of the type, for example I'm not sure if this is actually done in AppliedType which are normalized:
https://github.com/lampepfl/dotty/blob/741587d8e1c0238ca9c0986a77519cdf5a6f2fa8/compiler/src/dotty/tools/dotc/core/TypeOps.scala#L147-L148
Should we call simplify(normed, theMap)?

@odersky
Copy link
Contributor Author

odersky commented Jun 15, 2022

Not sure about it. So far, everything worked with just simplify. It's just that simplify was not applied inside MethodTypes. The PR fixes that concrete problem. I'd wait with the rest until we have test cases.

@odersky odersky merged commit 520beb2 into scala:main Jun 15, 2022
@odersky odersky deleted the fix-15428 branch June 15, 2022 17:40
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 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.

AssertionError: Orphan parameter reference
4 participants