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

Do not flag match types as Deferred and amend #20077 #20147

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

EugeneFlesselle
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Apr 10, 2024

Match types are already not flagged as Deferred when unpickled.
This caused varying results for ImplicitRunInfo#isAnchor, by not reaching the isMatchAlias condition when created from the Namer.

Ensures both #20071 and #20136 each have the same result, when compiled with a classpath dependency as when merged into a single file.

Fixes #20136

EugeneFlesselle and others added 3 commits April 10, 2024 00:26
Both observe different behaviours match type reductions
depending on whether they are compiled together or separately.
They both compile only with separate compilation.
This already wasn't the case for unpickled match types,
which caused varying results for `ImplicitRunInfo#isAnchor`,
by not reaching the `isMatchAlias` condition.

Ensures both scala#20071 and scala#20136 each have the same result,
when compiled with a classpath dependency as when merged.
Note that they both still fail (20071 compiles but shouldn't),
but at least do so consistently.

Also update TreeUnpickler MATCHtpt doc to align with changes from scala#19871

Co-authored-by: Guillaume Martres <[email protected]>
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.

LGTM. Nice catch!

@odersky odersky enabled auto-merge April 10, 2024 09:45
@odersky odersky merged commit 926e6a3 into scala:main Apr 10, 2024
16 checks passed
@odersky odersky deleted the match-types-deferred branch April 10, 2024 11:01
@@ -1542,8 +1542,6 @@ class TreeUnpickler(reader: TastyReader,
// as the reduction of the match type definition!
//
// We also override the type, as that's what Typer does.
// The difference here is that a match type that reduces to a non-match type
// makes the TypeRef for that definition will have a TypeAlias info instead of a MatchAlias.
Copy link
Member

Choose a reason for hiding this comment

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

Also update TreeUnpickler MATCHtpt doc to align with changes from #19871

I don't think this change is correct. #19871 is about making aliases to an applied type, even if that is backed by a match type, be a TypeAlias rather than a MatchAlias.

What we're dealing with here is a straight alias to a match type, except the match type immediately normalises to a non-match type. When compiling from source we simplify it and thus give the alias a TypeAlias. With this change, when we're unpickling we also simplify and thus give the alias a TypeAlias.

If you can find a clearer way to express that, we should keep this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I agree this shouldn't be impacted by the changes in #19871.
But I'm now not sure about which cases get reduced more from the Unpickler than when from the Typer ?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing more. The point is the have the same definition, whether from source code or from tasty, have the same info, so that differences in MatchAlias and TypeAlias don't become a factor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So assuming the match type reduces to non match type, won't they all have a TypeAlias now ?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, but only because I added overwriteType here. The deleted comment is part of (attempting to!) explain that and shouldn't have been removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I agree we need tpt.overwriteType(tpt.tpe.normalized) and that it is because that's what Typer does.

Still, it was my understanding that the Typer and Unpickler now use AliasingBounds in a consistent way, so I don't understand how there is a difference here for the TypeRef for that definition.

Copy link
Member

Choose a reason for hiding this comment

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

By "The difference here" I meant "The reason that is important to do is because..."

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.

Recent regression in interaction between match types and implicit conversion under separate compilation
4 participants