SI-7464 allows FieldMirror.set to update vals #2504
Merged
Conversation
This file contains 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
|
review @retronym |
|
PLS REBUILD/pr-checkin-per-commit@788d42a |
|
(kitty-note-to-self: ignore 17756701) |
|
@adriaanm what's up with the kitty? |
|
PLS REBUILD ALL |
|
(kitty-note-to-self: ignore 17913094) |
There's no reason to leave such sentinels in place inside a facility designed to circumvent usual restrictions of static types / visibility.
|
@retronym That probably will be a very easy review if you have some more time for reviews today. |
|
I'd also like to mention that this patch brings us to consistency with Java reflection. They also allow updating final fields (though only after calling |
|
LGTM. I was wondering whether we should (eventually) guard the use of the Scala reflection API with a set of permissions (for sandboxed environments.) But I'm not sure if that really makes sense. |
retronym
added a commit
that referenced
this pull request
May 17, 2013
SI-7464 allows FieldMirror.set to update vals
|
Sorry for the merging with the broken tests, the fix is now merged in #2550. |
eed3si9n
pushed a commit
to eed3si9n/scala
that referenced
this pull request
May 14, 2019
Add a pending test that shows a problem with instability of representing self variables. This test covers the bug described in scala#2504. In order to test API representation of a class declared either in source file or unpickled from a class file, ScalaCompilerForUnitTesting has been extended to extract APIs from multiple compiler instances sharing a classpath.
eed3si9n
pushed a commit
to eed3si9n/scala
that referenced
this pull request
May 14, 2019
The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if `thisSym` of a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, then `thisSym` represents either a self type or a self variable. The second (type test) was supposed to check whether the type of `thisSym` is different from a type of the class. However, it would always yield false because TypeRef of `thisSym` was compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you). Now the tricky bit: `thisSym` is not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, the symbol equality test would yield true and we would not see self type when just a self variable was declared. The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing. Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type. Fixes scala#2504.
lrytz
pushed a commit
to lrytz/scala
that referenced
this pull request
Nov 5, 2019
Add a pending test that shows a problem with instability of representing self variables. This test covers the bug described in scala#2504. In order to test API representation of a class declared either in source file or unpickled from a class file, ScalaCompilerForUnitTesting has been extended to extract APIs from multiple compiler instances sharing a classpath. Rewritten from sbt/zinc@3f26571
lrytz
pushed a commit
to lrytz/scala
that referenced
this pull request
Nov 5, 2019
The reason for instability is a bit tricky so let's unpack what the previous code checking if there's self type declared was doing. It would check if `thisSym` of a class is equal to a symbol representing the class. If that's true, we know that there's no self type. If it's false, then `thisSym` represents either a self type or a self variable. The second (type test) was supposed to check whether the type of `thisSym` is different from a type of the class. However, it would always yield false because TypeRef of `thisSym` was compared to ClassInfoType of a class. So if you had a self variable the logic would see a self type (and that's what API representation would give you). Now the tricky bit: `thisSym` is not pickled when it's representing just a self variable because self variable doesn't affect other classes referring to a class. If you looked at a type after unpickling, the symbol equality test would yield true and we would not see self type when just a self variable was declared. The fix is to check equality of type refs on both side of the type equality check. This makes the pending test passing. Also, I added another test that checks if self types are represented in various combinations of declaring a self variable or/and self type. Fixes scala#2504. Rewritten from sbt/zinc@81cbabf
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There's no reason to leave such sentinels in place inside a facility
designed to circumvent usual restrictions of static types / visibility.