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

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

Closed

Conversation

OndrejSpanel
Copy link
Contributor

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 OndrejSpanel marked this pull request as draft May 14, 2024 07:47
@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented May 14, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 xerial added the bug label May 23, 2024
@xerial
Copy link
Member

xerial commented Jul 22, 2024

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 added a commit that referenced this pull request Sep 29, 2024
Resolves issues when upgrading to Scala 3.3.4
- Surface for nested opaque types
- Simplified a path-dependent type test, as path-dependent type is not
frequently used, it should not be a blocker to use later versions of
Scala. #3533

---------

Co-authored-by: xerial-bot <leo+bot@xerial.org>
@xerial
Copy link
Member

xerial commented Sep 29, 2024

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

@xerial xerial closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants