Align unpickled Scala 2 accessors encoding with Scala 3 #18874
Conversation
nicolasstucki
changed the title
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
from
323b29a
to
cce6c89
Compare
This comment was marked as outdated.
This comment was marked as outdated.
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
2 times, most recently
from
a2a3f39
to
831c3c7
Compare
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
from
831c3c7
to
683af6c
Compare
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
3 times, most recently
from
d041410
to
c1a6f88
Compare
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
2 times, most recently
from
2dfe63c
to
6da4d4a
Compare
nicolasstucki
changed the title
|
This PR has completely shifted from the original draft. |
| @@ -329,7 +329,7 @@ class InlineReducer(inliner: Inliner)(using Context): | |||
| val paramCls = paramType.classSymbol | |||
| if (paramCls.is(Case) && unapp.symbol.is(Synthetic) && scrut <:< paramType) { | |||
| val caseAccessors = | |||
| if (paramCls.is(Scala2x)) paramCls.caseAccessors.filter(_.is(Method)) | |||
| if (paramCls.is(Scala2x, butNot = Scala2Tasty)) paramCls.caseAccessors.filter(_.is(Method)) | |||
There was a problem hiding this comment.
When we load the Scala 2 library TASTy we get the accessor as a val but if we load the library from classfiles we get both a val and a def. This guard is to avoid the filtering.
// Scala 2
case class Foo extends AnyRef with Product with Serializable {
<caseaccessor> <paramaccessor> private[this] val x: Int = _;
<stable> <caseaccessor> <accessor> <paramaccessor> def x: Int = Foo.this.x;
...// Scala 3
case <noinits> <touched> class Foo <method> <stable> <touched>(
<param> <touched> x: Int) extends Object(), _root_.scala.Product, _root_.
scala.Serializable {
<paramaccessor> <caseaccessor> <touched> val x: Int
...
}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Now I fixed the Scala2Unpickler to avoid loading the private val of case accessor and interpret all accessors as val/vars.
| @@ -25,7 +25,7 @@ class CompletionSuite extends BaseCompletionSuite: | |||
| | Lis@@ | |||
| |}""".stripMargin, | |||
| """ | |||
| |List scala.collection.immutable | |||
| |List: scala.collection.immutable | |||
There was a problem hiding this comment.
@rochala the List accessor changed from a getter into a proper value. I see that this has changed. Is this the correct output? Or is this a symptom of some logic not identifying the accessor correctly and not dealiasing it? I remember that there are some special cases for these aliases.
There was a problem hiding this comment.
Yes, this is the correct completion label.
There was a problem hiding this comment.
Is this PR going to be backported to LTS or is it targeted for 3.4 ?
There was a problem hiding this comment.
Not sure if we can. It would break some macros. We should at least see what happens in the open community build before we consider it as an option.
There was a problem hiding this comment.
OK, this will make backporting a bit harder as tests will diverge between LTS and Next. This is not a big deal, but will be a small pain. I'll investigate whether the labels can print in the same fashion.
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
from
6da4d4a
to
1d44b37
Compare
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
from
1d44b37
to
aa1e29e
Compare
Adapt the flags of getters so they become like vals/vars instead. The getters flags and info are modified. The private fields for case accessors are not unpickled anymore. We need these getters to be aligned with Scala 3 if we want to be able to use the Scala 2 library TASTy. Otherwise library classfiles would not behave in the same way as their TASTy counterpart. This change may cause some macros to fail if they relied on the old style accessors. This should be adapted to work with the old and new representation. We observed that such a change is needed in `verify`, other might be detected with the open community build.
nicolasstucki
force-pushed
the
fix-ci-for-scala2-library
branch
from
aa1e29e
to
d04b3c7
Compare
|
Open community build test
Possible
|
| false | ||
| 0 | ||
| false | ||
| List |
There was a problem hiding this comment.
Not sure if these changes in the coverage are fine. This seems to relate with the List accessor in Predef. Coverage might not be tracking it anymore because now it is a val instead of a def and there fore it does not consider it as a method call.
This was changed in #18874 connected to scalameta/metals#5885
|
on Discord today, @WojciechMazur wrote:
|
Adapt the flags of getters so they become like vals/vars instead. The
getters flags and info are modified. The private fields for case
accessors are not unpickled anymore.
We need these getters to be aligned with Scala 3 if we want to be able
to use the Scala 2 library TASTy. Otherwise library classfiles would not
behave in the same way as their TASTy counterpart.
This change may cause some macros to fail if they relied on the old
style accessors. This should be adapted to work with the old and new
representation. We observed that such a change is needed in
verify,other might be detected with the open community build.