Skip to content

Fix Scala Wart about implicit () class parameters #14840

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.

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 7 commits into from
Apr 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 9 additions & 5 deletions compiler/src/dotty/tools/dotc/ast/Desugar.scala
Original file line number Diff line number Diff line change
Expand Up @@ -585,12 +585,16 @@ object desugar {

// new C[Ts](paramss)
lazy val creatorExpr = {
val vparamss = constrVparamss match {
case (vparam :: _) :: _ if vparam.mods.isOneOf(GivenOrImplicit) => // add a leading () to match class parameters
val vparamss = constrVparamss match
case (vparam :: _) :: _ if vparam.mods.is(Implicit) => // add a leading () to match class parameters
Nil :: constrVparamss
case _ =>
constrVparamss
}
if constrVparamss.nonEmpty && constrVparamss.forall {
case vparam :: _ => vparam.mods.is(Given)
case _ => false
}
then constrVparamss :+ Nil // add a trailing () to match class parameters
else constrVparamss
val nu = vparamss.foldLeft(makeNew(classTypeRef)) { (nu, vparams) =>
val app = Apply(nu, vparams.map(refOfDef))
vparams match {
Expand Down Expand Up @@ -818,7 +822,7 @@ object desugar {
}

flatTree(cdef1 :: companions ::: implicitWrappers ::: enumScaffolding)
}.showing(i"desugared: $result", Printers.desugar)
}.showing(i"desugared: $cdef --> $result", Printers.desugar)

/** Expand
*
Expand Down
8 changes: 6 additions & 2 deletions compiler/src/dotty/tools/dotc/config/PathResolver.scala
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,9 @@ object PathResolver {

def fromPathString(path: String)(using Context): ClassPath = {
val settings = ctx.settings.classpath.update(path)
new PathResolver()(using ctx.fresh.setSettings(settings)).result
inContext(ctx.fresh.setSettings(settings)) {
new PathResolver().result
}
}

/** Show values in Environment and Defaults when no argument is provided.
Expand All @@ -147,7 +149,9 @@ object PathResolver {
val ArgsSummary(sstate, rest, errors, warnings) =
ctx.settings.processArguments(args.toList, true, ctx.settingsState)
errors.foreach(println)
val pr = new PathResolver()(using ctx.fresh.setSettings(sstate))
val pr = inContext(ctx.fresh.setSettings(sstate)) {
new PathResolver()
}
println(" COMMAND: 'scala %s'".format(args.mkString(" ")))
println("RESIDUAL: 'scala %s'\n".format(rest.mkString(" ")))

Expand Down
17 changes: 13 additions & 4 deletions compiler/src/dotty/tools/dotc/core/NamerOps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,23 @@ object NamerOps:
case TypeSymbols(tparams) :: _ => ctor.owner.typeRef.appliedTo(tparams.map(_.typeRef))
case _ => ctor.owner.typeRef

/** if isConstructor, make sure it has one leading non-implicit parameter list */
/** If isConstructor, make sure it has at least one non-implicit parameter list
* This is done by adding a () in front of a leading old style implicit parameter,
* or by adding a () as last -- or only -- parameter list if the constructor has
* only using clauses as parameters.
*/
def normalizeIfConstructor(paramss: List[List[Symbol]], isConstructor: Boolean)(using Context): List[List[Symbol]] =
if !isConstructor then paramss
else paramss match
case Nil :: _ => paramss
case TermSymbols(vparam :: _) :: _ if !vparam.isOneOf(GivenOrImplicit) => paramss
case TypeSymbols(tparams) :: paramss1 => tparams :: normalizeIfConstructor(paramss1, isConstructor)
case _ => Nil :: paramss
case TermSymbols(vparam :: _) :: _ if vparam.is(Implicit) => Nil :: paramss
case _ =>
if paramss.forall {
case TermSymbols(vparams) => vparams.nonEmpty && vparams.head.is(Given)
case _ => true
}
then paramss :+ Nil
else paramss

/** The method type corresponding to given parameters and result type */
def methodType(paramss: List[List[Symbol]], resultType: Type, isJava: Boolean = false)(using Context): Type =
Expand Down
46 changes: 44 additions & 2 deletions compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import typer.ConstFold
import typer.Checking.checkNonCyclic
import typer.Nullables._
import util.Spans._
import util.SourceFile
import util.{SourceFile, Property}
import ast.{Trees, tpd, untpd}
import Trees._
import Decorators._
Expand Down Expand Up @@ -1125,6 +1125,36 @@ class TreeUnpickler(reader: TastyReader,
readPathTerm()
}

/** Adapt constructor calls where class has only using clauses from old to new scheme.
* or class has mixed using clauses and other clauses.
* Old: leading (), new: nothing, or trailing () if all clauses are using clauses.
* This is neccessary so that we can read pre-3.2 Tasty correctly. There,
* constructor calls use the old scheme, but constructor definitions already
* use the new scheme, since they are reconstituted with normalizeIfConstructor.
*/
def constructorApply(fn: Tree, args: List[Tree]): Tree =
if fn.tpe.widen.isContextualMethod && args.isEmpty then
fn.withAttachment(SuppressedApplyToNone, ())
else
val fn1 = fn match
case Apply(fn1, Nil) if fn.removeAttachment(InsertedApplyToNone).isDefined =>
// We thought we inserted a final `()` but hit a user-written `()` instead.
// Remove the inserted `()`.
fn1
case _ =>
fn
val res = tpd.Apply(fn1, args)
if fn.removeAttachment(SuppressedApplyToNone).isEmpty then
res
else res.tpe.widen match
case mt @ MethodType(params) =>
if params.isEmpty then
// Assume it's the final synthesized `()` parameter
res.appliedToNone.withAttachment(InsertedApplyToNone, ())
else if mt.isContextualMethod then
res.withAttachment(SuppressedApplyToNone, ())
else res

def readLengthTerm(): Tree = {
val end = readEnd()
val result =
Expand All @@ -1135,7 +1165,9 @@ 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.

if fn.symbol.isConstructor then constructorApply(fn, args)
else tpd.Apply(fn, args)
case TYPEAPPLY =>
tpd.TypeApply(readTerm(), until(end)(readTpt()))
case TYPED =>
Expand Down Expand Up @@ -1523,4 +1555,14 @@ object TreeUnpickler {
inline val AllDefs = 2 // add everything

class TreeWithoutOwner extends Exception

/** An attachment key indicating that an old-style leading () in a constructor
* call that is followed by a using clause was suppressed.
*/
val SuppressedApplyToNone: Property.Key[Unit] = Property.Key()

/** An attachment key indicating that a trailing () in a constructor
* call that has otherwise only using clauses was inserted.
*/
val InsertedApplyToNone: Property.Key[Unit] = Property.Key()
}
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/Bridges.scala
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class Bridges(root: ClassSymbol, thisPhase: DenotTransformer)(using Context) {
* time deferred methods in `stats` that are replaced by a bridge with the same signature.
*/
def add(stats: List[untpd.Tree]): List[untpd.Tree] =
val opc = new BridgesCursor()(using preErasureCtx)
val opc = inContext(preErasureCtx) { new BridgesCursor }
while opc.hasNext do
if !opc.overriding.is(Deferred) then
addBridgeIfNeeded(opc.overriding, opc.overridden)
Expand Down
39 changes: 23 additions & 16 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2356,25 +2356,32 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer

/** If `ref` is an implicitly parameterized trait, pass an implicit argument list.
* Otherwise, if `ref` is a parameterized trait, error.
* Note: Traits and classes currently always have at least an empty parameter list ()
* before the implicit parameters (this is inserted if not given in source).
* We skip this parameter list when deciding whether a trait is parameterless or not.
* Note: Traits and classes have sometimes a synthesized empty parameter list ()
* in front or after the implicit parameter(s). See NamerOps.normalizeIfConstructor.
* We synthesize a () argument at the correct place in this case.
* @param ref The tree referring to the (parent) trait
* @param psym Its type symbol
* @param cinfo The info of its constructor
*/
def maybeCall(ref: Tree, psym: Symbol): Tree = psym.primaryConstructor.info.stripPoly match
case cinfo @ MethodType(Nil) if cinfo.resultType.isImplicitMethod =>
def maybeCall(ref: Tree, psym: Symbol): Tree =
def appliedRef =
typedExpr(untpd.New(untpd.TypedSplice(ref)(using superCtx), Nil))(using superCtx)
case cinfo @ MethodType(Nil) if !cinfo.resultType.isInstanceOf[MethodType] =>
ref
case cinfo: MethodType =>
if !ctx.erasedTypes then // after constructors arguments are passed in super call.
typr.println(i"constr type: $cinfo")
report.error(ParameterizedTypeLacksArguments(psym), ref.srcPos)
ref
case _ =>
ref
def dropContextual(tp: Type): Type = tp.stripPoly match
case mt: MethodType if mt.isContextualMethod => dropContextual(mt.resType)
case _ => tp
psym.primaryConstructor.info.stripPoly match
case cinfo @ MethodType(Nil)
if cinfo.resultType.isImplicitMethod && !cinfo.resultType.isContextualMethod =>
appliedRef
case cinfo =>
val cinfo1 = dropContextual(cinfo)
cinfo1 match
case cinfo1 @ MethodType(Nil) if !cinfo1.resultType.isInstanceOf[MethodType] =>
if cinfo1 ne cinfo then appliedRef else ref
case cinfo1: MethodType if !ctx.erasedTypes =>
report.error(ParameterizedTypeLacksArguments(psym), ref.srcPos)
ref
case _ =>
ref

val seenParents = mutable.Set[Symbol]()

Expand Down Expand Up @@ -3401,7 +3408,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
def isContextBoundParams = wtp.stripPoly match
case MethodType(EvidenceParamName(_) :: _) => true
case _ => false
if sourceVersion == `future-migration` && isContextBoundParams
if sourceVersion == `future-migration` && isContextBoundParams && pt.args.nonEmpty
then // Under future-migration, don't infer implicit arguments yet for parameters
// coming from context bounds. Issue a warning instead and offer a patch.
report.migrationWarning(
Expand Down
20 changes: 10 additions & 10 deletions tests/neg/i12344.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import scala.quoted.*

class C(using q: Quotes)(i: Int = 1, f: q.reflect.Flags = q.reflect.Flags.EmptyFlags)

def test1a(using q: Quotes) = new C() // error
def test2a(using q: Quotes) = new C(1) // error
def test3a(using q: Quotes) = new C(1, q.reflect.Flags.Lazy) // error
def test4a(using q: Quotes) = new C(f = q.reflect.Flags.Lazy) // error
def test1a(using q: Quotes) = new C()
def test2a(using q: Quotes) = new C(1)
def test3a(using q: Quotes) = new C(1, q.reflect.Flags.Lazy)
def test4a(using q: Quotes) = new C(f = q.reflect.Flags.Lazy)

def test1b(using q: Quotes) = C() // error
def test2b(using q: Quotes) = C(1) // error
def test3b(using q: Quotes) = C(1, q.reflect.Flags.Lazy) // error
def test4b(using q: Quotes) = C(f = q.reflect.Flags.Lazy) // error
def test1b(using q: Quotes) = C()
def test2b(using q: Quotes) = C(1)
def test3b(using q: Quotes) = C(1, q.reflect.Flags.Lazy)
def test4b(using q: Quotes) = C(f = q.reflect.Flags.Lazy)

def test1c(using q: Quotes) = new C(using q)()
def test2c(using q: Quotes) = new C(using q)(1)
Expand All @@ -22,5 +22,5 @@ def test2d(using q: Quotes) = C(using q)(1)
def test3d(using q: Quotes) = C(using q)(1, q.reflect.Flags.Lazy)
def test4d(using q: Quotes) = C(using q)(f = q.reflect.Flags.Lazy)

def test1e(using q: Quotes) = new C()()
def test2e(using q: Quotes) = C()()
def test1e(using q: Quotes) = new C()() // error
def test2e(using q: Quotes) = C()() // error
1 change: 0 additions & 1 deletion tests/pos/given-constrapps.scala
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ class Foo(using TC) {

object Test extends App {
new C(using tc)
new C()(using tc)
new C(using tc) {}
new C2(1)(using tc)(using List(tc))
new C2(1)(using tc)(using List(tc)) {}
Expand Down
3 changes: 3 additions & 0 deletions tests/pos/i2576.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Bar(using x: Int)(y: String)
given Int = ???
def test = new Bar("")
1 change: 1 addition & 0 deletions tests/run-macros/i12021/Macro_1.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def inspect2[A: Type](using Quotes): Expr[String] = {
val ps =
TypeRepr.of[A].typeSymbol.primaryConstructor.tree match
case DefDef(_, List(Nil, ps: TermParamClause), _, _) => ps
case DefDef(_, List(ps: TermParamClause, Nil), _, _) => ps
case DefDef(_, List(ps: TermParamClause), _, _) => ps

val names = ps.params.map(p => s"${p.name}: ${p.tpt.show}").mkString("(", ", ", ")")
Expand Down
20 changes: 20 additions & 0 deletions tests/run/backwardsCompat-implicitParens.check
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
Bar
Bar
Bar
Bar
()
Bat
Bat
Bat
Bat
()
Bax
Bax
Bax
Bax
()
Baz
Baz
Baz
Baz
()
32 changes: 32 additions & 0 deletions tests/run/backwardsCompat-implicitParens/A_1_c3.0.2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
class Bar(using x: Int)(y: String):
override def toString = "Bar"
object Bar:
given Int = 1
inline def foo =
println(new Bar()(""))
println(Bar()(""))

class Bat(using x: Int):
override def toString = "Bat"
object Bat:
given Int = 1
inline def foo =
println(new Bat())
println(Bat())

class Bax(using x: Int)():
override def toString = "Bax"
object Bax:
given Int = 1
inline def foo =
println(new Bax())
println(Bax())

class Baz(using x: Int)(using y: String):
override def toString = "Baz"
object Baz:
given Int = 1
given String = "x"
inline def foo =
println(new Baz())
println(Baz())
19 changes: 19 additions & 0 deletions tests/run/backwardsCompat-implicitParens/Test_2.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
@main def Test =
given Int = 1
given String = "x"

println(new Bar(""))
println(Bar(""))
println(Bar.foo)

println(new Bat())
println(Bat())
println(Bat.foo)

println(new Bax())
println(Bax())
println(Bax.foo)

println(new Baz())
println(Baz())
println(Baz.foo)
4 changes: 2 additions & 2 deletions tests/run/i2567.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ object Test extends App {
new Foo
new Foo(using tc)
new Foo()
new Foo()(using tc)
new Foo(using tc)
Foo()
Foo()(using tc)
Foo(using tc)
}
2 changes: 1 addition & 1 deletion tests/semanticdb/metac.expect
Original file line number Diff line number Diff line change
Expand Up @@ -1808,7 +1808,7 @@ givens/InventedNames$package.given_String. => final implicit lazy val given meth
givens/InventedNames$package.given_X. => final implicit given object given_X extends Object with X { self: given_X.type => +2 decls }
givens/InventedNames$package.given_X.doX(). => method doX => Int <: givens/X#doX().
givens/InventedNames$package.given_Y# => implicit given class given_Y extends Object with Y { self: given_Y => +3 decls }
givens/InventedNames$package.given_Y#`<init>`(). => primary ctor <init> ()(implicit val given param x$1: X): given_Y
givens/InventedNames$package.given_Y#`<init>`(). => primary ctor <init> (implicit val given param x$1: X)(): given_Y
givens/InventedNames$package.given_Y#`<init>`().(x$1) => implicit val given param x$1: X
givens/InventedNames$package.given_Y#doY(). => method doY => String <: givens/Y#doY().
givens/InventedNames$package.given_Y#x$1. => protected implicit val given method x$1 X
Expand Down