Don't expand the name of accessors

[joroKr21]

I couldn't find a single use case where this is needed.
The callee is usually either not an accessor at all
or all declarations in scope are looped over and expanded.

On the other hand this fixes a bug in specialization.
After all the accessor(s) could be implementing an abstract method.

The usual concerns about binary compatibility and specialization apply.

Fixes scala/bug#12222

[lrytz]

for class C(val x: AnyRef) extends AnyVal

diff --git a/C.class.asm b/C.class.asm
index 653c931..dbbb86a 100644
--- a/C.class.asm
+++ b/C.class.asm
@@ -3,15 +3,15 @@
 public final class C {


-  // access flags 0x12
-  private final Ljava/lang/Object; x
+  // access flags 0x11
+  public final Ljava/lang/Object; C$$x

[joroKr21]

I couldn't find a single use case where this is needed.

Yes, it looks like it's needed in ExtensionMethods after all.

[joroKr21]

Yes, it looks like it's needed in ExtensionMethods after all.

No, I just went too far and removed the call to expand accessed symbols as well.
But that is fine, accessed symbols can't override anything.

[lrytz]

Here's the bytecode diff for the standard library: https://gist.github.com/lrytz/49a317a85e3372fa4fdce69f9bf215e6. There are a few getters and setters that are no longer mangled, but all of them are, and remain, private (in bytecode).

The reason for these changes: before this PR, calling expandName on a getter caused expandName(getterSym) -> expandName(fieldSym) -> expandName(setterSym). Now the last call is omitted. Since we started off from a private getter, the setter is private too and its name doesn't matter for bin compat. This assumption (private getter implies private setter) was always baked into expandName, otherwise the old implementation would not be safe.

What's not correct (and causing the bug) is the assumption that a private accessor implies a private getter/setter. When some transformation calls expandName(fieldSym), it's not correct to change public accessor names. In the concrete case, the count field is made non-private (an name mangled) during constructors, when the assignment to count is added to the specialized subclass of Buffer.

I guess the original intent was to keep consistency between getter/setter/field names. The fact that renaming the field seems to be necessary for ExtensionMethods is maybe a sign that this assumption is used in some places? Would be worth looking at this more closely.

Worth noting that the consistency was only in the names, not in the flags; makeNotPrivate(getter) changes the name of the setter, but keeps the setter private. That's also a bit of a strange inconsistency.

So overall, it's a risky area to touch, but I think there's a chance that we can do the change. If possible I think the accessed.expandName(base) should be removed too. https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/ExpandPrivate.scala might be worth reading as well.

[joroKr21]

Here's the bytecode diff for the standard library: gist.github.com/lrytz/49a317a85e3372fa4fdce69f9bf215e6. There are a few getters and setters that are no longer mangled, but all of them are, and remain private (in bytecode).

Nice 👍 How can I produce such a diff for future reference?

So before we had expandName(getterSym) -> expandName(fieldSym) -> expandName(setterSym). Now the last call is omitted, but we started off from a private getter, so the setter is private too and its name doesn't matter for bin compat. This assumption (private getter implies private setter) was always baked into expandName, otherwise the old implementation would not be safe.

Very good analysis

What's not safe (and causing the bug) is the assumption that a private accessor implies a private getter/setter. It seems the count field is made non-private (an name mangled) during constructors, when the assignment to count is added to the specialized subclass of Buffer.

Small correction. The cause of the bug is expandName(fieldSym) -> expandName(getterSym) -> expandName(setterSym), i.e. we are making the field non-private to be able to initialize it in the specialized class. Here the assumption that the getter / setter are private breaks down.

I guess the original intent was to keep consistency between getter/setter/field names. The fact that renaming the field seems to be necessary for ExtensionMethods is maybe a sign that this assumption is used in some places? Would be worth looking at this more closely.

Good point 🤔

Worth noting that the consistency was only in the names, not in the flags; makeNotPrivate(getter) changes the name of the setter, but keeps the setter private. That's also a bit of a strange inconsistency.

Yes, I'm not sure it was intentional to even expand related names.

So overall, it's a risky area to touch, but I think there's a chance that we can do the change. If possible I think the accessed.expandName(base) should be removed too.

I think it can be removed but results in a bigger diff to the existing tests (e.g. we get completions in the presentation compiler of self from extension methods). Should I push a commit to show that?

[lrytz]

Nice 👍 How can I produce such a diff for future reference?

I use https://github.com/dwijnand/scala-runners and https://github.com/scala/jardiff, then

rm -rf a b && mkdir a b
scalac --scala-version 2.13.6 $(find ../src/library -name '*.scala') -d a
scalac --scala-pr 9703 $(find ../src/library -name '*.scala') -d b
jardiff a b

Should I push a commit to show that?

Sure, if you have it already?

[joroKr21]

Hmm, now serialization stability failed 🤔

[lrytz]

Ah serialization...

@SerialVersionUID(42L)
class C extends Serializable {
  private[C] val foo = 33
  class D {
    def bar = foo
  }
}

This class has field C$$foo before, and foo after, which breaks serialization compatibility. This is what happens for outerEnum.

So we cannot do the change for field names. Can you add that as a comment to the source code?

[lrytz]

LGTM, but I'd also like to have a second opinion here. I update my comment above #9703 (comment); @retronym could you take a look?

A slightly more conservative change would be this, but I think the current change is safe and clener.

         else if (hasGetter) {
-          getterIn(owner).expandName(base)
-          setterIn(owner).expandName(base)
+          getterIn(owner).filter(_.isPrivate).expandName(base)
+          setterIn(owner).filter(_.isPrivate).expandName(base)
         }
         name = nme.expandedName(name.toTermName, base)

[retronym]

LGTM after a careful reading of @lrytz's analysis.

One thing that we could change in this area would be use JEP 181 if we compiling with -target 11.

We could then declare companions, inner classes and specialized subclasses as "nest mates", which would allow us to leave things JVM private.

[joroKr21]

One thing that we could change in this area would be use JEP 181 if we compiling with -target 11.

Oh wow didn't know about that. We could use it in many cases, also in Scala 3 👍