Skip to content
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

Jump to bottom

Fix Scala Wart about implicit () class parameters #14840

Merged
merged 7 commits into from
Apr 22, 2022
Merged

Fix Scala Wart about implicit () class parameters #14840

merged 7 commits into from
Apr 22, 2022

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 4, 2022

Fixes #2576

As the discussion in #2576 shows, we still have some problems with the implicitly
inserted empty parameter lists for class constructors. We do need that empty list
to support syntax like C() and new C(). But it gets in the way if a class has
using clauses. Example from the issue:

class Bar(using x: Int)(y: String)
given Int = ???
def test = new Bar("")

Here, an implicitly inserted () in front makes the last line fail. We'd need
new Bar()("").

If a class has only using clauses as parameters we now insert a () at the end
instead of at the start. That makes the example compile.

For old-style implicit parameters we don't have a choice. We still need the () at
the start since otherwise we'd change the meaning of calls with explicit arguments
for the implicit parameters.

@odersky
Fix Scala Wart about implicit () class parameters
fe40c59 
Fixes scala#2576

As the discussion in scala#2576 shows, we still have some problems with the implicitly
inserted empty parameter lists for class constructors. We do need that empty list
to support syntax like `C()` and `new C()`. But it gets in the way if a class has
using clauses. Example from the issue:
```scala
class Bar(using x: Int)(y: String)
given Int = ???
def test = new Bar("")
```
Here, an implicitly inserted `()` in front makes the last line fail. We'd need
`new Bar()("")`.

If a class has only using clauses as parameters we now insert a `()` at the end
instead of at the start. That makes the example compile.

For old-style implicit parameters we don't have a choice. We still need the `()` at
the start since otherwise we'd change the meaning of calls with explicit arguments
for the implicit parameters.
@odersky
Update semanticDB expect file
3763e6b
@odersky
Harden patch logic
2fc2aa2
@odersky odersky added the needs-minor-release This PR cannot be merged until the next minor release label Apr 4, 2022
@nicolasstucki nicolasstucki added this to the 3.2.0-RC1 milestone Apr 4, 2022
@odersky
Make TreeUnpickler read pre-3.2 Tasty files correctly
00a839e 
This is needed to ensure backwards Tasty compatibility
@odersky odersky requested a review from smarter April 4, 2022 20:50
@odersky odersky assigned odersky and smarter and unassigned odersky Apr 4, 2022
@bishabosha bishabosha requested a review from sjrd April 5, 2022 08:51
@smarter
Test tasty backwards compatibility
df9f466 
This currently fail with:

MethodType(List(y), List(TypeRef(TermRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),Predef),String)), TypeRef(ThisType(TypeRef(NoPrefix,module class <empty>)),class Bar)) (of class dotty.tools.dotc.core.Types$CachedMethodType)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1154)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1313)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1137)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1313)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.$anonfun$24(TreeUnpickler.scala:1138)
        at dotty.tools.tasty.TastyReader.until(TastyReader.scala:125)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1138)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1313)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedStat(TreeUnpickler.scala:998)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedStats$$anonfun$1(TreeUnpickler.scala:1036)
        at dotty.tools.tasty.TastyReader.until(TastyReader.scala:125)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readIndexedStats(TreeUnpickler.scala:1036)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readStats(TreeUnpickler.scala:1040)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1166)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1313)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readLengthTerm$1(TreeUnpickler.scala:1158)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.readTerm(TreeUnpickler.scala:1313)
        at dotty.tools.dotc.core.tasty.TreeUnpickler$TreeReader.createMemberSymbol$$anonfun$1(TreeUnpickler.scala:614)
        at dotty.tools.dotc.core.Annotations$.dotty$tools$dotc$core$Annotations$$anon$2$$_$$lessinit$greater$$anonfun$1(Annotations.scala:162)
        at dotty.tools.dotc.core.Annotations$LazyBodyAnnotation.tree(Annotations.scala:150)
        at dotty.tools.dotc.typer.Inliner$.bodyToInline(Inliner.scala:53)
        at dotty.tools.dotc.typer.Inliner$.inlineCall(Inliner.scala:157)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:70)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform$$anonfun$1(Trees.scala:1494)
        at scala.collection.immutable.List.mapConserve(List.scala:472)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1494)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1388)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:73)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:78)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformBlock$$anonfun$1$$anonfun$1(tpd.scala:1211)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.loop$2(tpd.scala:1193)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformStats(tpd.scala:1206)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformBlock(tpd.scala:1211)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1402)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:49)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:66)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:56)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:64)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.loop$2(tpd.scala:1206)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformStats(tpd.scala:1206)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformStats(tpd.scala:1208)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:64)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:64)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1465)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:73)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:64)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.loop$2(tpd.scala:1206)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformStats(tpd.scala:1206)
        at dotty.tools.dotc.ast.tpd$TreeMapWithPreciseStatContexts.transformStats(tpd.scala:1208)
        at dotty.tools.dotc.ast.Trees$Instance$TreeMap.transform(Trees.scala:1476)
        at dotty.tools.dotc.ast.TreeMapWithImplicits.transform(TreeMapWithImplicits.scala:73)
        at dotty.tools.dotc.transform.Inlining$InliningTreeMap.transform(Inlining.scala:78)
        at dotty.tools.dotc.transform.Inlining$$anon$2.transform(Inlining.scala:56)
        at dotty.tools.dotc.transform.MacroTransform.run(MacroTransform.scala:18)
        at dotty.tools.dotc.transform.Inlining.run(Inlining.scala:28)
smarter
smarter requested changes Apr 5, 2022
View reviewed changes
compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
@@ -1135,7 +1135,23 @@ class TreeUnpickler(reader: TastyReader,
tpd.Super(qual, mixId, mixTpe.typeSymbol)
case APPLY =>
val fn = readTerm()
tpd.Apply(fn, until(end)(readTerm()))
val args = until(end)(readTerm())
Copy link
Member

Choose a reason for hiding this comment

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

I've added a commit to test backwards compatibility and when inlining a def from a previous compiler version, we crash when unpickling the arguments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I handled only one part before (where all parameters are using clauses). Now the mixed case is handled as well.

@smarter smarter assigned odersky and unassigned smarter Apr 5, 2022
@odersky odersky assigned smarter and unassigned odersky Apr 5, 2022
smarter
smarter reviewed Apr 5, 2022
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

The backwards compat support LGTM, but I think that forwards compat support via -scala-output-release will require more special-casing (new-style constructors need to be treated like old-style constructors when ctx.scalaRelease <= ScalaRelease.Release3_1), I don't have time right now to dig into this but perhaps @prolativ could take a look? It could be a good case study for the planned discussion on -scala-output-release.

@smarter smarter assigned prolativ and unassigned smarter Apr 5, 2022
@odersky
Copy link
Contributor Author

odersky commented Apr 6, 2022

I don't know how to reconcile this with -scala-output-release since it is a source breaking change. We can keep the old encoding under -source 3.0, but how that should interact with -scala-output-release. There are too many options interacting here for my taste.

@odersky
Copy link
Contributor Author

odersky commented Apr 20, 2022

I think this can be merged now?

@odersky odersky merged commit 74bc3fd into scala:main Apr 22, 2022
@odersky odersky deleted the fix-2576 branch April 22, 2022 09:18
@bishabosha bishabosha added the release-notes Should be mentioned in the release notes label May 11, 2022
@bishabosha bishabosha mentioned this pull request May 11, 2022
Closed
@nicolasstucki nicolasstucki mentioned this pull request May 17, 2022
Closed
@bishabosha
Copy link
Member

so it looks like this has caused some inference issues: #15171

@griggt griggt mentioned this pull request Jul 12, 2022
Closed
@WojciechMazur
Copy link
Contributor

WojciechMazur commented Jul 12, 2022 •
edited
Loading

@odersky can you take a look at reproducer snipper from #15647 for tinkoff/phobos failure

import scala.quoted.*
class ProductTypeField(using val quotes: Quotes)(val name: String)
def foo(using Quotes): ProductTypeField = ProductTypeField()("")

As I understood your comment for this PR, code ProductTypeField()("") should work fine, but ProductTypeField("") (without empty params for implicit parameters list) should fail. However, the first case fails in 3.2.x and the second one in 3.1.x
Or maybe I've understood this change incorrectly?

@odersky
Copy link
Contributor Author

odersky commented Jul 12, 2022 •
edited
Loading

Yes, it's the other way round. This works as expected.

@griggt griggt mentioned this pull request Jul 13, 2022
Closed
@soronpo soronpo mentioned this pull request Aug 19, 2022
Open
@Kordyjan Kordyjan mentioned this pull request Apr 4, 2023
Closed
@Sporarum Sporarum mentioned this pull request Jun 19, 2023
Open
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter requested changes

@sjrd sjrd Awaiting requested review from sjrd

Assignees

@prolativ prolativ

Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.

Scala Wart: Classes cannot have only implicit parameter lists
7 participants
@odersky @bishabosha @WojciechMazur @smarter @nicolasstucki @prolativ @Kordyjan