Skip to content

Fix #14667: Remove mangling of inner AnyQualifiedName's in fullNameSeparated #14749

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 20, 2022
Merged

Fix #14667: Remove mangling of inner AnyQualifiedName's in fullNameSeparated #14749

merged 1 commit into from
Jul 20, 2022

Conversation

gagandeepkalra
Copy link
Contributor

@gagandeepkalra gagandeepkalra commented Mar 22, 2022 •
edited
Loading

resolves #14667

cc @sjrd

@griggt
Copy link
Contributor

griggt commented Mar 22, 2022

I think the linked ticket ought to be #14667 ?

@sjrd sjrd linked an issue Mar 23, 2022 that may be closed by this pull request
Private field with non-alphanumeric identifier exhibits incorrect behavior at runtime #14667
Closed
@sjrd
Copy link
Member

sjrd commented Mar 23, 2022

@odersky You originally introduced this mangling in e7b2325. It seems that removing it now doesn't cause any failure, and it does fix the linked issue. Is there any reason to suspect that the mangling may still be necessary in some cases, not covered by the CI?

@odersky
Copy link
Contributor

odersky commented Mar 23, 2022

I don't know. It seems to be a very fundamental change to mangle or not, and mangling is explicitly demanded in the comment. So if we drop it I suggest to add an extensive test suite that makes sure nothing goes wrong. Maybe do first some research where fullNames are used in the compiler.

@sjrd
Copy link
Member

sjrd commented Jul 19, 2022 •
edited
Loading

I finally took the time to investigate the places where we use fullNames. I instrumented the case name @ AnyQualifiedName(_, _) => that we are modifying in this PR, and there are only 3 sources:

  • Mixin.traitSetterName, which is the method we are fixing with this change.
  • TreeChecker.testDuplicate, which is only testing duplicate class names under -Ycheck, and therefore never affects compilation.
  • ExpandPrivate.ensurePrivateAccessible, which manipulates private symbols that are visible in the same compilation unit but in a different class; since it is in the same compilation unit, separate compilation is not impacted, and therefore potentially changing the encoding is not an issue.

Therefore, I think it is safe to remove the mangling in this case.

Moreover, removing the mangling will bring the case AnyQualfiedName behavior in line with the case SimpleName behavior. I believe that making them more consistent is actually a good sign that this change is a good one.

@gagandeepkalra @sjrd
Fix scala#14667: Remove mangling of inner AnyQualifiedName's in fullN…
1b4e0e2 
…ameSeparated.

This brings the `case name @ AnyQualfiedName(_, _)` in line with
the behavior of `case naem: SimpleName`, which already did not
mangle before this change.

This fixes scala#14667 because `Mixin.traitSetterName` indirectly calls
`fullNameSeparated`, and expanded private symbolic member names
would get mangled, and therefore could not identify their
corresponding fields anymore.
@sjrd
Copy link
Member

sjrd commented Jul 19, 2022

I rebased and elaborated the commit message.

@sjrd sjrd requested a review from odersky July 19, 2022 16:14
@sjrd sjrd assigned odersky and unassigned sjrd Jul 19, 2022
@sjrd sjrd changed the title Remove mangling for non alphanumeric characters Fix #14667: Remove mangling of inner AnyQualifiedName's in fullNameSeparated Jul 20, 2022
odersky
odersky reviewed Jul 20, 2022
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Let's see whether we can drop the clause altigether.

compiler/src/dotty/tools/dotc/core/SymDenotations.scala
@@ -488,7 +488,7 @@ object SymDenotations {
if kind == FlatName && !encl.is(JavaDefined) then qn.compactified else qn
val fn = name replace {
case name: SimpleName => qualify(name)
case name @ AnyQualifiedName(_, _) => qualify(name.mangled.toSimpleName)
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact I now doubt whether we should have this case at all. If we remove this line, then #15702 succeeds. So if we can to this, it would be better.

odersky
odersky approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

I just found out that at least for trait setters dropping the clause does not work unfortunately. So I think this is at least a step in the right direction.

@odersky odersky merged commit 6c7acf9 into scala:main Jul 20, 2022
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@odersky odersky odersky approved these changes

Assignees

@odersky odersky

Labels
None yet
Projects
None yet
Milestone
3.2.1
Development

Successfully merging this pull request may close these issues.

Private field with non-alphanumeric identifier exhibits incorrect behavior at runtime
5 participants
@gagandeepkalra @griggt @sjrd @odersky @Kordyjan