Reinterpret Scala 2 case accessors xyz$access$idx
#18907
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
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
2 times, most recently
from
c5842db
to
ce831a4
Compare
nicolasstucki
changed the title
::.next$access$1 when unpickling from Scala 2xyz$access$idx when unpickling from Scala 2
nicolasstucki
changed the title
xyz$access$idx when unpickling from Scala 2xyz$access$idx
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
5 times, most recently
from
d3e3d16
to
c71e8a1
Compare
nicolasstucki
commented
Nov 14, 2023
sjrd
requested changes
Nov 16, 2023
| @@ -27,12 +27,14 @@ object TestSources { | |||
|
|
|||
| def runFromTastyBlacklistFile: String = "compiler/test/dotc/run-from-tasty.blacklist" | |||
| def runTestPicklingBlacklistFile: String = "compiler/test/dotc/run-test-pickling.blacklist" | |||
| def runScalaJSBlacklistFile: String = "compiler/test/dotc/run-scala-js.blacklist" | |||
There was a problem hiding this comment.
Use // scalajs: --skip in the test file instead.
compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala
Outdated
Show resolved
Hide resolved
| // undistinguishable from non-case parameter accessors (example | ||
| // `case class Foo(private[p] var x: T)(private[p] var y: T)`). | ||
| val accessorIndex = name.toString().split('$').last.toInt | ||
| val accessors = owner.asClass.info.decls |
There was a problem hiding this comment.
This is too dangerous at this point. Store the necessary information in a map like paramsOfMethodType and clean up when completing the class decls.
There was a problem hiding this comment.
Cleaning up when completing the symbol did not work #18907 (comment).
The other option I found is to use symScope(owner). This is also used above when skipping other symbols.
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
from
c71e8a1
to
3d8c24d
Compare
sjrd
requested changes
Nov 16, 2023
compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala
Outdated
Show resolved
Hide resolved
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
from
3d8c24d
to
88276c5
Compare
sjrd
previously approved these changes
Nov 16, 2023
compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala
Outdated
Show resolved
Hide resolved
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
from
88276c5
to
a2d2b8c
Compare
nicolasstucki
commented
Nov 17, 2023
| // If there was a `<xyz>$access$<idx>` case accessor, we also need to | ||
| // make this accessor a case accessor. | ||
| if paramAccessorNeedsCaseAccessor(denot.owner.asClass, denot.name) then | ||
| denot.setFlag(CaseAccessor) |
There was a problem hiding this comment.
This is setting the flag too late. We have failures with mirrors where we seem to access the symbol flags before we complete the signature.
tests/run/typeclass-derivation3.scala failed
tests/run/i13332intersection.scala failed
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
from
a2d2b8c
to
f857c98
Compare
nicolasstucki
dismissed
sjrd’s stale review
The implementation that was reviewed had a bug
nicolasstucki
force-pushed
the
remove-list-cons-next-access-1
branch
from
f857c98
to
b035f76
Compare
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.
Fixes #18884
Related to #18874