Skip to content

fix(#19266): better warning for impure synthetic lambda in stmt position #19540

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(#19266): better warning for impure synthetic lambda in stmt position #19540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c942f1f
draft:fix(#19266): add test cases
i10416 Jan 26, 2024
a953135
draft: better error for impure lambda in stmt position
i10416 Jan 26, 2024
8684632
warn impure synthetic lambda with in stmt position
i10416 Jan 26, 2024
ea306da
fix: tweak reporting
i10416 Jan 26, 2024
7330791
fix: false-positive closureDef match
i10416 Jan 27, 2024
4b3e82b
Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…
i10416 Feb 9, 2024
62c569a
Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…
i10416 Feb 24, 2024
19e8865
tidy: apply code review
i10416 Feb 24, 2024
f969d1a
Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…
i10416 Mar 10, 2024
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
5 changes: 4 additions & 1 deletion compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,10 +790,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
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should mix default arguments with this extractor. I prefer to leave it as it was before.

Copy link
Contributor Author

@i10416 i10416 Feb 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@odersky

just because it is more convenient for some lint. ...
#19540 (review)

I also cared about it and when I added isDefaultArg, I thought Block(Nil, expr) here is the special case for closure where default argument stmts is empty, therefore this change would be safe.

Do you think the // closure 1 in the following example is not closure?

Block(Nil, expr)

{
  {
      // closure 0
  }
}

Block(stmts, expr)

{
  // These are synthesized ValDefs from default arguments invisible to users.
  val meth$default0 = ???
  val meth$default1 = ???
  // ...
  {
     // closure 1
  } 

}

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand, but I don't think we should generalize closureDef that way.

I also cared about it and when I added isDefaultArg, I thought Block(Nil, expr) here is the special case for closure where default argument stmts is empty, therefore this change would be safe.

That's one of the scenarios. But the compiler is also free to wrap closureDef in a block in other situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will change it back. Thank you for feedback.

What do you think about prohibiting impure lambda in statement position?

#19540 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

We could try that and see what breaks. The comments on #11769 indicate that this won't be easy, though.

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(Nil, expr) =>
case Block(stmts, expr) if stmts.forall(isDefaultArg) =>
unapply(expr)
case _ =>
None
Expand Down
16 changes: 10 additions & 6 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4436,10 +4436,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)
Expand All @@ -4452,12 +4452,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)
case _ if isUnitExpr =>
report.warning(PureUnitExpression(original, tree.tpe), original.srcPos)
if isPure then
missingArgs(tree, tree.tpe.widen)
else
report.warning(UnusedNonUnitValue(tree.tpe),original.srcPos)
Copy link
Contributor

@odersky odersky Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A UnusedNonUnitValue warning would have to be issued by the linter. It's a conscious design decision not to mix typing and linting one one phase. Typing is mandatory, linting is optional. Typing is precise according to spec, whereas linting is ad-hoc and not specified by the language.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this is a challenge in scala 2, which is more constrained. Typer has information you want to exploit which may not be available later, but you don't want to bloat or break typer; scala 2 resorts to attachments. In this case, I don't care about purity, -Wnonunit-statement should always warn. Maybe it won't issue a duplicate warning if the positions are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a challenge in scala 2, ... Typer has information you want to exploit which may not be available later

I wonder we can take advantage of Tasty information for linting. I guess the difficulty would be some linting might depend on compiler state that would not be available from Tasty.

case _ if isPure =>
if isUnitExpr then
report.warning(PureUnitExpression(original, tree.tpe), original.srcPos)
else
report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos)
case _ =>
report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos)

/** Types the body Scala 2 macro declaration `def f = macro <body>` */
protected def typedScala2MacroBody(call: untpd.Tree)(using Context): Tree =
// TODO check that call is to a method with valid signature
Expand Down
16 changes: 16 additions & 0 deletions tests/neg/i19266.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-- [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`
-- [E176] Potential Issue Warning: tests/neg/i19266.scala:9:7 ----------------------------------------------------------
9 | fn1(1) // warn
| ^^^^^^
| unused value of type Double => Some[String]
20 changes: 20 additions & 0 deletions tests/neg/i19266.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
4 changes: 4 additions & 0 deletions tests/warn/i19266.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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]
6 changes: 6 additions & 0 deletions tests/warn/i19266.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
object i19266:
def fn1(x: Int, y: String = "y")(z: Double) = Some(s"$x$y$z")

def checkWarning: Int =
fn1(1) // warn
1