From c942f1f89ac575f63640496eda3be6f0993cf11d Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 27 Jan 2024 00:08:38 +0900 Subject: [PATCH 1/6] draft:fix(#19266): add test cases --- tests/pos/i19266.scala | 9 +++++++++ tests/warn/i19266.scala | 8 ++++++++ 2 files changed, 17 insertions(+) create mode 100644 tests/pos/i19266.scala create mode 100644 tests/warn/i19266.scala diff --git a/tests/pos/i19266.scala b/tests/pos/i19266.scala new file mode 100644 index 000000000000..b8a13c04f9d4 --- /dev/null +++ b/tests/pos/i19266.scala @@ -0,0 +1,9 @@ +object i19266: + def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z") + def fn2(x: Int)(y: String) = Some(s"$x$y") + + def checkCompile = + fn1(1) + // This should compile with warning + fn2(2) + val _ = fn2(3) diff --git a/tests/warn/i19266.scala b/tests/warn/i19266.scala new file mode 100644 index 000000000000..1813981eda1b --- /dev/null +++ b/tests/warn/i19266.scala @@ -0,0 +1,8 @@ +object i19266: + def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z") + def fn2(x: Int)(y: String) = Some(s"$x$y") + + def checkWarning = + fn1(1) // warn + fn2(2) // warn + val a = fn2(3) // warn From a953135b25baeeb865663e363f6b0b3769950d46 Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 27 Jan 2024 05:08:32 +0900 Subject: [PATCH 2/6] draft: better error for impure lambda in stmt position --- .../src/dotty/tools/dotc/typer/Typer.scala | 15 +++++++++----- tests/neg/i19266.check | 12 +++++++++++ tests/neg/i19266.scala | 20 +++++++++++++++++++ tests/pos/i19266.scala | 9 --------- 4 files changed, 42 insertions(+), 14 deletions(-) create mode 100644 tests/neg/i19266.check create mode 100644 tests/neg/i19266.scala delete mode 100644 tests/pos/i19266.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 3f3057836682..a30ecaf9ac55 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -4409,10 +4409,10 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer case _ => private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol, isUnitExpr: Boolean = false)(using Context): Unit = + val isPure = isPureExpr(tree) if !tree.tpe.isErroneous && !ctx.isAfterTyper && !tree.isInstanceOf[Inlined] - && isPureExpr(tree) && !isSelfOrSuperConstrCall(tree) then tree match case closureDef(meth) @@ -4425,11 +4425,16 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer // if the original tree was a lambda. This does not work always either since // sometimes we do not have the original anymore and use the transformed tree instead. // But taken together, the two criteria are quite accurate. - missingArgs(tree, tree.tpe.widen) + if isPure then + missingArgs(tree, tree.tpe.widen) + else + report.warning(UnusedNonUnitValue(tree.tpe)) case _ if isUnitExpr => - report.warning(PureUnitExpression(original, tree.tpe), original.srcPos) - case _ => - report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos) + if isPure then + report.warning(PureUnitExpression(original, tree.tpe), original.srcPos) + case _ => + if isPure then + report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos) /** Types the body Scala 2 macro declaration `def f = macro ` */ protected def typedScala2MacroBody(call: untpd.Tree)(using Context): Tree = diff --git a/tests/neg/i19266.check b/tests/neg/i19266.check new file mode 100644 index 000000000000..9c6fb8cdb3a4 --- /dev/null +++ b/tests/neg/i19266.check @@ -0,0 +1,12 @@ +-- [E178] Type Error: tests/neg/i19266.scala:13:4 ---------------------------------------------------------------------- +13 | fn2(2) // error + | ^^^^^^ + | missing argument list for value of type String => Some[String] + | + | longer explanation available when compiling with `-explain` +-- [E178] Type Error: tests/neg/i19266.scala:19:4 ---------------------------------------------------------------------- +19 | fn3(4) // error + | ^^^^^^ + | missing argument list for value of type Double => Some[String] + | + | longer explanation available when compiling with `-explain` diff --git a/tests/neg/i19266.scala b/tests/neg/i19266.scala new file mode 100644 index 000000000000..f2eb665f8242 --- /dev/null +++ b/tests/neg/i19266.scala @@ -0,0 +1,20 @@ +object i19266: + def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z") + def fn2(p: Int)(q: String) = Some(s"$p$q") + def fn3(x: Int, y: => String = "y")(z: Double) = Some(s"$x$y$z") + + def checkCompile = + // It compiles because default value for by-value + // parameter is impure and may perform side effect. + fn1(1) // warn + // This does not compile because (pure) synthesized lambda from + // eta-expansion in statement position is prohibited. + // See https://github.com/lampepfl/dotty/pull/11769 + fn2(2) // error + // This compiles. + val a = fn2(3) + // This does not compile because default value for by-name parameter + // is still pure. Thus, it also violates the rule for lambda in + // statement position. + fn3(4) // error + 1 \ No newline at end of file diff --git a/tests/pos/i19266.scala b/tests/pos/i19266.scala deleted file mode 100644 index b8a13c04f9d4..000000000000 --- a/tests/pos/i19266.scala +++ /dev/null @@ -1,9 +0,0 @@ -object i19266: - def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z") - def fn2(x: Int)(y: String) = Some(s"$x$y") - - def checkCompile = - fn1(1) - // This should compile with warning - fn2(2) - val _ = fn2(3) From 868463238a6143e2d9c116acfc0536e33f82e887 Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 27 Jan 2024 05:27:17 +0900 Subject: [PATCH 3/6] warn impure synthetic lambda with in stmt position synthetic lambda generated from a method with default arguments skipped through `closureDef` pattern. --- compiler/src/dotty/tools/dotc/ast/TreeInfo.scala | 2 ++ tests/neg/i19266.check | 1 + 2 files changed, 3 insertions(+) diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index 5ded0e1262e4..d8ac38ff1c66 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -792,6 +792,8 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => def unapply(tree: Tree)(using Context): Option[DefDef] = tree match { case Block((meth : DefDef) :: Nil, closure: Closure) if meth.symbol == closure.meth.symbol => Some(meth) + case Block((vd:ValDef) :: _, Block((meth:DefDef)::Nil,closure:Closure)) if meth.symbol == closure.meth.symbol => + Some(meth) case Block(Nil, expr) => unapply(expr) case _ => diff --git a/tests/neg/i19266.check b/tests/neg/i19266.check index 9c6fb8cdb3a4..5e8baa96e0d3 100644 --- a/tests/neg/i19266.check +++ b/tests/neg/i19266.check @@ -1,3 +1,4 @@ +unused value of type Double => Some[String] -- [E178] Type Error: tests/neg/i19266.scala:13:4 ---------------------------------------------------------------------- 13 | fn2(2) // error | ^^^^^^ From ea306da37fcf6ae4cb8ff653834e1c1c5c16b21e Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 27 Jan 2024 05:41:43 +0900 Subject: [PATCH 4/6] fix: tweak reporting --- compiler/src/dotty/tools/dotc/ast/TreeInfo.scala | 2 +- compiler/src/dotty/tools/dotc/typer/Typer.scala | 2 +- tests/neg/i19266.check | 5 ++++- tests/warn/i19266.check | 4 ++++ tests/warn/i19266.scala | 6 ++---- 5 files changed, 12 insertions(+), 7 deletions(-) create mode 100644 tests/warn/i19266.check diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index d8ac38ff1c66..616c7a7d029d 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -792,7 +792,7 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => def unapply(tree: Tree)(using Context): Option[DefDef] = tree match { case Block((meth : DefDef) :: Nil, closure: Closure) if meth.symbol == closure.meth.symbol => Some(meth) - case Block((vd:ValDef) :: _, Block((meth:DefDef)::Nil,closure:Closure)) if meth.symbol == closure.meth.symbol => + case Block((vd: ValDef) :: _, Block((meth: DefDef)::Nil, closure: Closure)) if meth.symbol == closure.meth.symbol => Some(meth) case Block(Nil, expr) => unapply(expr) diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index a30ecaf9ac55..895c8228a73c 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -4428,7 +4428,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer if isPure then missingArgs(tree, tree.tpe.widen) else - report.warning(UnusedNonUnitValue(tree.tpe)) + report.warning(UnusedNonUnitValue(tree.tpe),original.srcPos) case _ if isUnitExpr => if isPure then report.warning(PureUnitExpression(original, tree.tpe), original.srcPos) diff --git a/tests/neg/i19266.check b/tests/neg/i19266.check index 5e8baa96e0d3..83eda72e7808 100644 --- a/tests/neg/i19266.check +++ b/tests/neg/i19266.check @@ -1,4 +1,3 @@ -unused value of type Double => Some[String] -- [E178] Type Error: tests/neg/i19266.scala:13:4 ---------------------------------------------------------------------- 13 | fn2(2) // error | ^^^^^^ @@ -11,3 +10,7 @@ unused value of type Double => Some[String] | missing argument list for value of type Double => Some[String] | | longer explanation available when compiling with `-explain` +-- [E176] Potential Issue Warning: tests/neg/i19266.scala:9:7 ---------------------------------------------------------- +9 | fn1(1) // warn + | ^^^^^^ + | unused value of type Double => Some[String] diff --git a/tests/warn/i19266.check b/tests/warn/i19266.check new file mode 100644 index 000000000000..dcbb3ca9bedd --- /dev/null +++ b/tests/warn/i19266.check @@ -0,0 +1,4 @@ +-- [E176] Potential Issue Warning: tests/warn/i19266.scala:5:7 --------------------------------------------------------- +5 | fn1(1) // warn + | ^^^^^^ + | unused value of type Double => Some[String] diff --git a/tests/warn/i19266.scala b/tests/warn/i19266.scala index 1813981eda1b..f06fc8dabf5b 100644 --- a/tests/warn/i19266.scala +++ b/tests/warn/i19266.scala @@ -1,8 +1,6 @@ object i19266: def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z") - def fn2(x: Int)(y: String) = Some(s"$x$y") - def checkWarning = + def checkWarning: Int = fn1(1) // warn - fn2(2) // warn - val a = fn2(3) // warn + 1 From 7330791782fe4344386e9aa63dc33242fc04148c Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 27 Jan 2024 20:10:19 +0900 Subject: [PATCH 5/6] fix: false-positive closureDef match Before this commit, closureDef matching rule was not strict enough to filter out false-positive cases, which caused tests/init-global/neg/return2.scala to fail. It matched a plain block with ValDef at the begining and Closure at the end. As we want to match `closureDef` against a closure or a synthetic closure generated from eta-expansion, we should match against 1. a simple closure 2. a closure inside nested blocks without statement, which is generated from eta-expansion on method without default arguments 3. a closure inside nested blocks with statements consisting only of ValDef of default arguments, which is generated from eta-expansion on method with default arguments. This commit adds the condition for 3. --- compiler/src/dotty/tools/dotc/ast/TreeInfo.scala | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala index 616c7a7d029d..20fa94633b01 100644 --- a/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala +++ b/compiler/src/dotty/tools/dotc/ast/TreeInfo.scala @@ -789,12 +789,13 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] => /** An extractor for def of a closure contained the block of the closure. */ object closureDef { + private def isDefaultArg(tree:Tree): Boolean = tree match + case ValDef(_, _, Select(_,name)) if name.is(NameKinds.DefaultGetterName) => true + case _ => false def unapply(tree: Tree)(using Context): Option[DefDef] = tree match { case Block((meth : DefDef) :: Nil, closure: Closure) if meth.symbol == closure.meth.symbol => Some(meth) - case Block((vd: ValDef) :: _, Block((meth: DefDef)::Nil, closure: Closure)) if meth.symbol == closure.meth.symbol => - Some(meth) - case Block(Nil, expr) => + case Block(stmts, expr) if stmts.forall(isDefaultArg) => unapply(expr) case _ => None From 19e8865152dcfa66f4fca48e152567b73595c2e7 Mon Sep 17 00:00:00 2001 From: i10416 Date: Sat, 24 Feb 2024 16:31:28 +0900 Subject: [PATCH 6/6] tidy: apply code review Apply code review to simplify condition arms - https://github.com/lampepfl/dotty/pull/19540/files#r1501178130 --- compiler/src/dotty/tools/dotc/typer/Typer.scala | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index eb9d560d3103..af365f9bd147 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -4449,13 +4449,12 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer missingArgs(tree, tree.tpe.widen) else report.warning(UnusedNonUnitValue(tree.tpe),original.srcPos) - case _ if isUnitExpr => - if isPure then + case _ if isPure => + if isUnitExpr then report.warning(PureUnitExpression(original, tree.tpe), original.srcPos) - case _ => - if isPure then + else report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos) - + case _ => /** Types the body Scala 2 macro declaration `def f = macro ` */ protected def typedScala2MacroBody(call: untpd.Tree)(using Context): Tree = // TODO check that call is to a method with valid signature