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.

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

patch that adds warning for implicit methods with by-name parameters #3237

Closed
scabug opened this issue Mar 31, 2010 · 20 comments · Fixed by scala/scala#8590
Closed

patch that adds warning for implicit methods with by-name parameters #3237

scabug opened this issue Mar 31, 2010 · 20 comments · Fixed by scala/scala#8590
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Mar 31, 2010

Attached is a patch that adds an option -Ywarn-by-name. This option enables a warning when an implicit method has a call-by-name parameter.

It is a common mistake to assume an entire block will be passed to the implicit. It is explicitly mentioned in the specification that implicits with call-by-name parameters are allowed, otherwise I would have suggested disallowing this entirely.

I see that the fix for the similar ticket #2688 was implemented in the parser, but it was easier for me to do it in !RefChecks.

It is only lightly tested. If this patch is of interest, I'll add tests.

@scabug
Copy link
Author

scabug commented Mar 31, 2010

Imported From: https://issues.scala-lang.org/browse/SI-3237?orig=1
Reporter: @harrah
Attachments:

@scabug
Copy link
Author

scabug commented Apr 7, 2010

@odersky said:
There are very good use cases for implicits with by-name parameters. Look for instance at how we add ::# to Streams, or consider the Scala equivalent of Ruby's unless.

Generally, I don't believe we should have many cases of warnings, or options that control them. If people care about certain issues, there's a possibility to write a lint-like tool, maybe as a plugin.

@scabug
Copy link
Author

scabug commented Apr 7, 2010

@harrah said:
I did not realize it behaved as expected for a block when the conversion is triggered by a missing member. That is certainly useful and not problematic.

The problem I was trying to address is restricted to when the conversion is to an expected type. For example, this prints "side effect 1":

object ExpectedType {
  implicit def runnable(act: => Unit): Runnable =
    new Runnable { def run() = act }

  def main(args: Array[String]) {
      val runnable: Runnable =
      {
          println("side effect 1")
          println("side effect 2")
      }
      
      if(false)
          runnable.run()
  }
}

Since the two cases are handled separately in the specification, what about specifying that implicits with by-name parameters are not considered for this case (situation 1 in the spec)?

@scabug
Copy link
Author

scabug commented May 13, 2010

@Sciss said:
i am having the same problem here:

trait Lala

class Test[ T <% Lala ]( thunk: => T ) {
   def test : Int = {
       println( "HERE" )
       val t: Lala = thunk
       42
   }
}

implicit def thunkToTest[ T <% Lala ]( thunk: => T ) = new Test( thunk )

def rara( f: Test[ _ ]) : Int = f.test

rara { println( "!!!" ); new Lala{} }

obviously "!!!" is printed early and not the whole curly braces block treated as the thunk. i think this is counter-intuitive. someone suggested that a call rara({ println( "!!!" ); new Lala{} }) would work, but looks rather bad will confuse people in a DSL (where they will in all other cases make calls such as lulu { ... })

if you decide not to change the behaviour, a compiler warning would be very useful. i spent half a day figuring out what was going on and till i could distill the above example.

@scabug
Copy link
Author

scabug commented May 13, 2010

@Sciss said:
i should point out that this is not such a synthetically constructed case. here is an example usage from my DSL

      play( server, out ) {
         val ply = PlayBuf.ar( numChannels, id, BufRateScale.kr( id ), loop = if( loop ) 1 else 0 )
         if( !loop ) FreeSelfWhenDone.kr( ply )
         ply * "amp".kr( amp )
      }

now this failed to work, because PlayBuf was early constructed out of the Graph context:

object DSL {
   implicit def thunkToGraphFunction[ T <% GE ]( thunk: => T ) = new GraphFunction( thunk )
   def play( target: Node = Server.default.defaultGroup, outBus: Int = 0,
             fadeTime: Option[Float] = Some( 0.02f ),
             addAction: AddAction = addToHead )( f: GraphFunction[ _ ]) : Synth =
      f.play( target, outBus, fadeTime, addAction )
}

note that GE is a trait to which different graph elements can be converted, e.g. number literals, a Seq[ GE ], but also the ugens like PlayBuf.

but, if i do instead:

object DSL {
   ...
   def play( target: Node = Server.default.defaultGroup, outBus: Int = 0,
             fadeTime: Option[Float] = Some( 0.02f ),
             addAction: AddAction = addToHead )( thunk: => GE ) : Synth =
      new GraphFunction( thunk ).play( target, outBus, fadeTime, addAction )

it suddenly works, the thunk is not prematurely evaluated. this is clearly inconsistent, and for the sake of DSL building, i think the implicit def thunkToGraphFunction should work exactly the same. Here is another example which does work:

   {
      val ply = PlayBuf.ar( numChannels, id, BufRateScale.kr( id ), loop = if( loop ) 1 else 0 )
      if( !loop ) FreeSelfWhenDone.kr( ply )
      ply * "amp".kr( amp )
   }.play( s, 0 )

so this last example is implicitly converted to GraphFunction (which has the play method) and the thunk captures everything inside the curly braces, not just the last line (ply * "amp".kr( amp ))...

@scabug
Copy link
Author

scabug commented Oct 30, 2012

@etorreborre said:
I hit this issue today and it looks like an inconsistency to me. Is there a reason why this shouldn't be fixed?

@scabug
Copy link
Author

scabug commented Nov 1, 2012

Eugene Yokota (eed3si9n) said (edited on Nov 1, 2012 10:59:25 PM UTC):
I saw Eric's post to scala-lang:

case class Converted[+T](t: () => T)
implicit def convert[T](t: =>T) = Converted(() => t)
  
var evaluated = false
val t: Converted[Int] = { evaluated = true; 1 }

// prints true
evaluated

The above behavior looks pathological given the following explicit application does not trigger the side effect:

convert { evaluated = true; 1 }

Here are the relevant sections in the Scala Language Specification:

h3. 7.3 Views

Implicit parameters and methods can also define implicit conversions called views. A view from type _S_ to type _T_ is defined by an implicit value which has function type _S=>T_ or _(=>S)=>T_ or by a method convertible to a value of that type.

Views are applied in three situations.

1. If an expression _e_ is of type _T_, and _T_ does not conform to the expression’s expected type _pt_. In this case an implicit _v_ is searched which is applicable to e and whose result type conforms to _pt_. The search proceeds as in the case of implicit parameters, where the implicit scope is the one of _T => pt_. If such a view is found, the expression _e_ is converted to _v(e)_.

....

The implicit view, if it is found, can accept is argument e as a call-by-value or as a call-by-name parameter. However, call-by-value implicits take precedence over call-by-name implicits.

h4. 4.6.1 By-NameParameters

Syntax:
{code}    ParamType ::= '=>' Type{code}

The type of a value parameter may be prefixed by _=>_, e.g. _x: => T_. The type of such a parameter is then the parameterless method type _=> T_. This indicates that the corresponding argument is not evaluated at the point of function application, but instead is evaluated at each use within the function. That is, the argument is evaluated using _call-by-name_.

Because the block evaluating to 1 does not conform to Converted[Int], it's the first condition of view that's being used, not the missing method or incompatible arg case. According to the spec, this converts the expression e to v(e), which in this case is:

convert { evaluated = true; 1 }

Unless t.t is invoked, I don't see why this block should be evaluated.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

@jrudolph said:

val t: Converted[Int] = { evaluated = true; 1 }

Here, the whole code block is typed with an expected type of Converted[Int]. Then, inside the block, when looking at the return value, the compiler sees 1 not complying to the expected type and then introduces the conversion inside the block. Afterwards, the whole block does comply to the expected type and there is no reason to apply a view to the whole block. I think that's the reason for the current behavior and it seems to be covered by the spec.

That's just the simplest example but you may introduce many more possible levels of nesting where the expected type flows to the very inside and the implicit conversion actually should be applied inside.

See http://www.scala-lang.org/node/8487 for an older discussion as well.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

Eugene Yokota (eed3si9n) said:
In http://www.scala-lang.org/node/8487 Viktor wrote:

So, the reason is that the implicit conversion is applied to the result of the block.

And I guess Johannes is also saying that this is what happened:

val t: Converted[Int] = { evaluated = true; convert(1) }

According to SLS 6.11, a block {s1; ...; sn; x } is an expression, which happens to have the type T forSome {Q} where T is the type of x. In terms of the type, wrapping x may satisfy the typechecking, but doesn't the View spec say that e is converted to v(e)? Even if the type is compatible, I don't think compiler can switch around sequential statement orders.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

@jrudolph said:
bq. I don't think compiler can switch around sequential statement orders.

I don't see where it does.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

Eugene Yokota (eed3si9n) said (edited on Nov 2, 2012 3:44:58 PM UTC):
@johannes I am trying to piece things together using only the spec. According to SLS 6.11:

Evaluation of the block entails evaluation of its statement sequence, followed by an evaluation of the final expression _e_, which defines the result of the block.

I am interpreting the above as:

  1. {s1; ...; sn; x } needs to happen in sequential order
  2. the whole block is one expression that's either evaluated or not evaluated.

SLS 7.3 says

If such a view is found, the expression e is converted to v(e).

Here, I am reading that e is the entire {s1; ...; sn; x } since there are no special clause about expanding blocks. So it's v({s1; ...; sn; x }), and when the parameter is passed by name the entire block is still not evaluated. {s1; ...; sn; v(x) } is a close approximation, but it works only when the parameter is strictly evaluated.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

Ryan Hendrickson (ryan.hendrickson_bwater) said:
@eugene Both your points 1 and 2 are right, but the trouble is that the compiler is solving the type-check from the outside in. So it sees the block needs to have type Converted[Int], so it knows that the target type of 1 needs to be Converted[Int]. It doesn't backtrack out to the outer block when it sees that 1 does not have that type; it just applies the conversion locally.

I don't think this is a violation of the spec; the spec just says that the type of a block shall be the type of its last expression, and that view conversions work by converting e to v(e); it seems silent on the order in which these rules should be applied when solving the type-check (unless I'm missing something).

I wonder, though, if someone were to propose clarifying the spec on this point to explicitly pull back conversions to the largest scope to which they could apply, (A) how difficult that would be to implement (I imagine non-trivial, but I don't know much about it) and (B) whether that would have other undesirable consequences.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

Ryan Hendrickson (ryan.hendrickson_bwater) said:
Oh, no, it's not ambiguous at all. ‘The expected type of the final expression e is the expected type of the block.’ So that pushes the conversion into the block.

I suppose my wonderings still stand, but now it's a question of (probably) complicating the spec and (very probably) the implementation for gains in this particular edge case and uncertain other effects.

@scabug
Copy link
Author

scabug commented Nov 2, 2012

Ryan Hendrickson (ryan.hendrickson_bwater) said:
For a totally analogous situation that has nothing to do with either by-name or blocks:

scala> def noisyIdentity[A](a: A): A = { println("LOUD NOISES"); a }
noisyIdentity: [A](a: A)A

scala> implicit def intToStr(i: Int): String = { println("intToStr here"); i.toString }
warning: there were 1 feature warnings; re-run with -feature for details
intToStr: (i: Int)String

scala> noisyIdentity(noisyIdentity(5)): String
intToStr here
LOUD NOISES
LOUD NOISES
res0: String = 5

intToStr runs first for the same reason: in type-checking, expected types of inner expressions are determined by the expected types of outer expressions, and implicit conversions are applied as a fallback when expected types can't be satisfied.

@scabug
Copy link
Author

scabug commented May 8, 2014

Julien Oster (anyfoo) said:
FWIW, here's an instance in the real world where the behavior did cause some confusion:

erikvanoosten/metrics-scala#42

The library authors reacted quickly by changing the parameter type from being the by-name => Boolean to being an unambiguous function type () => Boolean, but I wonder what other code uses it. I would personally appreciate a warning in those cases, especially because blocks passed to by-name parameters do exhibit the intuitive behavior when there's no implicit conversion.

@scabug
Copy link
Author

scabug commented May 10, 2014

Erik van Oosten (erikvanoosten) said:
As main author of metrics-scala I decided to hold back on using () -> for now. I first would like to consider other solutions such as method overloading. Perhaps there are other tricks to make the compiler behave more as expected, or perhaps this issue will be resolved soonish.

If you know a trick to try, please add a comment to erikvanoosten/metrics-scala#42.

@scabug
Copy link
Author

scabug commented May 10, 2014

Erik van Oosten (erikvanoosten) said (edited on May 10, 2014 2:28:27 PM UTC):
I now read http://www.scala-lang.org/old/node/8487. If this issue won't be solved, then please close it with a 'not a bug'.

@scabug
Copy link
Author

scabug commented May 11, 2014

Erik van Oosten (erikvanoosten) said:
For metrics-scala we found a solution by declaring our magnet parameter by-name as well.

@som-snytt
Copy link

Added a lint just before the statute of limitations ran out.

@SethTisue SethTisue added this to the 2.13.2 milestone Dec 11, 2019
@jeffrey-aguilera
Copy link

I got 275 such warnings on my source. Not sure why this is deemed a warning; but I can just mute it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants