[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17583845#comment-17583845
]
ASF GitHub Bot commented on GROOVY-8788:
eric-milles merged PR #1768:
URL: https://github.com/apache/groovy/pull/1768
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17583097#comment-17583097
]
Eric Milles commented on GROOVY-8788:
-
The pull request has been updated with mitigation for type checking of map
value read and write. The one item that suffered is key type checking, as
noted above.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17583059#comment-17583059
]
Eric Milles commented on GROOVY-8788:
-
Since basically everything allows get/put property using string for property
name via {{getAt(Object,String)}} and {{putAt(Object,String,Object)}} maybe an
optional type checking extension should be created, much like was done for
regular expressions.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17583055#comment-17583055
]
Eric Milles commented on GROOVY-8788:
-
It is this excerpt of
[StaticTypeCheckingVisitor#getResultType|https://github.com/apache/groovy/blob/7f6fcb2f81b58a54bc7beef33ac382cb50a79cb4/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java#L4522]
that determines the type of "x = map['key']" and "map['key'] = x". Yes, it
only looks for getAt...
{code:java}
if (isArrayOp(op)) {
Expression copy = binX(leftExpression, expr.getOperation(),
rightExpression);
copy.setSourcePosition(expr); // do not propagate
BINARY_EXP_TARGET, etc.
MethodNode method = findMethodOrFail(copy, left, "getAt",
rightRedirect);
if (method != null && !isNumberCategory(getWrapper(rightRedirect)))
{
return inferReturnTypeGenerics(left, method, rightExpression);
}
return inferComponentType(left, right);
}
{code}
When method selection changes, it determines Object not Type for
Map. I can add a special case for Map left expression and String
right expression.
For put, type checking is done via this block in
[StaticTypeCheckingVisitor#visitBinaryExpression|https://github.com/apache/groovy/blob/7f6fcb2f81b58a54bc7beef33ac382cb50a79cb4/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java#L820]:
{code:java}
if (isArrayOp(op)) {
...
if (!lType.isArray()
&& enclosingBinaryExpression != null
&& enclosingBinaryExpression.getLeftExpression() ==
expression
&&
isAssignment(enclosingBinaryExpression.getOperation().getType())) {
// left hand side of a subscript assignment: map['foo'] =
...
Expression enclosingExpressionRHS =
enclosingBinaryExpression.getRightExpression();
if (!(enclosingExpressionRHS instanceof ClosureExpression))
{
enclosingExpressionRHS.visit(this);
}
ClassNode[] arguments = {rType,
getType(enclosingExpressionRHS)};
List methods = findMethod(lType, "putAt",
arguments);
if (methods.size() == 1) {
typeCheckMethodsWithGenericsOrFail(lType, arguments,
methods.get(0), enclosingExpressionRHS);
} else if (methods.isEmpty()) {
addNoMatchingMethodError(lType, "putAt", arguments,
enclosingBinaryExpression);
}
}
}
{code}
It relies on method selection to produce "Cannot call #putAt(Map,K,V) with
arguments ..." errors. Since {{putAt(Object,String,Object)}} is now selected
for string keys, this block checks nothing. However, the result type allows
"Cannot assign value of type X to variable of type Y" to be produced.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17583001#comment-17583001
]
Eric Milles commented on GROOVY-8788:
-
So I can think of a few options to move forward from this point:
1. Add {{getAt(Map,String}} and {{putAt(Map,String,V}} extension methods. This
gets in front of the getProperty/setProperty behavior. But it means
"map['key']" diverges from "map.key". I tried this option in the [previous
PR|https://github.com/apache/groovy/pull/1766]
2. Add special-case logic in STC to select {{getAt(Map,Object)}} for string
argument and map receiver. (same for putAt). This lets type-checker work like
before. But the public property case described above will still be a cast
exception for SC at least.
3. Add type inference logic to STC for left-square-bracket binary expression
with map receiver and String key. This overrides the type information provided
by method selection.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582993#comment-17582993
]
Eric Milles commented on GROOVY-8788:
-
First of all, thank you for taking the time to read through all this and
provide feedback.
Now we come to the hard decisions. {{getAt(Object,String)}} is chosen over
{{getAt(Map,Object)}} for dynamic runtime and now STC for String input. As you
can see from all the test failures, this goes against expectations because the
inferred return type is Object not the map's value type. There is an edge case
where this bears out now:
{code:groovy}
class A { }
class B { }
class HM extends HashMap {
B b = new B()
}
map = new HM()
map['a'] = new A()
A value = map['a']
value = map['b'] // Groovy 5 returns the property declared by HM
(GROOVY-5001, GROOVY-5491, GROOVY-6144)
{code}
https://github.com/apache/groovy/blame/c18411fe233bd60d4d045d65d9bb04b459f95e12/src/main/java/groovy/lang/MetaClassImpl.java#L1966
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582810#comment-17582810
]
Jochen Theodorou commented on GROOVY-8788:
--
[~emilles] I think your analysis so far is very spot on. Sorry for writing so
much text all the time, but I think it is important that I explain you a bit my
thinking, not only the result.
{code:Java}
void testPutWithWrongValueType() {
shouldFailWithMessages '''
def map = new HashMap()
map.put('hello', new Object())
''',
'Cannot find matching method java.util.HashMap#put(java.lang.String,
java.lang.Object). Please check if the declared type is correct and if the
method exists.'
}
{code}
This test passes now, but should have failed. It is not about getAt though.
Map#put(K,V) is then not the selected method here - which makes sense, since it
is not fitting. But there is no DGM for this either. So what method is actually
selected here? There might be more like this.
Of course the majority is, as you mentioned because of
Object#getAt(String):Object being selected over Map#getAt(Object):V
To solve this I see a couple of possibilities which I all don't like all that
much. The problem is that the tests IMHO show the desired behavior. Changing
them would not only mean breaking existing code most likely, but also - to
avoid that - further work. So I am not really for this, unless we make that a
blocking issue for the next release. So my take on this is, fix the tests if
you want this PR going through, but document all the failing tests with a new
blocking issue and there is the danger of having to revert more than just the
test changes. What I would prefer is adding commits to this PR to try find a
solution. but I think some more discussion is required. So maybe may thoughts
on possible solutions:
(1) replace Map#getAt(Object):V with Map#getAt(K):V. This raises of
course the question of if this even shadows (and even should do so) the Object
method. A Map could be super troublesome here. And of course this
signature is not aligned with the normal get-method Map has. I personally think
that having getAt more specific would be no problem, you still have get for the
strange things where you use a key from a different class with same hashcode
and accordingly behaving equals method - which should be super rare. And I do
not see a requirement to honor the old contract of Map here. But as I said..
should this really be preferred over the Object variant in case of Map. Or
maybe it would be worth it, even if this fails in method selection? I think it
would. It is kind of my preferred solution without knowing all the implications
of course
(2) change Object#getAt(String) to Object#getAt(Object). This change would
require that we try to convert the argument to String in the getAt method. As
in (1) Map would then be no problem. And as in (1) Map could be
a problem. Actually again we have to ask what method is preferred by the static
compiler and which should be. And again I do not care about a method selection
error during compile time here. But this would be a breaking change whenever
somebody made at getAt(String) method with @Override - unless our
implementation of this does not check for DGM. though the question here is then
if that is correct.
(3) add Map#getAt(String). This would mean to have it additionally to
Map#getAt(Object). Here we would have to clear if there is a case of
Map#get(String) being called instead of Object#getAt(String) as part of the MOP
is possible. It is not get, but getAt, so there might be no conflict. But there
are direct calls to getAt as part of the meta class to map property/attribute
access I think. And is it then right to call Object#getAt(String) instead of
Map#getAt(String)?
For me all 3 of them have potential of breaking code.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
>
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582677#comment-17582677
]
ASF GitHub Bot commented on GROOVY-8788:
eric-milles closed pull request #1766: GROOVY-8788: STC: prefer closer
parameter match over self-type match
URL: https://github.com/apache/groovy/pull/1766
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582672#comment-17582672
]
ASF GitHub Bot commented on GROOVY-8788:
eric-milles opened a new pull request, #1768:
URL: https://github.com/apache/groovy/pull/1768
Given choice between methods m(String,Object) and m(Object,String), prefer
closer parameter matching. This aligns with the method selection of the dynamic
runtime. There is much more discussion of the various test cases in the ticket.
With proper selection of `getAt(Object,String)` over `getAt(Map,Object)` and
`putAt(Object,String,Object)` over `putAt(Map,K,V)` there are quite a few STC
test issues. Not sure if I should separate them out to a test script just for
8788 or mitigate them with some minor changes or something else. I'd be
interested to hear how this sits now. @blackdrag @paulk-asert
https://issues.apache.org/jira/browse/GROOVY-8788
https://issues.apache.org/jira/browse/GROOVY-6504
https://issues.apache.org/jira/browse/GROOVY-6849
https://issues.apache.org/jira/browse/GROOVY-6970
https://issues.apache.org/jira/browse/GROOVY-8787
https://issues.apache.org/jira/browse/GROOVY-9069
https://issues.apache.org/jira/browse/GROOVY-9420
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582550#comment-17582550
]
Eric Milles commented on GROOVY-8788:
-
6. mixed declaration and extension (pt.2)
{code:groovy}
class A { int m(String s) {4} }
class B extends A { }
class C extends B { }
class Ext {
static m(B self, Object o) {2}
}
use (Ext) {
def a = new A()
assert a.m(new String()) == 4
def b = new B()
assert b.m(new Object()) == 2
assert b.m(new String()) == 4
def c = new C()
assert c.m(new Object()) == 2
assert c.m(new String()) == 4
}
{code}
This case behaves under dynamic and static compilation. So no change required.
:)
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582543#comment-17582543
]
Eric Milles commented on GROOVY-8788:
-
If my understanding of #1-#5 is consistent, this commit does work to fix the
self-type weighting of STC/SC:
https://github.com/apache/groovy/pull/1766/commits/488f8a8f336efae32349317f219a8f8df8994114
I can separate out the getAt change if that helps. There will be some fallout
to having {{getAt(Object,String)}} selected over {{getAt(Map,Object)}}.
And based on all this, I actually noticed that the javadoc assertions for
{{getAt(Map,Object)}} are not even targeting that method. They go to
{{getAt(Object,String)}} because they use string keys.
Further, because {{getAt(Object,String)}} is implemented via getProperty, there
is an edge case where you can extend map and declare a property, which is what
this refers to (edited to reflect current behavior):
{code:groovy}
class HM extends HashMap {
String a = 'x'
}
map = new HM()
map.put("a",1)
assert map["a"] == 'x'
{code}
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582540#comment-17582540
]
Eric Milles commented on GROOVY-8788:
-
4. overload and override -- I think we all expect {{b.m(new String())}} to
return 3 now and Groovy follow suit
{code:groovy}
class A { }
class B extends A { }
class Cat {
static m(A self, String s) {1}
static m(B self, Object o) {2}
static m(B self, String s) {3}
}
use (Cat) {
def b = new B()
assert b.m(new Object()) == 2
assert b.m(new String()) == 3
}
{code}
5. mixed declaration and extension
{code:groovy}
class A { int m(String s) {4} }
class B extends A { }
class Cat {
static m(A self, String s) {1}
static m(B self, Object o) {2}
}
use (Cat) {
def a = new A()
assert a.m(new String()) == 1
def b = new B()
assert b.m(new Object()) == 2
assert b.m(new String()) == 1
}
{code}
Here we have something interesting and new. Groovy chooses extension method
{{m(String)}} over declared method {{m(String)}}. I think this is how {{String
toString(Object self)}} works. Even in this case for {{b.m(new String())}},
the static compiler selects {{m(Object)}} given two {{m(String)}} options.
Further confirmation for me that SC is improperly weighting the self type B.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582532#comment-17582532
]
Eric Milles commented on GROOVY-8788:
-
Let me see if I can break this down from your examples, with some small edits
of mine.
1. overload is more specific -- nothing surprising here
{code:groovy}
class A {
def m(Object o) {1}
}
class B extends A {
def m(String s) {2}
}
def a = new A()
assert a.m(new Object()) == 1
assert a.m(new String()) == 1
def b = new B()
assert b.m(new Object()) == 1
assert b.m(new String()) == 2
{code}
2. overload is more general -- selection is based on argument type
{code:groovy}
import static groovy.test.GroovyAssert.shouldFail
class A {
def m(String s) {1}
}
class B extends A {
def m(Object o) {2}
}
def a = new A()
shouldFail(MissingMethodException) { a.m(new Object()) }
assert a.m(new String()) == 1
def b = new B()
assert b.m(new Object()) == 2
assert b.m(new String()) == 1
{code}
3. category/extension overloads -- same behavior as declared methods; OP
expects this behavior
{code:groovy}
class A { }
class B extends A { }
class Cat {
static m(A self, String s) {1}
static m(B self, Object o) {2}
}
use (Cat) {
def a = new A()
assert a.m(new String()) == 1
def b = new B()
assert b.m(new Object()) == 2
assert b.m(new String()) == 1
}
{code}
Hopefully nothing surprising so far. #1 and #2 run the same under static
compilation. #3 requires a little more work since {{use}} cannot be used under
static compilation.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17582251#comment-17582251
]
Jochen Theodorou commented on GROOVY-8788:
--
Point A is that consistency. The static compiler should behave the same for
extension methods and non extension methods in terms of what extension methods
mean (I made examples above). This is not specific to map of course.
But the trouble here already starts with Point B. From what I gathered
Map would convert GString keys to String. But then in case of
Map I got from your comment that this is not the case. If GString would
really extend String, then this would not matter, but since it does not extend
it we get a problem that does not exist for example in Java. GString->String is
no cast, it is a conversion. Such exist in Java as widening and as boxing. But
since you cannot use a different key for Map than for Map
boxing does not matter. Potentially there would be a problem with trying to use
an int for Map and Map, but I have not checked if that is actually
legal without using any cast. there are also Lambda expressions... but since
they have no specific type on their own the problem does not exist here as well.
I think you can do Point A, Point B+ is a can of worms.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17581945#comment-17581945
]
Eric Milles commented on GROOVY-8788:
-
I did look at putAt. It is declared as "public V putAt(Map map, K
key, V val)". So there is some difference as far as the static compiler is
concerned. For "Map" it says that GString is not compatible with
String. GROOVY-9420 changed this same error for getAt, which now accepts
Object like Map#get does.
Is there a way to break all this down and proceed with stepwise refinements?
8788 is about inconsistent selection of extension methods. I can make the
static compiler select like the dynamic compiler does (the PR) but there would
be consequences for map access and some other things like set intersect. It
sounds like you explicitly wanted the static compiler to work differently in
this space. So is there a play to bring the dynamic compiler in line or should
we just close this out and accept the difference in bahavior?
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17581874#comment-17581874
]
Jochen Theodorou commented on GROOVY-8788:
--
>From the comments I am not sure the fix goes in the right direction - of
>course I could be totally wrong here.
{code:Java}
class A {
def m(o){1}
}
class B extends A {
def m(String s) {2}
}
def a = new A()
assert a.m(new Object()) == 1
def b = new B()
assert b.m(new Object()) == 1
assert b.m("") == 2
{code}
in this case it is obvious that the assert should be fulfilled this way.
Extension methods are kind of overlays for the class.
{code:Java}
class MExt {
static m(A self, String s) {1}
static m(B self, Object o) {2}
}
class A {}
class B extends A {}
@groovy.transform.CompileStatic
void usageExtStatic() {
def b = new B()
assert b.m(new Object()) == 2
assert b.m("") == 1
}
usageExtStatic()
{code}
m(Object) is to be seen as if it is a method of B. m(String) is to be seen as
if a method of A. Since B extends A and since B does not define a new
m(String), the extension method for A has to be chosen here.
{code:Java}
class MExt {
static m(A self, String s) {1}
static m(B self, Object o) {2}
}
class A {}
class B extends A {
def m(String s){3}
}
@groovy.transform.CompileStatic
void usageExtStatic() {
def b = new B()
assert b.m(new Object()) == 2
assert b.m("") == 3
}
usageExtStatic()
{code}
In this case m(String) exists on A as extension method, but B defined its own
m(String), which "overwrites" m(String) from A. Neither overwrite nor overload
are the right terms for this I guess.
class MExt {
static m(A self, String s) {1}
static m(B self, Object o) {2}
}
class A {
def m(String s) {4}
}
class B extends A {}
class C extends B {}
@groovy.transform.CompileStatic
void usageExtStatic() {
def b = new B()
assert b.m(new Object()) == 2
assert b.m("") == 1
def c = new C()
assert c.m(new Object()) == 2
assert c.m("") == 1
}
usageExtStatic()
{code}
Here, even though A defines m(String), the extension method "shadows" it and
inherits it to B. For C again it is the extension methods only.
I know very long comment, but well... the above was for the issue described
here, to show how it is supposed to work - and does in the dynamic case. The
fix though involves getAt(Map,String) and GString:
{code:Java}
class Container {
public someObject = ""
String toString(){ return "Container_" +someObject}
int hashCode() { return 42 + someObject.hashCode() }
boolean equals(o) {
return o!=null && o.class==Container && o.someObject == someObject
}
void newObject(o) {this.someObject = o}
}
def m = new HashMap()
def container = new Container(someObject: "1")
m.put(container, 1)
assert m.get(container) == 1
container.newObject("2")
assert m.get(container) == null
{code}
Here the second get fails even though container itself did not change really.
But since the contained Object did, you cannot access the map value using that
same reference. This is given by Java.
{code:Java}
class Container {
public someObject = ""
String toString(){ return "Container_" +someObject}
int hashCode() { return 42 + someObject.hashCode() }
boolean equals(o) {
return o!=null && o.class==Container && o.someObject == someObject
}
void newObject(o) {this.someObject = o}
}
def m = new HashMap()
def container = new Container(someObject: "1")
m.put("$container", 1)
assert m.get("$container") == 1
container.newObject("2")
assert m.get("$container") == null
{code}
In this version it is a similar situation to before, the only change is that we
create now a ton of GString objects. GString though has the same problem that
Container has, hashcode and equals depend on the contained object, which is
subject to change. Examples are GStrings made from Closures and GStrings made
of containers like here. the more idiomatic usage would be:
{code:Java}
// Container like before
def m = [:]
def container = new Container(someObject: "1")
m["$container"] = 1 //putAt
assert m["$container"] == 1 //getAt
container.newObject("2")
assert m["$container"] == null //getAt
{code}
This proofed to be difficult to understand for the people, which is why we said
we do not allow that for the static compiler.
I noticed you added getAt(Map,String), to allow assert map["a"] == 1 // not 'x'
But what about the putAt? This will go through InvokerHelper, thus map["a"] = 1
will set the property. I think one is not complete without the other. Right
now{code:Java}
class HM extends HashMap {
String a = 'x'
}
def map = new HM()
map["a"] = 1
assert map["a"] == 1
{code}
will fail Groovy because map["a"] sets the property, while map["a"] gets the
map value.
{code:Java}
class HM extends HashMap {
String a = 'x'
}
def map = new HM()
def x = "a"
map["$x"] = 1
assert map["$x"] == 1
{code}
did behave just the same. The reason is that in dynamic Groovy
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17581698#comment-17581698
]
ASF GitHub Bot commented on GROOVY-8788:
daniellansun commented on PR #1766:
URL: https://github.com/apache/groovy/pull/1766#issuecomment-1220335936
I am keen to vote +1
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17581448#comment-17581448
]
ASF GitHub Bot commented on GROOVY-8788:
eric-milles commented on PR #1766:
URL: https://github.com/apache/groovy/pull/1766#issuecomment-1219738720
With the addition of `getAt(Map,String)` a small change in
`StaticTypeCheckingSupport#getDistance` can resolve the `map["$key"]` issues.
I know Cedric stated in 6668 that this should be an STC error, but if there is
just one method and it accepts String, it will be chosen for a GString
argument. This changes just closes the distance between GString and String so
that `m(String)` is selected over `m(Object)`.
https://issues.apache.org/jira/browse/GROOVY-6668
https://issues.apache.org/jira/browse/GROOVY-8212
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17581022#comment-17581022
]
ASF GitHub Bot commented on GROOVY-8788:
sonatype-lift[bot] commented on code in PR #1766:
URL: https://github.com/apache/groovy/pull/1766#discussion_r948409220
##
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingSupport.java:
##
@@ -1103,6 +1104,21 @@ Person foo(B b) {}
onlyExtensionMethods.add(choice);
}
}
+if (onlyExtensionMethods.size() > 1) {
+// GROOVY-8788: prefer closer parameter match over closer
self-type match
+bestDist = Integer.MAX_VALUE; List bestExtensions
= new LinkedList<>();
Review Comment:
*[JdkObsolete](https://errorprone.info/bugpattern/JdkObsolete):* It is very
rare for LinkedList to out-perform ArrayList or ArrayDeque. Avoid it unless
you're willing to invest a lot of time into benchmarking. Caveat: LinkedList
supports null elements, but ArrayDeque does not.
---
```suggestion
bestDist = Integer.MAX_VALUE; List
bestExtensions = new ArrayList<>();
```
---
Reply with *"**@sonatype-lift help**"* for info about LiftBot commands.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the
above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all
the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its
response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get
to know more about LiftBot commands.
---
Was this a good recommendation?
[ [ Not
relevant](https://www.sonatype.com/lift-comment-rating?comment=317131720_comment_rating=1)
] - [ [ Won't
fix](https://www.sonatype.com/lift-comment-rating?comment=317131720_comment_rating=2)
] - [ [ Not critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=317131720_comment_rating=3)
] - [ [ Critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=317131720_comment_rating=4)
] - [ [ Critical, fixing
now](https://www.sonatype.com/lift-comment-rating?comment=317131720_comment_rating=5)
]
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17580975#comment-17580975
]
ASF GitHub Bot commented on GROOVY-8788:
eric-milles opened a new pull request, #1766:
URL: https://github.com/apache/groovy/pull/1766
Given choice between extension methods `m(String,Object)` and
`m(Object,String)`, prefer closer parameter matching. This aligns with the
method selection of the dynamic runtime. So there are some edge cases that
were giving false STC errors or missing them.
This is a breaking change! `getAt(Map,String)` was added to prevent STC
errors for `Type obj = map[str]` expressions.
https://issues.apache.org/jira/browse/GROOVY-8788
https://issues.apache.org/jira/browse/GROOVY-6504
https://issues.apache.org/jira/browse/GROOVY-6849
https://issues.apache.org/jira/browse/GROOVY-6970
https://issues.apache.org/jira/browse/GROOVY-8787
https://issues.apache.org/jira/browse/GROOVY-9069
https://issues.apache.org/jira/browse/GROOVY-9420
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17580956#comment-17580956
]
Eric Milles commented on GROOVY-8788:
-
One thing that would reduce the impact of this change is to add
{{getAt(Map,String)}} extension method to step in front of
{{getAt(Object,String)}}.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Assignee: Eric Milles
>Priority: Major
> Labels: breaking
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17580951#comment-17580951
]
Eric Milles commented on GROOVY-8788:
-
I can fix this by updating {{StaticTypeCheckingSupport#chooseBestMethod}} (see
below). There are a number of cases that stretch intuition and may result in
breaking existing code. In both cases the method on the left is chosen by the
static compiler and the method on the right is chosen by the dynamic runtime
and the new static compiler (with chooseBestMethod below).
The dynamic runtime has preference for parameter matching and the static
compiler prefers receiver matching. This is the source of the problem
described in the original post. Changing this preference will bring closer
alignment for the two modes but has some consequences.
{code:groovy}
@groovy.transform.CompileStatic void test(Set one, Set two) {
one.intersect(two) // Set#intersect(Iterable) vs.
Collection#intersect(Collection)
}
@groovy.transform.CompileStatic void test(Map map, String key) {
Number num = map[key] // Map#getAt(Object) vs. Object#getAt(String)
}
{code}
[~paulk] [~blackdrag]
{code:java}
/**
* Returns the method(s) which best fit the argument types.
*
* @return zero or more results
*/
public static List chooseBestMethod(final ClassNode receiver,
final Collection methods, final ClassNode... argumentTypes) {
if (!asBoolean(methods)) {
return Collections.emptyList();
}
int bestDist = Integer.MAX_VALUE;
List bestChoices = new LinkedList<>();
boolean noCulling = methods.size() <= 1 ||
"".equals(methods.iterator().next().getName());
Iterable candidates = noCulling ? methods :
removeCovariantsAndInterfaceEquivalents(methods);
for (MethodNode candidate : candidates) {
MethodNode safeNode = candidate;
ClassNode[] safeArgs = argumentTypes;
boolean isExtensionMethod = candidate instanceof
ExtensionMethodNode;
if (isExtensionMethod) {
int nArgs = argumentTypes.length;
safeArgs = new ClassNode[nArgs + 1];
System.arraycopy(argumentTypes, 0, safeArgs, 1, nArgs);
safeArgs[0] = receiver; // prepend self-type as first argument
safeNode = ((ExtensionMethodNode)
candidate).getExtensionMethodNode();
}
/* TODO: corner case
class B extends A {}
Animal foo(A a) {}
Person foo(B b) {}
B b = new B()
Person p = foo(b)
*/
ClassNode declaringClass = candidate.getDeclaringClass();
ClassNode actualReceiver = receiver != null ? receiver :
declaringClass;
Map spec;
if (/*!isExtensionMethod && */candidate.isStatic()) {
spec = Collections.emptyMap(); // none visible
} else {
spec =
GenericsUtils.makeDeclaringAndActualGenericsTypeMapOfExactType(declaringClass,
actualReceiver);
GenericsType[] methodGenerics = candidate.getGenericsTypes();
if (/*!isExtensionMethod && */methodGenerics != null) { //
GROOVY-10322: remove hidden type parameters
for (int i = 0, n = methodGenerics.length; i < n &&
!spec.isEmpty(); i += 1) {
for (Iterator it =
spec.keySet().iterator(); it.hasNext(); ) {
if
(it.next().getName().equals(methodGenerics[i].getName())) it.remove();
}
}
}
}
Parameter[] params = makeRawTypes(safeNode.getParameters(), spec);
int dist = measureParametersAndArgumentsDistance(params, safeArgs);
if (dist >= 0) {
if (!isExtensionMethod) { // GROOVY-6849, GROOVY-8788, et al.
dist += (1 + getClassDistance(declaringClass,
actualReceiver));
}
if (dist < bestDist) {
bestDist = dist;
bestChoices.clear();
bestChoices.add(candidate);
} else if (dist == bestDist) {
bestChoices.add(candidate);
}
}
}
if (bestChoices.size() > 1) {
// GROOVY-6849: prefer extension method in case of ambiguity
List onlyExtensionMethods = new LinkedList<>();
for (MethodNode choice : bestChoices) {
if (choice instanceof ExtensionMethodNode) {
onlyExtensionMethods.add(choice);
}
}
if (onlyExtensionMethods.size() > 1) {
// GROOVY-8788: prefer closer parameter match over closer
self-type match
bestDist = Integer.MAX_VALUE; List bestExtensions =
new
[jira] [Commented] (GROOVY-8788) Inconsistency in extension method selection with @CompileStatic
[
https://issues.apache.org/jira/browse/GROOVY-8788?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16613377#comment-16613377
]
Daniil Ovchinnikov commented on GROOVY-8788:
Possibly the same cause.
> Inconsistency in extension method selection with @CompileStatic
> ---
>
> Key: GROOVY-8788
> URL: https://issues.apache.org/jira/browse/GROOVY-8788
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation, Static Type Checker
>Affects Versions: 2.4.15, 2.5.2
>Reporter: Daniil Ovchinnikov
>Priority: Major
>
> Given properly registered extension class:
> {code:java|title=MyExtensions.java}
> public class MyExtensions {
> public static void foo(Object self, String s) {
> System.out.println("Object#foo(String)");
> }
> public static void foo(String self, Object o) {
> System.out.println("String#foo(Object)");
> }
> }
> {code}
> Run
> {code:java|title=playground.groovy}
> void usageExt() {
> "".foo("") // prints "Object#foo(String)" which is correct
> }
> @groovy.transform.CompileStatic
> void usageExtStatic() {
> "".foo("") // prints "String#foo(Object)" which is questionable
> }
> usageExt()
> usageExtStatic()
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
