SI-8944 A more resiliant naming scheme for case accessors#4125
Merged
gkossakowski merged 1 commit intoscala:2.12.xfrom Jan 14, 2015
Merged
SI-8944 A more resiliant naming scheme for case accessors#4125gkossakowski merged 1 commit intoscala:2.12.xfrom
gkossakowski merged 1 commit intoscala:2.12.xfrom
Conversation
538ea95 to
556388a
Compare
Case class parameters that are less that public have an extra accessor method created to ensure universal pattern matchability. See scala#4081 for more background. Currently, this is given a fresh name based on the parameter name. However, this is fragile and the name can change based on unrelated edits higher up in the source file. This commit switches to a stable naming scheme for these methods. A non-public case field `foo` has a corresponding accessor `foo$access$N`, where `N` is the index of the parameter within the constructor parameter list. The enclosed tests show a case that used to trigger a linkage error under separate compilation that now works; shows that by choosing the `foo$access$1` rather than `foo$1` we don't clash with lambda lifted methods in the class; and shows the names of the accessor methods as seen via Java reflection.
556388a to
ff2cd80
Compare
Contributor
|
@retronym, this looks fine to me. Would you like to merge? Also, would you consider deprecating private parameters to constructors of case classes? I think they make little sense. |
Member
Author
|
Let's get another opinion from @lrytz on the new scheme. It should be noted that |
Member
|
We could make |
Member
|
LGTM too |
Contributor
|
I'm not sure I'm all for adding an abide check, though, for those who don't mean to be weird. |
gkossakowski
added a commit
that referenced
this pull request
Jan 14, 2015
SI-8944 A more resiliant naming scheme for case accessors
Contributor
|
My heart sank when I learnt about this Scala feature but I give in to your reasons. :) |
retronym
added a commit
to retronym/scala
that referenced
this pull request
Mar 5, 2015
Case class parameters may be declared as non-public. If this is the case, the case accessor method is still synthesized as public, but it has a mangled name of the form `origName$N`. See also SI-8944 / scala#4125. The compiler maps from case fields to the corresponding accessors with this naming convention in mind. However, it is too lenient, and assumes that only one accessor will be of the form "origName\$.*". If, however, a regular, public case parameter is named `a b`, it will have a name `a$u0020b`, which spurously matches that overly permissive pattern. This commit changes tightens up the logic to expect "origName\$\d+".
adriaanm
pushed a commit
to adriaanm/scala
that referenced
this pull request
Jan 10, 2017
Case class parameters may be declared as non-public. If this is the case, the case accessor method is still synthesized as public, but it has a mangled name of the form `origName$N`. See also SI-8944 / scala#4125. The compiler maps from case fields to the corresponding accessors with this naming convention in mind. However, it is too lenient, and assumes that only one accessor will be of the form "origName\$.*". If, however, a regular, public case parameter is named `a b`, it will have a name `a$u0020b`, which spurously matches that overly permissive pattern. This commit changes tightens up the logic to expect "origName\$\d+".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Case class parameters that are less that public have an extra
accessor method created to ensure universal pattern matchability.
See #4081 for more background.
Currently, this is given a fresh name based on the parameter name.
However, this is fragile and the name can change based on unrelated
edits higher up in the source file.
This commit switches to a stable naming scheme for these methods.
A non-public case field
foohas a corresponding accessorfoo$access$N, whereNis the index of the parameter withinthe constructor parameter list.
The enclosed tests show a case that used to trigger a linkage
error under separate compilation that now works; shows that by
choosing the
foo$access$1rather thanfoo$1we don't clash withlambda lifted methods in the class; and shows the names of the
accessor methods as seen via Java reflection.
Just submitting for a test run for now.
/cc @gkossakowski who is usually has some good insights when it
comes to accessor naming schemes. This is one of the cross
compilation unit use cases that we have to deal with that Java
doesn't.