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

i10416 · 2024-01-26T15:08:52Z

close #19266

Before this PR, checkStatementPurity filtered out impure expression too eagerly and closureDef ignores closure definition generated from eta-expansion on method with default arguments, which prevented impure lambda at statement position from showing a warning.

This PR modifies

checkStatementPurity to check expression purity at later so that impure lambda will not slip through the check
closureDef to match against a synthetic closure from eta-expansion on method with default arguments

Related to

i10416 · 2024-01-26T15:18:45Z

tests/pos/i19266.scala

+  def fn2(x: Int)(y: String) = Some(s"$x$y")
+
+  def checkCompile =
+    fn1(1)


Scala 2.13.12 with -Xsource:3 compiles with warnings; " a pure expression does nothing in statement position".

Scala 2.13.12 without -Xsource:3 raises errors for fn1(1), fn2(2) and fn2(3).

Unapplied methods are only converted to functions when a function type is expected.
You can make this conversion explicit by writing fn2 _ or fn2(_)(_) instead of fn2.

Do the following to unapplied methods used as value

What characteristic allows us to assume unapplied methods are actually used as value?

https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/compiler/src/dotty/tools/dotc/typer/Typer.scala#L3645

* (4) Do the following to unapplied methods used as values: * (4.1) If the method has only implicit parameters pass implicit arguments * (4.2) otherwise, if `pt` is a function type and method is not a constructor, * convert to function by eta-expansion, * (4.3) otherwise, if the method is nullary with a result type compatible to `pt` * and it is not a constructor, apply it to () * otherwise issue an error

https://github.com/lampepfl/dotty/blob/main/docs/_spec/06-expressions.md

Method Conversions

The following four implicit conversions can be applied to methods which are not applied to some argument list.

Evaluation

A parameterless method ´m´ of type => ´T´ is always converted to type ´T´ by evaluating the expression to which ´m´ is bound.

Implicit Application

If the method takes only implicit parameters, implicit arguments are passed following the rules here.

Eta Expansion

Otherwise, if the method is not a constructor, and the expected type ´\mathit{pt}´ is a function type, or, for methods of non-zero arity, a type sam-convertible to a function type, ´(\mathit{Ts}') \Rightarrow T'´, eta-expansion is performed on the expression ´e´.

(The exception for zero-arity methods is to avoid surprises due to unexpected sam conversion.)

Empty Application

Otherwise, if ´e´ has method type ´()T´, it is implicitly applied to the empty argument list, yielding ´e()´.

In the following example m can be partially applied to the first two parameters. Assigning m to f1 will automatically eta-expand

def m(x: Boolean, y: String)(z: Int): List[Int] val f1 = m val f2 = m(true, "abc")

(The emphasis is added by me)
What should happen if m is not assigned to value?

https://dotty.epfl.ch/docs/reference/changed-features/eta-expansion-spec.html#automatic-eta-expansion-and-partial-application-1

Note for myself.
https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/compiler/src/dotty/tools/dotc/typer/Typer.scala#L4411

i10416 · 2024-01-26T17:04:46Z

tests/pos/i19266.scala

@@ -0,0 +1,9 @@
+object i19266:


note:

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 checkCompile = // This is NOT regarded as `isPureExpr`, but I think should be to warn `nonunit-statement`. // See https://github.com/lampepfl/dotty/issues/19266#issuecomment-1856248380 fn1(1) // This is regarded as `isPureExpr` and therefore it results in error. fn2(2)

Block( List( ValDef( y$1, TypeTree[AnnotatedType(TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class scala)),object Predef),type String),ConcreteAnnotation(Apply(Select(New(TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class unchecked)),class uncheckedVariance)]),<init>),List())))], Select(This(Ident(i19266$)),fn1$default$2) ) ), Block( List( DefDef( $anonfun, List(List(ValDef(z,TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),class Double)],EmptyTree))), TypeTree[AppliedType(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Some),List(TypeRef(ThisType(TypeRef(NoPrefix,module class lang)),class String)))], Apply(Apply(Ident(fn1),List(Literal(Constant(1)), Ident(y$1))),List(Ident(z))) ) ), Closure(List(),Ident($anonfun),EmptyTree) ) )

Block( List( DefDef( $anonfun, List(List(ValDef(q,TypeTree[TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class scala)),object Predef),type String)],EmptyTree))), TypeTree[AppliedType(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Some),List(TypeRef(ThisType(TypeRef(NoPrefix,module class lang)),class String)))], Apply(Apply(Ident(fn2),List(Literal(Constant(2)))),List(Ident(q))) ) ), Closure(List(),Ident($anonfun),EmptyTree) )

f1(1) is regarded as Block with ValDef statement and Block expr whereas f2(2) is regarded as Block with DefDef statement and Closure.

i10416 · 2024-01-26T18:59:34Z

Note for myself.

Default by-value arguments may have side-effect, so it is reasonable not to warn " A pure expression does nothing in statement position". Perhaps, we should warn NonUnitUnusedValue.

Method without default argument and method with default arguments for by-name parameters are pure ~~and (I think) should show warnings instead of error~~

It seems an expected behavior for compiler to raise error rather than warning.

#11769

import scala.collection.mutable.ArrayBuffer
import scala.util.Random

val arr = ArrayBuffer[Int]()

def fn0(x: Int)(y: String) = Some(s"$x$y")
def fn1(x: Int, y: Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")
def fn2(x: Int, y: => Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")

// This is pure and falls into the missing argument arm
// here https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
fn0(0)
// This is impure(perform side-effect to mutate `arr`). Therefore this passes through `checkStatementPurity`.
fn1(1)
// This is pure and falls into the missing argument arm 
// https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
fn2(2)

i10416 · 2024-01-26T19:06:57Z

It seems expected behavior to raise error rather than warning 🤔

Disallow lambdas in statement position or if the expected type is Unit. This was a loophole that opened up due to the changes to eta expansion in Scala 3. We did get a warning in these cases, but an error might be better.

#11769

tests/neg/i19266.check

synthetic lambda generated from a method with default arguments skipped through `closureDef` pattern.

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.

…rams-list

dwijnand · 2024-02-23T11:16:13Z

@i10416 before I merge this, what's the status from your perspective? Are your commits here ready to be merged?

I think your arguments are very reasonable and your changes are right. But I wonder if it would be better to be bolder to make things simpler. I'm thinking we make closures not compile, whether they're pure or not. Using your example:

import scala.collection.mutable.ArrayBuffer
import scala.util.Random

val arr = ArrayBuffer[Int]()

def fn0(x: Int)(y: String) = Some(s"$x,$y")
def fn1(x: Int, y: Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")
def fn2(x: Int, y: => Unit = arr.addOne(Random.nextInt()))(z:Double) = Some(s"$x,$y,$z")

// Thus, it shouldn't compile.
fn0(0)
// This is impure(perform side-effect to mutate `arr`). Therefore this passes through `checkStatementPurity`.
fn1(1)
// This is pure and falls into the missing argument arm 
// https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/tests/pos-with-compiler-cc/dotc/typer/Typer.scala#L4203
// Thus, it shouldn't compile.
fn2(2)

I think we should just also make fn1(1) not compile, even if it has a side-effecting default by-value argument. Any project that crucially relies on that effect can do one of many workarounds to preserving their program's behaviour. What do you think?

i10416 · 2024-02-23T13:31:41Z

@dwijnand

Are your commits here ready to be merged?

Yes, it is ready for review.

I think we should just also make fn1(1) not compile, e.....

I agree with you. It feels consistent to prohibit lambda in statement position regardless of its purity.
I guess few projects if any depend on this behavior and preserving similar behavior is not so hard.

I will add some changes and let's see whether any projects in the community build fail to compile.

dwijnand · 2024-02-23T18:10:37Z

@odersky do you agree to prohibiting more lambdas, extending #11769?

odersky

I think if we want an unused warning (which for me is debatable, but I am I know little about unused policy and am generally considered a skeptic), we need to implement this in the linting phase, not typer. That phase could use a new extractor similar to clsoureDef, but it should not change the original closureDef extractor.

In particular, we should not change fundamental abstractions like a closureDef extractor just because it is more convenient for some lint. The risk this breaks something in type inference or code generation is far too big.

odersky · 2024-02-23T19:08:26Z

compiler/src/dotty/tools/dotc/typer/Typer.scala

+        if isPure then
+          missingArgs(tree, tree.tpe.widen)
+        else
+          report.warning(UnusedNonUnitValue(tree.tpe),original.srcPos)


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.

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.

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.

odersky · 2024-02-23T19:10:36Z

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

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


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

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

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.

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

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

#19540 (comment)

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

dwijnand · 2024-02-23T23:10:37Z

@odersky do you agree to prohibiting more lambdas, extending #11769?

@odersky asides from the extractor changes, which can be changed back, what do you think about making some more lambdas type errors, like #11769 did?

…rams-list

Apply code review to simplify condition arms - https://github.com/lampepfl/dotty/pull/19540/files#r1501178130

dwijnand · 2024-03-03T12:11:11Z

Needs a rebase, for the new onnx submodule version.

…rams-list

draft:fix(scala#19266): add test cases

c942f1f

i10416 commented Jan 26, 2024

View reviewed changes

i10416 changed the title ~~draft:fix(#19266): add test cases~~ draft:fix(#19266): inconsistent compile error for eta-expanded methods Jan 26, 2024

i10416 commented Jan 26, 2024

View reviewed changes

i10416 closed this Jan 26, 2024

i10416 reopened this Jan 26, 2024

i10416 changed the title ~~draft:fix(#19266): inconsistent compile error for eta-expanded methods~~ draft:fix(#19266): better error for impure lambda in stmt position Jan 26, 2024

i10416 commented Jan 26, 2024

View reviewed changes

tests/neg/i19266.check Outdated Show resolved Hide resolved

draft: better error for impure lambda in stmt position

a953135

i10416 force-pushed the fix/19266-inconsistency-in--method-with-params-list branch from 2645e36 to a953135 Compare January 26, 2024 20:17

i10416 added 2 commits January 27, 2024 05:27

warn impure synthetic lambda with in stmt position

8684632

synthetic lambda generated from a method with default arguments skipped through `closureDef` pattern.

fix: tweak reporting

ea306da

i10416 changed the title ~~draft:fix(#19266): better error for impure lambda in stmt position~~ draft:fix(#19266): better warning for impure lambda in stmt position Jan 26, 2024

i10416 changed the title ~~draft:fix(#19266): better warning for impure lambda in stmt position~~ fix(#19266): better warning for impure lambda in stmt position Jan 26, 2024

i10416 changed the title ~~fix(#19266): better warning for impure lambda in stmt position~~ fix(#19266): better warning for impure synthetic lambda in stmt position Jan 26, 2024

i10416 mentioned this pull request Jan 26, 2024

https://github.com/lampepfl/dotty/blob/1716bcd9dbefbef88def848c09768a698b6b9ed9/compiler/src/dotty/tools/dotc/typer/Typer.scala#L3645 #19543

Closed

i10416 marked this pull request as ready for review January 27, 2024 12:36

nicolasstucki requested a review from dwijnand February 1, 2024 08:27

nicolasstucki assigned dwijnand Feb 1, 2024

Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…

4b3e82b

…rams-list

odersky requested changes Feb 23, 2024

View reviewed changes

i10416 added 2 commits February 24, 2024 16:28

Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…

62c569a

…rams-list

tidy: apply code review

19e8865

Apply code review to simplify condition arms - https://github.com/lampepfl/dotty/pull/19540/files#r1501178130

Merge branch 'master' into fix/19266-inconsistency-in--method-with-pa…

f969d1a

…rams-list

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

i10416 commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

i10416 Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i10416 Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Method Conversions

Evaluation

Implicit Application

Eta Expansion

Empty Application

Uh oh!

i10416 Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i10416 Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i10416 commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

i10416 commented Jan 26, 2024

Uh oh!

Uh oh!

dwijnand commented Feb 23, 2024

Uh oh!

i10416 commented Feb 23, 2024

Uh oh!

dwijnand commented Feb 23, 2024

Uh oh!

odersky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

i10416 Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwijnand commented Feb 23, 2024

Uh oh!

dwijnand commented Mar 3, 2024

Uh oh!

Uh oh!

i10416 commented Jan 26, 2024 •

edited

Loading

i10416 Jan 26, 2024 •

edited

Loading

i10416 Jan 26, 2024 •

edited

Loading

i10416 Jan 26, 2024 •

edited

Loading

i10416 Jan 26, 2024 •

edited

Loading

i10416 commented Jan 26, 2024 •

edited

Loading

odersky left a comment •

edited

Loading

odersky Feb 23, 2024 •

edited

Loading

i10416 Feb 24, 2024 •

edited

Loading