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

Test for Enumeration support (#3411) #3429

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OndrejSpanel
Copy link
Contributor

Test for #3411

With Scala 3 the test currently fails with:

[error] -- [E007] Type Mismatch Error: C:\Dev\airframe\airframe-surface\src\test\scala\wvlet\airframe\surface\i3411.scala:30:29
[error] 30 |    val m = Surface.methodsOf[SomeEnum]
[error]    |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |    Found:    Enumeration.this.Value
[error]    |    Required: wvlet.airframe.surface.i3411.SomeEnum.Value

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Feb 28, 2024

The same error can be seen with a standalone repro:

  trait Base {
    class InnerType {
      def compare(that: InnerType): Int = 0
    }
  }

  object OuterType extends Base {
    def create: InnerType = new InnerType
  }

    val m = Surface.methodsOf[OuterType.InnerType]
val m = Surface.methodsOf[OuterType.InnerType]
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Found:    Base.this.InnerType
Required: wvlet.airframe.surface.i3411.OuterType.InnerType

In this case the type of the compare parameter is represented as:

TypeRef(ThisType(TypeRef(ThisType(TypeRef(ThisType(TypeRef(NoPrefix,module class surface)),module class i3411$)),trait Base)),class InnerType)

When using .show on this, I get Base.this.InnerType. I would like to omit the Base from the path and replace it with OuterType. I will inspect the corresponding TypeRef a hopefully I can find a pattern.

@OndrejSpanel
Copy link
Contributor Author

With a significant effort I was able to manipulate the TypeRef so that I can get OuterType.InnerType instead of Base.this.InnerType. However when I do this, I get NoDenotation.owner later on.

I am afraid this might be too difficult for me - there are too many unfamiliar concepts and too little documentation.

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Feb 29, 2024

However when I do this, I get NoDenotation.owner later on.

Possible Scala 3 bug

This was due to a mistake on my part. I was able to create the correct type, yet in the result the type is interpreted as Base.this.InnerType. I start to suspect a bug on Scala 3 side.

I think the type should be OuterType.InnerType from the beginning, there is no reason to get Base.this.InnerType at all. I will try to check the process step by step and report if I find anything.

Workaround?

As for supporting Enumeration, perhaps the more practical solution would be to implement some workaround. At least in my case I am not interested in methods of Enumeration.Value at all, but it is important that it does not fail when an Enumeration is present somewhere in AST. This would not solve the OuterType / InnerType underlying cause, but that one is can be avoided by adjusting the AST in question - in case of Enumeration there is no choice as the path dependent Value is always there.

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Feb 29, 2024

I have stored my previous progress in enumeration-test-3411-type-tranformation branch and implemented the workaround here.

I have checked the Enumeration.Value methods. They are inherited from Value or even from Ordered (compare and all comparison operators). Many of them fail because they reference the enumeration value via a base class. The only methods which do not fail are id, equals and hashCode. It would be possible to keep them by implementing a more intricate test, but I doubt it is worth the effort,

I suggest to see what will Scala 3 reaction to my bug report be. If the issue is not fixed and no alternative solution advised, I suggest proceeding with the simple workaround.


Note: This is not a blocking issue for me now. I do not use Surface.methodsOf on ASTs using Enumeration, only Surface.of, which works fine. I have implemented the workaround so that it is ready if you feel you like it, but I do not really need it at the moment.

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Feb 29, 2024

Note: it seems the tests are currently failing with Scala 2 / Scala.js. See https://github.com/wvlet/airframe/actions/runs/8099295592?pr=3431:

[error] /home/runner/work/airframe/airframe/airframe-surface/src/test/scala/wvlet/airframe/surface/i3411.scala:30:30: type mismatch;
[error] found : Enumeration.this.Value
[error] required: wvlet.airframe.surface.i3411.SomeEnum.Value
[error] val m = Surface.methodsOf[SomeEnum]

The error is very similar to the one provided by Scala 3. The Scala 2 / JVM passes fine. I do not understand what this means.

The Scala 3 error is:

[error] -- [E007] Type Mismatch Error: /home/runner/work/airframe/airframe/airframe-surface/src/test/scala/wvlet/airframe/surface/i3411.scala:30:29
[error] 30 | val m = Surface.methodsOf[SomeEnum]
[error] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error] | Found: Enumeration.this.Value
[error] | Required: wvlet.airframe.surface.i3411.SomeEnum.Value

@xerial
Copy link
Member

xerial commented Feb 29, 2024

@OndrejSpanel Thank you for leaving the work log. I'm also not worried about supporting the legacy enumeration as Scala 3 enum is already supported. If we can just get Surface.methodsOf to compile, ignoring these enumeration methods that use dependent types is ok.

I can take over this PR. thanks

@OndrejSpanel
Copy link
Contributor Author

OndrejSpanel commented Mar 12, 2024

A fact which might be interesting: in the fork mentioned in #3440 (comment) I get the same compilation error with Scala 2.12 / Scala.js version:

[info] compiling 26 Scala sources to C:\Dev\airframe\airframe-surface\.js\target\scala-2.12\test-classes ...
[error] C:\Dev\airframe\airframe-surface\src\test\scala\org\opengrabeso\airframe\surface\i3439.scala:33:30: type mismatch;
[error]  found   : Base.this.InnerType
[error]  required: i3439.this.OuterType.InnerType
[error]     val m = Surface.methodsOf[OuterType.InnerType]

I think it is again the methodCaller issue - I did not remove this from Scala 2 version yet, thinking it is not necessary to remove it in Scala 2, as it worked fine in Scala 2.13

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