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

Erase constant value type #9431

Closed
wants to merge 1 commit into from
Closed

Conversation

som-snytt
Copy link
Contributor

Mixin was confused when String("hello, world") overrides String.

Fixes scala/bug#12301

@scala-jenkins scala-jenkins added this to the 2.13.5 milestone Jan 12, 2021
@som-snytt
Copy link
Contributor Author

som-snytt commented Jan 12, 2021

Quick lunchtime fix. This probably breaks overriding a method of literal type. (Edit: that works after all. I was wondering about the asymmetry of the fix.)

@@ -4855,6 +4855,8 @@ trait Types
false
case PolyType(_, _) =>
false
case ConstantType(_) =>
matchesType(tp1, tp2.underlying.widen, alwaysMatchSimple)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced this is the right fix. IIUC, matches is used for override checking.


I really don't understand why alwaysMatchSimple is !phase.erasedTypes, an explanation of this would be very welcome. The result is (without your patch):

scala> typeOf[Int] matches typeOf[String]
val res3: Boolean = true

scala> exitingErasure(typeOf[Int] matches typeOf[String])
val res4: Boolean = false

The bug is that Mixin creates a mixin forwarder in X for P.p, it shouldn't do that. Your patch fixes that, so it does have an effect in Mixin, but I fear that it has more (potentially undesired) effects. With your patch:

scala> exitingErasure(typeOf[""] matches typeOf[String])
val res2: Boolean = false

scala> typeOf[Int] matches typeOf[String]
val res3: Boolean = true

In Mixin, there is a call to matchesType which explicitly sets alwaysMatchSimple = true. So maybe the bug is that through some different patch we get from Mixin to matchesType with alwaysMatchSimple = false?

Or maybe the bug is that the ConstantType is not erased?

Copy link
Member

Choose a reason for hiding this comment

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

I really don't understand why alwaysMatchSimple is !phase.erasedTypes, an explanation of this would be very welcome

Maybe the idea is "let's assume things override" before erasure. RefChecks will issue errors when overridding is not correct.

Or maybe the bug is that the ConstantType is not erased?

scala> trait P { def p: String = "p" }
     | class X extends P { override final val p      = "x" }
     | class Y extends P { override final val p: "x" = "x" }

scala> exitingErasure(typeOf[X].member(TermName("p")).tpe)
val res1: $r.intp.global.Type = String("x") <and> String

scala> exitingErasure(typeOf[Y].member(TermName("p")).tpe)
val res2: $r.intp.global.Type = (): String

scala> (new X).p
java.lang.ClassFormatError: Duplicate method name "p" with signature "()Ljava.lang.String;" in class file X
  at java.lang.ClassLoader.defineClass1(Native Method)
  at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
  at scala.reflect.internal.util.AbstractFileClassLoader.findClass(AbstractFileClassLoader.scala:77)
  at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
  at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
  ... 36 elided

scala> (new Y).p
val res4: String = x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll convert to draft and give it some thought.

I moved the scouting commits to a separate PR.

@som-snytt som-snytt marked this pull request as draft January 14, 2021 20:29
@som-snytt som-snytt closed this Jan 19, 2021
@SethTisue SethTisue removed this from the 2.13.5 milestone Jan 20, 2021
@som-snytt som-snytt reopened this Mar 31, 2022
@scala-jenkins scala-jenkins added this to the 2.13.9 milestone Mar 31, 2022
@som-snytt
Copy link
Contributor Author

@lrytz I'll rework this. Also Jean Rondeau will be in Zürich per the website

@SethTisue SethTisue modified the milestones: 2.13.9, 2.13.10 Apr 25, 2022
@lrytz lrytz modified the milestones: 2.13.10, 2.13.11 Sep 26, 2022
@SethTisue
Copy link
Member

@som-snytt is this one you're thinking of reviving...?

@som-snytt
Copy link
Contributor Author

@SethTisue yes, although like all old relationships, I've forgotten what was so attractive about it.

@SethTisue SethTisue modified the milestones: 2.13.11, 2.13.12 Feb 28, 2023
@som-snytt
Copy link
Contributor Author

For my stage name, I may take "Johnny Rondo". TIL "Ryan Starr" is just a stage name.

This is the "quick lunchtime fix" I must have intended two years ago. It is also the last of Lukas's semi-rhetorical questions.

In the test, explicitly annotated override def x: "x" = "x" was erased correctly. Unannotated x(): String("x") was untouched.

I did not answer Lukas's other question about matches during this visit; I left that red herring for another meal.

@som-snytt som-snytt marked this pull request as ready for review March 7, 2023 06:17
@som-snytt som-snytt modified the milestones: 2.13.12, 2.13.11 Mar 7, 2023
@som-snytt som-snytt changed the title Constant type matches widened type Erase constant value type Mar 7, 2023
@som-snytt
Copy link
Contributor Author

The answer is

[error]  * method BLACK()java.lang.String in interface scala.io.AnsiColor does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("scala.io.AnsiColor.BLACK")

and various osgi

[error] Test tools.test.osgi.reflection.toolbox.ReflectionToolBoxTest.basicMirrorThroughOsgi failed: java.lang.ClassFormatError: Method LogPendingSubTypesThreshold in class scala/reflect/internal/tpe/TypeComparers has illegal modifiers: 0x402, took 0.212 sec

@som-snytt som-snytt marked this pull request as draft March 7, 2023 08:53
@SethTisue SethTisue modified the milestones: 2.13.11, 2.13.12 Mar 13, 2023
@som-snytt som-snytt closed this Jun 5, 2023
@SethTisue SethTisue removed this from the 2.13.12 milestone Jun 10, 2023
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.

final val overriding def: Duplicate method name
4 participants