Skip to content

Align unpickled Scala 2 accessors encoding with Scala 3 #18874

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

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Align unpickled Scala 2 accessors encoding with Scala 3 #18874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
032f6cd
Move Scala 2 library tests from Windows into Linux
nicolasstucki Nov 7, 2023
d04b3c7
Align unpickled Scala 2 accessors encoding with Scala 3
nicolasstucki Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ jobs:
run: |
./project/scripts/sbt ";sjsSandbox/run ;sjsSandbox/test ;sjsJUnitTests/test ;set sjsJUnitTests/scalaJSLinkerConfig ~= switchToESModules ;sjsJUnitTests/test ;sjsCompilerTests/test"

- name: Test with Scala 2 library TASTy
run: ./project/scripts/sbt ";set ThisBuild/Build.useScala2LibraryTasty := true ;scala3-bootstrapped/testCompilation i5; scala3-bootstrapped/testCompilation tests/run/typelevel-peano.scala" # only test a subset of test to avoid doubling the CI execution time

test_windows_fast:
runs-on: [self-hosted, Windows]
if: "(
Expand Down Expand Up @@ -209,10 +212,6 @@ jobs:
run: sbt ";dist/pack ;scala3-bootstrapped/compile ;scala3-bootstrapped/test"
shell: cmd

- name: Test with Scala 2 library TASTy
run: sbt ";set ThisBuild/Build.useScala2LibraryTasty := true ;scala3-bootstrapped/testCompilation i5" # only test a subset of test to avoid doubling the CI execution time
shell: cmd

- name: Scala.js Test
run: sbt ";sjsJUnitTests/test ;set sjsJUnitTests/scalaJSLinkerConfig ~= switchToESModules ;sjsJUnitTests/test ;sjsCompilerTests/test"
shell: cmd
Expand Down
2 changes: 1 addition & 1 deletion community-build/community-projects/verify
Open in desktop
Submodule verify updated from 073921 to ae37d7
17 changes: 16 additions & 1 deletion compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,10 +450,18 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
// Scala 2 sometimes pickle the same type parameter symbol multiple times
// (see i11173 for an example), but we should only unpickle it once.
|| tag == TYPEsym && flags.is(TypeParam) && symScope(owner).lookup(name.asTypeName).exists
// We discard the private val representing a case accessor. We only load the case accessor def.
|| flags.isAllOf(CaseAccessor| PrivateLocal, butNot = Method)
then
// skip this member
return NoSymbol

// Adapt the flags of getters so they become like vals/vars instead.
// The info of this symbol is adapted in the `LocalUnpickler`.
if flags.isAllOf(Method | Accessor) && !name.toString().endsWith("_$eq") then
flags &~= Method | Accessor
if !flags.is(StableRealizable) then flags |= Mutable

name = name.adjustIfModuleClass(flags)
if (flags.is(Method))
name =
Expand Down Expand Up @@ -618,7 +626,14 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
setClassInfo(denot, tp, fromScala2 = true, selfInfo)
NamerOps.addConstructorProxies(denot.classSymbol)
case denot =>
val tp1 = translateTempPoly(tp)
val tp1 = translateTempPoly(tp) match
case ExprType(resultType) if !denot.isOneOf(Param | Method) =>
// Adapt the flags of getters so they become like vals/vars instead.
// This is the `def` of an accessor that needs to be transformed into
// a `val`/`var`. Note that the `Method | Accessor` flags were already
// striped away in `readDisambiguatedSymbol`.
resultType
case tp1 => tp1
denot.info =
if (tag == ALIASsym) TypeAlias(tp1)
else if (denot.isType) checkNonCyclic(denot.symbol, tp1, reportErrors = false)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/inlines/InlineReducer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) then paramCls.caseAccessors
else paramCls.asClass.paramAccessors
val selectors =
for (accessor <- caseAccessors)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/PatternMatcher.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ object PatternMatcher {
/** Plan for matching the result of an unapply against argument patterns `args` */
def unapplyPlan(unapp: Tree, args: List[Tree]): Plan = {
def caseClass = unapp.symbol.owner.linkedClass
lazy val caseAccessors = caseClass.caseAccessors.filter(_.is(Method))
lazy val caseAccessors = caseClass.caseAccessors

def isSyntheticScala2Unapply(sym: Symbol) =
sym.isAllOf(SyntheticCase) && sym.owner.is(Scala2x)
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Synthesizer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ class Synthesizer(typer: Typer)(using @constructorOnly c: Context):
New(defn.RuntimeTupleMirrorTypeRef, Literal(Constant(arity)) :: Nil)

def makeProductMirror(pre: Type, cls: Symbol, tps: Option[List[Type]]): TreeWithErrors =
val accessors = cls.caseAccessors.filterNot(_.isAllOf(PrivateLocal))
val accessors = cls.caseAccessors
val elemLabels = accessors.map(acc => ConstantType(Constant(acc.name.toString)))
val typeElems = tps.getOrElse(accessors.map(mirroredType.resultType.memberInfo(_).widenExpr))
val nestedPairs = TypeOps.nestedPairs(typeElems)
Expand Down
2 changes: 1 addition & 1 deletion language-server/test/dotty/tools/languageserver/CompletionTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class CompletionTest {
code"class Foo { val foo: BigD${m1} }"
.completion(
("BigDecimal", Field, "BigDecimal"),
("BigDecimal", Method, "=> scala.math.BigDecimal.type"),
("BigDecimal", Field, "scala.math.BigDecimal"),
)
}

Expand Down
2 changes: 1 addition & 1 deletion presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionDocSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ class CompletionDocSuite extends BaseCompletionSuite:
""".stripMargin,
"""
|> Found documentation for scala/package.Vector.
|Vector scala.collection.immutable
|Vector: scala.collection.immutable
|""".stripMargin,
includeDocs = true
)
Expand Down
8 changes: 4 additions & 4 deletions presentation-compiler/test/dotty/tools/pc/tests/completion/CompletionSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class CompletionSuite extends BaseCompletionSuite:
| Lis@@
|}""".stripMargin,
"""
|List scala.collection.immutable
|List: scala.collection.immutable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is the correct completion label.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this PR going to be backported to LTS or is it targeted for 3.4 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@rochala rochala Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

|List - java.awt
|List - java.util
|List - scala.collection.immutable
Expand Down Expand Up @@ -647,7 +647,7 @@ class CompletionSuite extends BaseCompletionSuite:
|}
|""".stripMargin,
"""|None scala
|NoManifest scala.reflect
|NoManifest: scala.reflect
|""".stripMargin,
topLines = Some(2)
)
Expand All @@ -660,8 +660,8 @@ class CompletionSuite extends BaseCompletionSuite:
|}
|""".stripMargin,
"""|Some(value) scala
|Seq scala.collection.immutable
|Set scala.collection.immutable
|Seq: scala.collection.immutable
|Set: scala.collection.immutable
|""".stripMargin,
topLines = Some(3)
)
Expand Down
14 changes: 7 additions & 7 deletions presentation-compiler/test/dotty/tools/pc/tests/tokens/SemanticTokensSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class SemanticTokensSuite extends BaseSemanticTokensSuite:
s"""|package <<example>>/*namespace*/
|
|object <<A>>/*class*/ {
| val <<x>>/*variable,definition,readonly*/ = <<List>>/*class*/(1,2,3)
| val <<x>>/*variable,definition,readonly*/ = <<List>>/*variable,readonly*/(1,2,3)
| val <<s>>/*variable,definition,readonly*/ = <<Some>>/*class*/(1)
| val <<Some>>/*class*/(<<s1>>/*variable,definition,readonly*/) = <<s>>/*variable,readonly*/
| val <<Some>>/*class*/(<<s2>>/*variable,definition,readonly*/) = <<s>>/*variable,readonly*/
Expand Down Expand Up @@ -269,7 +269,7 @@ class SemanticTokensSuite extends BaseSemanticTokensSuite:
|object <<A>>/*class*/ {
| val <<a>>/*variable,definition,readonly*/ = 1
| var <<b>>/*variable,definition*/ = 2
| val <<c>>/*variable,definition,readonly*/ = <<List>>/*class*/(1,<<a>>/*variable,readonly*/,<<b>>/*variable*/)
| val <<c>>/*variable,definition,readonly*/ = <<List>>/*variable,readonly*/(1,<<a>>/*variable,readonly*/,<<b>>/*variable*/)
| <<b>>/*variable*/ = <<a>>/*variable,readonly*/
|""".stripMargin
)
Expand All @@ -278,10 +278,10 @@ class SemanticTokensSuite extends BaseSemanticTokensSuite:
check(
"""
|object <<Main>>/*class*/ {
| val <<a>>/*variable,definition,readonly*/ = <<List>>/*class*/(1,2,3)
| val <<y>>/*variable,definition,readonly*/ = <<Vector>>/*class*/(1,2)
| val <<z>>/*variable,definition,readonly*/ = <<Set>>/*class*/(1,2,3)
| val <<w>>/*variable,definition,readonly*/ = <<Right>>/*class*/(1)
|val <<a>>/*variable,definition,readonly*/ = <<List>>/*variable,readonly*/(1,2,3)
|val <<y>>/*variable,definition,readonly*/ = <<Vector>>/*variable,readonly*/(1,2)
|val <<z>>/*variable,definition,readonly*/ = <<Set>>/*variable,readonly*/(1,2,3)
|val <<w>>/*variable,definition,readonly*/ = <<Right>>/*variable,readonly*/(1)
|}""".stripMargin
)

Expand Down Expand Up @@ -326,7 +326,7 @@ class SemanticTokensSuite extends BaseSemanticTokensSuite:
|
|object <<B>>/*class*/ {
| val <<a>>/*variable,definition,readonly*/ = for {
| <<foo>>/*variable,definition,readonly*/ <- <<List>>/*class*/("a", "b", "c")
| <<foo>>/*variable,definition,readonly*/ <- <<List>>/*variable,readonly*/("a", "b", "c")
| <<_>>/*class,abstract*/ = <<println>>/*method*/("print!")
| } yield <<foo>>/*variable,readonly*/
|}
Expand Down
19 changes: 1 addition & 18 deletions tests/coverage/pos/For.scoverage.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,23 +262,6 @@ covtest
For$package$
Object
covtest.For$package$
testForeach
301
304
13
Nil
Ident
false
0
false
Nil

15
For.scala
covtest
For$package$
Object
covtest.For$package$
$anonfun
318
343
Expand All @@ -290,7 +273,7 @@ false
false
println("user code here")

16
15
For.scala
covtest
For$package$
Expand Down
52 changes: 9 additions & 43 deletions tests/coverage/pos/Inlined.scoverage.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,23 +94,6 @@ Object
covtest.Inlined$package$
testInlined
155
159
6
List
Ident
false
0
false
List

5
Inlined.scala
covtest
Inlined$package$
Object
covtest.Inlined$package$
testInlined
155
169
6
length
Expand All @@ -120,7 +103,7 @@ false
false
List(l).length

6
5
Inlined.scala
covtest
Inlined$package$
Expand All @@ -137,7 +120,7 @@ false
false
scala.runtime.Scala3RunTime.assertFailed()

7
6
Inlined.scala
covtest
Inlined$package$
Expand All @@ -154,7 +137,7 @@ true
false
scala.runtime.Scala3RunTime.assertFailed()

8
7
Inlined.scala
covtest
Inlined$package$
Expand All @@ -171,7 +154,7 @@ true
false


9
8
Inlined.scala
covtest
Inlined$package$
Expand All @@ -188,24 +171,7 @@ false
false
List(l)

10
Inlined.scala
covtest
Inlined$package$
Object
covtest.Inlined$package$
testInlined
180
184
7
List
Ident
false
0
false
List

11
9
Inlined.scala
covtest
Inlined$package$
Expand All @@ -222,7 +188,7 @@ false
false
List(l).length

12
10
Inlined.scala
covtest
Inlined$package$
Expand All @@ -239,7 +205,7 @@ false
false
scala.runtime.Scala3RunTime.assertFailed()

13
11
Inlined.scala
covtest
Inlined$package$
Expand All @@ -256,7 +222,7 @@ true
false
scala.runtime.Scala3RunTime.assertFailed()

14
12
Inlined.scala
covtest
Inlined$package$
Expand All @@ -273,7 +239,7 @@ true
false


15
13
Inlined.scala
covtest
Inlined$package$
Expand Down
Loading