[
https://issues.apache.org/jira/browse/GROOVY-12747?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140878#comment-18140878
]
Paul King commented on GROOVY-12747:
------------------------------------
The fix in [PR #3281|https://github.com/apache/groovy/pull/3281] takes the
second option from the description: an assignment of an incompatible value to
such a parameter is now a static type error, as it is for a Java lambda
parameter.
{code:groovy}
List<Integer> numbers = [1, 2]
numbers.stream().map { n -> n = n.toString(); n } // [Static type checking] -
Cannot assign value of type java.lang.String to variable of type
java.lang.Integer
{code}
h3. What changed
The parameter is still declared with the type its functional interface gives
it. Assignments to it are now checked against that type, not against its
original dynamic type. Nothing is silently converted any more:
||Under {{@TypeChecked}} / {{@CompileStatic}}||Before||After||
|{{n = n.toString()}}, lambda {{n = 'x'}}|{{[49, 50]}} / {{[120, 120]}}|compile
error|
|{{n = n + 0.5}}|{{[1, 2]}} (fraction dropped)|compile error|
|{{n = 'ab'}}|{{GroovyCastException}} at run time|compile error|
|{{n = n * 2}}, {{n += 1}}, {{n = null}}|works|unchanged|
|implicit {{it = 'x'}} (declared {{Object}})|works|unchanged|
|{{@ClosureParams}} parameters, e.g. DGM {{collect}}|keep the value|unchanged|
|dynamic Groovy|keeps the value|unchanged|
h3. Why not the first option
The first option was to keep the assigned value, as the {{@ClosureParams}} path
and dynamic Groovy do. I tried it by flow-typing the parameter instead of
declaring its type. It broke 51 tests, because the declared type is relied on
in several places:
* the synthetic lambda for a method reference resolves the method from its
parameter types;
* closures and lambdas are generated with the inferred parameter types, e.g.
{{doCall(I)}}, with type annotations attached to them;
* primitive functional interfaces such as {{IntUnaryOperator}} get unboxed
parameters;
* GROOVY-11083 checks a parameter's default value against its type;
* GROOVY-11085 computes the closure's own functional type from its parameter
types.
h3. Possible path forward
If the first option is wanted later, the least invasive way is a shadow local
for only those closures that reassign a parameter with an incompatible value:
* keep the parameter as it is, under a synthetic name;
* start the body with an {{Object}}-typed local of the original name,
initialized from the parameter;
* point every use in the body at the local, so that flow typing applies, as for
a {{def}} variable.
Every other closure would compile exactly as today, with no signature or
performance change. The check added here already identifies exactly the
closures that would need the rewrite. The work would cover:
* the AST rewrite before code generation;
* nested closures that capture the reassigned parameter;
* compound assignments;
* assignments in only some branches;
* default values;
* primitive parameters.
The alternative, generating every such parameter as {{Object}} and changing
everything in the list above to read the inferred types, is far more invasive.
It would also box primitives and change generated signatures.
Moving later from the current error to the first option would only turn compile
errors into working code, so it would be a compatible change.
> STC: assigning to a closure or lambda parameter typed from a functional
> interface converts the value to the inferred type
> -------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12747
> URL: https://issues.apache.org/jira/browse/GROOVY-12747
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> Under {{@TypeChecked}} and {{@CompileStatic}}, an untyped closure or lambda
> parameter whose type is inferred from a functional-interface parameter
> behaves as a declared, typed parameter. Assigning a value of another type to
> the parameter therefore converts the value to the parameter's type, silently
> or with a {{GroovyCastException}}. Dynamic Groovy keeps the value.
> {code:groovy}
> @groovy.transform.TypeChecked // or @CompileStatic
> def test() {
> List<Integer> numbers = [1, 2]
> numbers.stream().map { n -> n = n.toString(); n }.toList()
> }
> assert test() == [1, 2] // fails: [49, 50]
> {code}
> ||Closure (for {{Stream.map}} on {{List<Integer>}} [1,
> 2])||Dynamic||{{@TypeChecked}} / {{@CompileStatic}}||
> |{{\{ n -> n = n.toString(); n \}}}|[1, 2]|*[49, 50]*, the char codes of "1"
> and "2"|
> |{{\{ n -> n = n + 0.5; n \}}}|[1.5, 2.5]|*[1, 2]*, truncated|
> |{{n -> \{ n = 'x'; n \}}} (lambda)|[x, x]|*[120, 120]*|
> |{{Function<Integer, Object> f = \{ n -> n = 'x'; n \}}}|[x, x]|*[120, 120]*|
> |{{\{ n -> n = 'ab'; n \}}}|[ab, ab]|{{GroovyCastException}}: Cannot cast
> object 'ab' with class 'java.lang.String' to class 'java.lang.Integer'|
> |{{\{ it = 'x'; it \}}} (implicit parameter)|[x, x]|[x, x]|
> |DGM {{numbers.collect \{ n -> n = n.toString(); n \}}}
> ({{@ClosureParams}})|[1, 2]|[1, 2]|
> So the same closure body behaves differently depending on how its parameter
> type was inferred. A parameter typed through {{@ClosureParams}}, or an
> implicit {{it}}, keeps the assigned value, as dynamic Groovy does. A
> parameter typed from a functional interface converts it.
> h3. Cause
> {{StaticTypeCheckingVisitor.inferParameterAndReturnTypesOfClosureOnRHS}} sets
> the inferred type on an untyped parameter ({{parameter.setType(type)}},
> commented "GROOVY-11083, GROOVY-11085, et al."). The parameter is then
> declared with that type, so an assignment to it goes through a cast to the
> parameter type: a one-character String becomes its char code, a
> {{BigDecimal}} is narrowed to {{Integer}}, and other Strings fail. The
> {{@ClosureParams}} path ({{processClosureParams}}) records the inferred types
> without changing the parameters' declared type, so flow typing applies.
> h3. Expected
> The assignment must not silently change the value. Either:
> * treat the parameter as the {{@ClosureParams}} path does, so it keeps the
> assigned value as in dynamic Groovy; or
> * report a static type error for an assignment that is incompatible with the
> inferred type, as Java does for a lambda parameter.
> The first is consistent with the rest of Groovy's closure handling. Whichever
> is chosen, the {{@ClosureParams}} and functional-interface paths should agree.
> h3. Notes
> * Seen on master. {{setType}} dates from GROOVY-11083 and GROOVY-11085
> (3.0.18 / 4.0.13); earlier versions were not checked.
> * GROOVY-12746 types the parameters of closures passed to extension methods
> with functional-interface parameters, such as {{collectParallel}}, from the
> receiver. Those closures now have this behaviour too:
> {{numbers.collectParallel \{ n -> n = n.toString(); n \}}} gives [49, 50].
> Before GROOVY-12746 their parameters fell back to {{Object}}.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
