Fix #2772: Special case Devalify for java.lang.System.* #2781
Conversation
Also update DropNoEffects to keep Select on static java field, to make sure a Java class doesn't end up in value position. That's another bug than the one reported in scala#2772, but it got triggered by the new test case.
| false | ||
| } else | ||
| readingOnlyVals(qual) | ||
|
|
||
| case t: Ident if !t.symbol.is(Mutable) && !t.symbol.is(Method) && !t.symbol.info.dealias.isInstanceOf[ExprType] => |
There was a problem hiding this comment.
Nothing says that there might not be an Ident corresponding to System.in, and in that case you would still inline it. Maybe add a isReallyMutable instead that you would use everywhere?
There was a problem hiding this comment.
The implementation of this case calls desugarIdent and recuse, I think the case where the Ident is System.in is covered.
@DarkDimius Could you explain why we even need this desugarIdent, why not use Select everywhere directly? Also where would be the proper was to move that function to factor out the version that's currently copy pasted in the backend?
| case Select(rec, _) if t.symbol.is(Method) => | ||
| if (t.symbol.isGetter && !t.symbol.is(Mutable)) readingOnlyVals(rec) // getter of a immutable field | ||
| if (t.symbol.isGetter && !t.symbol.is(Mutable)) | ||
| readingOnlyVals(rec) // getter of a immutable field |
|
This also won't work if someone uses reflection to mutate a final field. I suggest creating a documentation page for |
|
I went thought all usages of |
|
(I will later properly populate this documentation with content from the semester project report) |
| @@ -164,7 +164,17 @@ class Devalify extends Optimisation { | |||
| case _ => t | |||
| } | |||
|
|
|||
| def readingOnlyVals(t: Tree)(implicit ctx: Context): Boolean = dropCasts(t) match { | |||
| def readingOnlyVals(t: Tree)(implicit ctx: Context): Boolean = { | |||
| def isGetterOfAImmutableField = t.symbol.isGetter && !t.symbol.is(Mutable) | |||
| @@ -146,4 +146,15 @@ object Simplify { | |||
| case _ => None | |||
| } | |||
| } | |||
|
|
|||
| // System.in is static final fields that, for legacy reasons, must be | |||
There was a problem hiding this comment.
is -> is a
fields -> field
This description is a bit confusing: it starts by talking just about System.in and then mention the rest.
The name of the method is also not great, it's too easy to confuse with is(Mutable), I would call it something like isEffectivelyMutable, isReallyMutable or isChanging
| // https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5.4 | ||
| def isMutable(t: Tree)(implicit ctx: Context): Boolean = t match { | ||
| case _ if t.symbol.is(Mutable) => true | ||
| case s: Symbol => (s.symbol == defn.SystemModule) |
There was a problem hiding this comment.
I think you want s.owner.symbol here.
docs/docs/reference/optimisations.md
Outdated
|
|
||
| The Dotty compiler implements several local optimisations to generate faster bytecode. These optimisations can be enabled by compiling with the `-optimise` flag. The current best source of documentation about what tree transformations are performed under `-optimise` is the Scaladoc classes in the [localopt package](https://github.com/lampepfl/dotty/tree/master/compiler/src/dotty/tools/dotc/transform/localopt). | ||
|
|
||
| Local optimisations assumes no use of Java Reflection to mutate static final fields. |
There was a problem hiding this comment.
What about non-static final fields, are those safe to mutate?
| */ | ||
| def isEffectivelyMutable(t: Tree)(implicit ctx: Context): Boolean = t match { | ||
| case _ if t.symbol.is(Mutable) => true | ||
| case s: Select => (s.symbol == defn.SystemModule) |
There was a problem hiding this comment.
This still looks wrong to me, s may be System.in, so you need to look at the owner
@smarter thanks for the nice minimization!