Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Make opaque type refinements of inline proxy objects abstract type constructors #20903

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

Open
EugeneFlesselle wants to merge 2 commits into scala:main
base: main
Choose a base branch
Loading
from dotty-staging:mt-opaque-inline-2

Conversation

Copy link
Contributor

@EugeneFlesselle EugeneFlesselle commented Jul 1, 2024

Change the rhs of the added refinements from TypeAliases to RealTypeBoundss.
This allows the opaque types to remain abstract type constructors, which is necessary for the MatchTypeCasePattern.AbstractTypeConstructor logic.

In particular, an AppliedType where the tycon was a reference to the refinement, used to dealias before proceeding with the comparison of the tycons, which is a requirement of the aforementioned AbstractTypeConstructor case of the MatchReducer.

Fixes #20427
Alternative to #20457

smarter and others added 2 commits July 1, 2024 13:26
The inliner tries to handle opaque types by replacing prefixes containing them
by proxy objects with type aliases. When the type we're mapping is a match type
application, this can end up breaking its reduction.
Change the rhs of the added refinements from `TypeAlias`es to `RealTypeBounds`s.
This allows the opaque types to remain abstract type constructors,
which is necessary for the `MatchTypeCasePattern.AbstractTypeConstructor` logic.
In particular, an AppliedType where the tycon was a reference to the refinement,
used to dealias before proceeding with the comparison of the tycons,
which is a requirement of the aforementioned `AbstractTypeConstructor`
case of the MatchReducer.
Fixes scala#20427
Alternative to scala#20457 
Copy link
Member

sjrd commented Jul 1, 2024

This bound (ah ah!) to be break something. There are very real things we can do with type aliases that we cannot do with abstract type members of equal bounds. For example, manipulating arrays:

scala> class Foo { type T = Int }
// defined class Foo
 
scala> val foo = new Foo
val foo: Foo = Foo@2ca54da9
 
scala> val a: Array[Int] = { val b = new Array[foo.T](1); b }
val a: Array[Int] = Array(0)
 
scala> class Foo { type T >: Int <: Int }
// defined class Foo
 
scala> val foo = new Foo
val foo: Foo = Foo@2bfc2f8b
 
scala> val a: Array[Int] = { val b = new Array[foo.T](1); b }
-- [E172] Type Error: ----------------------------------------------------------
1 |val a: Array[Int] = { val b = new Array[foo.T](1); b }
 | ^
 | No ClassTag available for foo.T
1 error found
soronpo reacted with laugh emoji

Copy link
Contributor Author

@sjrd Indeed, but that (specific issue) should not be a problem for inlining right ? since implicit resolution is already done by that point.
i.e. val b = Array[foo.T](1) is an error,
but val b = Array[foo.T](1)(using evidence1ドル$proxy1 : ClassTag[Int]) is ok.

I am by no means claiming there is nothing else that could be impacted though.

Copy link
Member

sjrd commented Jul 1, 2024

Perhaps. There are definitely other things that require an actual type alias. Extending or instantiating an aliased class, for example. And that will remain through inlining.

Copy link
Contributor Author

Again, just an empirical observation, but they both seem to work in the following simple test:

object A:
 opaque type W[T] = T
 inline def foo[T: ClassTag](x: T): Array[W[T]] =
 Array[W[T]](x)
 class Bar
 inline def bar =
 class Baz extends W[Bar]
 new W[Bar]
def Test =
 A.foo[Int](0) // ok
 A.bar // ok

Copy link
Contributor Author

It would be great to know if the changes break anything in the openCB, @WojciechMazur would it be possible to test that ?

Copy link
Contributor

I've started the OpenCB, I'll report the results when it's done

Copy link
Contributor Author

I've started the OpenCB, I'll report the results when it's done

Cool, thank you!

Copy link
Contributor

We've tested 1591 projects, 61 of them were already failing in last week nightly (3.5.1-RC1-bin-20240626-41f1489-NIGHTLY) and there are no new regressions found since that nightly version

EugeneFlesselle reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter Awaiting requested review from smarter

At least 1 approving review is required to merge this pull request.

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NamedTuple selection on the result of NamedTuple.Concat doesn't work

AltStyle によって変換されたページ (->オリジナル) /