wvlet / airframe

Essential Building Blocks for Scala
https://wvlet.org/airframe
Apache License 2.0
632 stars 66 forks source link

surface: Simplify extractSymbol - avoid .tree to make library working with Scala 3.4.2 #3533

Closed OndrejSpanel closed 5 days ago

OndrejSpanel commented 4 months ago

Followup to #3521

The method extractSymbol used to dealias inner types for both opaque types and type aliases does not work when the library is used from Scala 3.4.2 application. The reason is infamous .tree method again, which is known and documented to be unreliable.

I am not sure why it was used, the code seems simpler without it, but maybe I am missing something. After the simplification all tests still pass.

OndrejSpanel commented 4 months ago

After the simplification all tests still pass.

The airframe tests passed locally, but some tests for other modules fail on CI. I will check what is going on.

OndrejSpanel commented 4 months ago

The issue is with:

diJVM/testOnly *.PathDependentTypeTest

Compiler crashes with:

exception while retyping JdbcBackend.this of class This # -1

[error] java.lang.AssertionError: assertion failed: missing outer accessor in class PathDependentTypeTest
[error] scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:8)
[error] dotty.tools.dotc.transform.ExplicitOuter$.dotty$tools$dotc$transform$ExplicitOuter$$$outerParamAccessor(ExplicitOuter.scala:237)

My fix causes bad representation of type Database = DatabaseDef, which is now represented as Database:=Database instead of Database:=DatabaseDef.

OndrejSpanel commented 4 months ago

I am afraid I am unable to fix this. I was missing one dealias, once I have added it, types both with and without the fix seem correct, without the fix the type is represented as _ >: MyBackend.this.DatabaseDef <: MyBackend.this.DatabaseDef, which somehow prevents something which happens latter with MyBackend.this.DatabaseDef, the type used with the fix.

It works fine in my "light" fork without the method accessors.

Unless this is fixed, expect trouble once users of the library port to 3.4.2.

No fix logs:

dealiased wvlet.airframe.surface.PathDependentType.MyProfile#Backend.Database of wvlet.airframe.surface.PathDependentType.MyProfile#Backend.Database
  maybeOwner.declarations <init>,Database,DatabaseDef,DatabaseDef,DatabaseDef$
Match 1 _ >: MyBackend.this.DatabaseDef <: MyBackend.this.DatabaseDef in Some(type Database)

With the fix:

dealiased wvlet.airframe.surface.PathDependentType.MyProfile#Backend.Database of wvlet.airframe.surface.PathDependentType.MyProfile#Backend.Database
  maybeOwner.declarations <init>,Database,DatabaseDef,DatabaseDef,DatabaseDef$
Match 1 type Database in Some(type Database) as MyBackend.this.Database -> MyBackend.this.DatabaseDef

Note MyBackend.this.DatabaseDef, which reminds of #3411. The error itself reminds of #3440

xerial commented 2 months ago

As a preliminary work, I've added a CI for Scala 3.4.2 to reproduce this issue #3590. As path-dependent type support is a bit complicated, I will revisit the current implementation later based on this PR. Thanks for reporting and trying to fix the issue.

xerial commented 5 days ago

3669 adopted this approach. Instead of handling the complicated ownership issue, I've fixed the test case side so that we don't need to address the outer reference for path-dependent types, which will not be used so frequently.