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
Comments
Imported From: https://issues.scala-lang.org/browse/SI-3237?orig=1
|
@odersky said: 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. |
@harrah said: 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)? |
@Sciss said: 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. |
@Sciss said: 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 ))... |
@etorreborre said: |
Eugene Yokota (eed3si9n) said (edited on Nov 1, 2012 10:59:25 PM UTC): 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:
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. |
@jrudolph said: val t: Converted[Int] = { evaluated = true; 1 } Here, the whole code block is typed with an expected type of 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. |
Eugene Yokota (eed3si9n) said:
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. |
@jrudolph said: I don't see where it does. |
Eugene Yokota (eed3si9n) said (edited on Nov 2, 2012 3:44:58 PM UTC):
I am interpreting the above as:
SLS 7.3 says
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. |
Ryan Hendrickson (ryan.hendrickson_bwater) said: 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 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. |
Ryan Hendrickson (ryan.hendrickson_bwater) said: 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. |
Ryan Hendrickson (ryan.hendrickson_bwater) said:
|
Julien Oster (anyfoo) said: erikvanoosten/metrics-scala#42 The library authors reacted quickly by changing the parameter type from being the by-name |
Erik van Oosten (erikvanoosten) said: If you know a trick to try, please add a comment to erikvanoosten/metrics-scala#42. |
Erik van Oosten (erikvanoosten) said (edited on May 10, 2014 2:28:27 PM UTC): |
Erik van Oosten (erikvanoosten) said: |
Added a lint just before the statute of limitations ran out. |
I got 275 such warnings on my source. Not sure why this is deemed a warning; but I can just mute it. |
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.
The text was updated successfully, but these errors were encountered: