You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@groovy.apache.org by "Milles, Eric (TR Tech, Content & Ops)" <er...@thomsonreuters.com> on 2020/06/04 15:07:12 UTC
STC: Closure shared variable assignment handling
There are a couple open bugs related to STC handling of closure shared variables. When a shared variable is assigned a value within the closure, the assigned type is not included in the inferred type outside the closure/lambda and this leads to some problems.
If this behavior is changed to save the LUB(A,B) as the inferred type of "x", as is suggested in 9516 and seems required by 9344, the second pass checks for method calls will be replaced by no matching method errors. That is "Cannot find matching method java.lang.Object#something(). Please check if the declared type is correct and if the method exists." will replace "A closure shared variable [x] has been assigned with various types and the method [charAt(int)] does not exist in the lowest upper bound...". Is this an acceptable change in STC behavior?
@groovy.transform.CompileStatic
void test() {
def x = new A();
{ -> x = new B() } // may also be called immediately
x.something() // current STC infers typeof(x) == A
}
I think it was done this way because you could define the closure but not call it (like above). Or call it asynchronously:
@groovy.transform.CompileStatic
void test() {
def x = new A();
Thread.start { -> x = new B() }
x.something() // current STC infers typeof(x) == A
}
https://issues.apache.org/jira/browse/GROOVY-9516
https://issues.apache.org/jira/browse/GROOVY-9344
https://issues.apache.org/jira/browse/GROOVY-5874
This e-mail is for the sole use of the intended recipient and contains information that may be privileged and/or confidential. If you are not an intended recipient, please notify the sender by return e-mail and delete this e-mail and any attachments. Certain required legal entity disclosures can be accessed on our website: https://www.thomsonreuters.com/en/resources/disclosures.html
Re: STC: Closure shared variable assignment handling
Posted by Paul King <pa...@asert.com.au>.
Ah yes, Stack not Number, that was just me checking whether a false case
was detected and I forgot to change it back.
On Sat, Jun 6, 2020 at 3:24 PM Jochen Theodorou <bl...@gmx.org> wrote:
> On 06.06.20 06:57, Paul King wrote:
> > Here is a good example for your point Jochen:
> >
> > ///////////////////
> > @groovy.transform.TypeChecked
> > def method() {
> > def component = new Random().nextBoolean() ? new ArrayDeque() : new
> Stack()
> > component.clear() // 'clear' in LUB (AbstractCollection or
> Serializable or Cloneable)
> > if (component instanceof ArrayDeque) {
> > component.addFirst(1) // 'addFirst' only in ArrayDeque
> > } else if (component instanceof Stack) {
> > component.addElement(2) // 'addElement' only in Stack
> > }
> > if (component instanceof ArrayDeque || component instanceof Number)
> {
> > // checked duck typing
> > assert component.peek() in 1..2 // 'peek' in ArrayDeque and
> Stack but not LUB
> > }
> > }
> >
> > method()
> > ///////////////////
>
> I guess
>
> if (component instanceof ArrayDeque || component instanceof Number) {
>
> is really
>
> if (component instanceof ArrayDeque || component instanceof Stack) {
>
> > The peek() case is what you call LUBU. I am still not sure what the best
> > name is.
> > This is where we'd need either an invokedynamic or smarter multi-branch
> > logic with several alternative invokevirtual paths.
>
> I would go with invokedynamic. That is better for debugging and code
> coverage tools.
>
> bye Jochen
>
>
Re: STC: Closure shared variable assignment handling
Posted by Jochen Theodorou <bl...@gmx.org>.
On 06.06.20 06:57, Paul King wrote:
> Here is a good example for your point Jochen:
>
> ///////////////////
> @groovy.transform.TypeChecked
> def method() {
> def component = new Random().nextBoolean() ? new ArrayDeque() : new Stack()
> component.clear() // 'clear' in LUB (AbstractCollection or Serializable or Cloneable)
> if (component instanceof ArrayDeque) {
> component.addFirst(1) // 'addFirst' only in ArrayDeque
> } else if (component instanceof Stack) {
> component.addElement(2) // 'addElement' only in Stack
> }
> if (component instanceof ArrayDeque || component instanceof Number) {
> // checked duck typing
> assert component.peek() in 1..2 // 'peek' in ArrayDeque and Stack but not LUB
> }
> }
>
> method()
> ///////////////////
I guess
if (component instanceof ArrayDeque || component instanceof Number) {
is really
if (component instanceof ArrayDeque || component instanceof Stack) {
> The peek() case is what you call LUBU. I am still not sure what the best
> name is.
> This is where we'd need either an invokedynamic or smarter multi-branch
> logic with several alternative invokevirtual paths.
I would go with invokedynamic. That is better for debugging and code
coverage tools.
bye Jochen
Re: STC: Closure shared variable assignment handling
Posted by Paul King <pa...@asert.com.au>.
Here is a good example for your point Jochen:
///////////////////
@groovy.transform.TypeChecked
def method() {
def component = new Random().nextBoolean() ? new ArrayDeque() : new
Stack()
component.clear() // 'clear' in LUB (AbstractCollection or Serializable
or Cloneable)
if (component instanceof ArrayDeque) {
component.addFirst(1) // 'addFirst' only in ArrayDeque
} else if (component instanceof Stack) {
component.addElement(2) // 'addElement' only in Stack
}
if (component instanceof ArrayDeque || component instanceof Number) {
// checked duck typing
assert component.peek() in 1..2 // 'peek' in ArrayDeque and Stack
but not LUB
}
}
method()
///////////////////
The peek() case is what you call LUBU. I am still not sure what the best
name is.
This is where we'd need either an invokedynamic or smarter multi-branch
logic with several alternative invokevirtual paths.
Cheers, Paul.
On Sat, Jun 6, 2020 at 2:44 PM Jochen Theodorou <bl...@gmx.org> wrote:
> On 04.06.20 21:45, Daniil Ovchinnikov wrote:
> [...]
> >> If we do not give up on flow typing, then I would like to
> >> know why the assignment should not be legal. And if it is legal in the
> >> if-branch, then why not in the Closure block?
> >
> > All assignments should be legal, and assignment in Closure block as well.
>
> to be clear. An assignment should be legal if the flow type of a
> variable and the static type of a variable are compatible. The new type
> of the variable is then the flow type (in Java it would stay being the
> static type).
>
> > Let me annotate the last example with types:
> >
> > class A {}
> > class B extends A{ def b() {}}
> > class C extends A{}
> >
> > A x // inferred to A because LUB(B,C) = A
> > x = new B()
> > // typeOf(x) as this point is B due to flow typing
> > if (true) {
> > x = new C()
> > // typeOf(x) as this point is C due to flow typing
> > }
> > // typeOf(x) as this point is A due to flow typing merging branches
> where typeOf(x) is B and C
> > A z // inferred to A because LUB(A) = A
> > z = x
> > // typeOf(z) as this point is A
> > z.b() // STC error, no method b in A
>
> Which means we are on the same page, only that you work with LUB and I
> with LUBU. I know LUBU is a difficult thing. Not really in
> understanding, but in teaching the JVM, the compiler and the IDEs. The
> "normal" LUB is much more easy here.
>
> bye Jochen
>
Re: STC: Closure shared variable assignment handling
Posted by Jochen Theodorou <bl...@gmx.org>.
On 04.06.20 21:45, Daniil Ovchinnikov wrote:
[...]
>> If we do not give up on flow typing, then I would like to
>> know why the assignment should not be legal. And if it is legal in the
>> if-branch, then why not in the Closure block?
>
> All assignments should be legal, and assignment in Closure block as well.
to be clear. An assignment should be legal if the flow type of a
variable and the static type of a variable are compatible. The new type
of the variable is then the flow type (in Java it would stay being the
static type).
> Let me annotate the last example with types:
>
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> A x // inferred to A because LUB(B,C) = A
> x = new B()
> // typeOf(x) as this point is B due to flow typing
> if (true) {
> x = new C()
> // typeOf(x) as this point is C due to flow typing
> }
> // typeOf(x) as this point is A due to flow typing merging branches where typeOf(x) is B and C
> A z // inferred to A because LUB(A) = A
> z = x
> // typeOf(z) as this point is A
> z.b() // STC error, no method b in A
Which means we are on the same page, only that you work with LUB and I
with LUBU. I know LUBU is a difficult thing. Not really in
understanding, but in teaching the JVM, the compiler and the IDEs. The
"normal" LUB is much more easy here.
bye Jochen
Re: STC: Closure shared variable assignment handling
Posted by Daniil Ovchinnikov <da...@jetbrains.com>.
typeOf ... *at* this point
Sorry.
> On 4 Jun 2020, at 22:45, Daniil Ovchinnikov <Da...@jetbrains.com> wrote:
>
> Hi -
>
>> Of course if we want to define intuitive as "do it like Java", we make
>> def an alias for var, take its semantics and give up on Flow typing in
>> general.
>
> Nobody wants to give up flow typing.
>
>> If we do not give up on flow typing, then I would like to
>> know why the assignment should not be legal. And if it is legal in the
>> if-branch, then why not in the Closure block?
>
> All assignments should be legal, and assignment in Closure block as well.
>
> Let me annotate the last example with types:
>
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> A x // inferred to A because LUB(B,C) = A
> x = new B()
> // typeOf(x) as this point is B due to flow typing
> if (true) {
> x = new C()
> // typeOf(x) as this point is C due to flow typing
> }
> // typeOf(x) as this point is A due to flow typing merging branches where typeOf(x) is B and C
> A z // inferred to A because LUB(A) = A
> z = x
> // typeOf(z) as this point is A
> z.b() // STC error, no method b in A
>
> —
> Daniil Ovchinnikov
> JetBrains
>
>
>> On 4 Jun 2020, at 21:47, Jochen Theodorou <bl...@gmx.org> wrote:
>>
>> On 04.06.20 18:55, Milles, Eric (TR Tech, Content & Ops) wrote:
>>> I am suggesting that when a closure is encountered, it is treated like an optional branch of the code. So assignments within alter the inferred type the same way an assignment within an if with no else would.
>>
>> sure I am for that, but I am not sure we are doing those right either ;)
>>
>>> I am not looking to examine the code flow and distinguish between closure executed immediately and closure that is executed asynchronously or later or not at all.
>>
>> me neither
>>
>> [...]
>>> If you are talking about using a union type,
>>
>> did not want to really use that word, because I think people understand
>> different things about it, but basically yes. Let me call it Least Upper
>> Bounds Union Type (LUBU)
>>
>>> I don't think that is the safe way to go. Static compilation is enforcing a typecast to the inferred type and this is a runtime error for 9344.
>>
>> If you say that then
>>
>>> class A{}
>>> class B{}
>>>
>>> def a = new A()
>>> a = new B()
>>
>> must fail, because the inferred type for a is A, so I cannot assign B to
>> it. That is quite far away from the initial idea. Of course you are more
>> referring to
>>
>>> class A{}
>>> class B{}
>>> class C{}
>>>
>>> def a = new A()
>>> if (b) {
>>> a = new B()
>>> }
>>> a = new C()
>>
>> but again, each of these assignments are legal in my opinion. The real
>> question should be if in
>>
>>> class A {}
>>> class B extends A{ def b() {}}
>>> class C extends A{}
>>>
>>> def x = new B()
>>> if (true) { x = new C() }
>>> def z = x
>>> z.b()
>>
>> The call z.b() is legal. And my answer is here with LUBU yes, with LUB,
>> as we have defined it probably no. But I find the later not intuitive.
>>
>> Of course if we want to define intuitive as "do it like Java", we make
>> def an alias for var, take its semantics and give up on Flow typing in
>> general. Then my two examples here should both fail compilation on the
>> reassignment. If we do not give up on flow typing, then I would like to
>> know why the assignment should not be legal. And if it is legal in the
>> if-branch, then why not in the Closure block?
>>
>>
>> bye Jochen
>
Re: STC: Closure shared variable assignment handling
Posted by Daniil Ovchinnikov <da...@jetbrains.com>.
Hi -
> Of course if we want to define intuitive as "do it like Java", we make
> def an alias for var, take its semantics and give up on Flow typing in
> general.
Nobody wants to give up flow typing.
> If we do not give up on flow typing, then I would like to
> know why the assignment should not be legal. And if it is legal in the
> if-branch, then why not in the Closure block?
All assignments should be legal, and assignment in Closure block as well.
Let me annotate the last example with types:
class A {}
class B extends A{ def b() {}}
class C extends A{}
A x // inferred to A because LUB(B,C) = A
x = new B()
// typeOf(x) as this point is B due to flow typing
if (true) {
x = new C()
// typeOf(x) as this point is C due to flow typing
}
// typeOf(x) as this point is A due to flow typing merging branches where typeOf(x) is B and C
A z // inferred to A because LUB(A) = A
z = x
// typeOf(z) as this point is A
z.b() // STC error, no method b in A
—
Daniil Ovchinnikov
JetBrains
> On 4 Jun 2020, at 21:47, Jochen Theodorou <bl...@gmx.org> wrote:
>
> On 04.06.20 18:55, Milles, Eric (TR Tech, Content & Ops) wrote:
>> I am suggesting that when a closure is encountered, it is treated like an optional branch of the code. So assignments within alter the inferred type the same way an assignment within an if with no else would.
>
> sure I am for that, but I am not sure we are doing those right either ;)
>
>> I am not looking to examine the code flow and distinguish between closure executed immediately and closure that is executed asynchronously or later or not at all.
>
> me neither
>
> [...]
>> If you are talking about using a union type,
>
> did not want to really use that word, because I think people understand
> different things about it, but basically yes. Let me call it Least Upper
> Bounds Union Type (LUBU)
>
>> I don't think that is the safe way to go. Static compilation is enforcing a typecast to the inferred type and this is a runtime error for 9344.
>
> If you say that then
>
>> class A{}
>> class B{}
>>
>> def a = new A()
>> a = new B()
>
> must fail, because the inferred type for a is A, so I cannot assign B to
> it. That is quite far away from the initial idea. Of course you are more
> referring to
>
>> class A{}
>> class B{}
>> class C{}
>>
>> def a = new A()
>> if (b) {
>> a = new B()
>> }
>> a = new C()
>
> but again, each of these assignments are legal in my opinion. The real
> question should be if in
>
>> class A {}
>> class B extends A{ def b() {}}
>> class C extends A{}
>>
>> def x = new B()
>> if (true) { x = new C() }
>> def z = x
>> z.b()
>
> The call z.b() is legal. And my answer is here with LUBU yes, with LUB,
> as we have defined it probably no. But I find the later not intuitive.
>
> Of course if we want to define intuitive as "do it like Java", we make
> def an alias for var, take its semantics and give up on Flow typing in
> general. Then my two examples here should both fail compilation on the
> reassignment. If we do not give up on flow typing, then I would like to
> know why the assignment should not be legal. And if it is legal in the
> if-branch, then why not in the Closure block?
>
>
> bye Jochen
Re: STC: Closure shared variable assignment handling
Posted by Jochen Theodorou <bl...@gmx.org>.
On 04.06.20 18:55, Milles, Eric (TR Tech, Content & Ops) wrote:
> I am suggesting that when a closure is encountered, it is treated like an optional branch of the code. So assignments within alter the inferred type the same way an assignment within an if with no else would.
sure I am for that, but I am not sure we are doing those right either ;)
> I am not looking to examine the code flow and distinguish between closure executed immediately and closure that is executed asynchronously or later or not at all.
me neither
[...]
> If you are talking about using a union type,
did not want to really use that word, because I think people understand
different things about it, but basically yes. Let me call it Least Upper
Bounds Union Type (LUBU)
> I don't think that is the safe way to go. Static compilation is enforcing a typecast to the inferred type and this is a runtime error for 9344.
If you say that then
> class A{}
> class B{}
>
> def a = new A()
> a = new B()
must fail, because the inferred type for a is A, so I cannot assign B to
it. That is quite far away from the initial idea. Of course you are more
referring to
> class A{}
> class B{}
> class C{}
>
> def a = new A()
> if (b) {
> a = new B()
> }
> a = new C()
but again, each of these assignments are legal in my opinion. The real
question should be if in
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> def x = new B()
> if (true) { x = new C() }
> def z = x
> z.b()
The call z.b() is legal. And my answer is here with LUBU yes, with LUB,
as we have defined it probably no. But I find the later not intuitive.
Of course if we want to define intuitive as "do it like Java", we make
def an alias for var, take its semantics and give up on Flow typing in
general. Then my two examples here should both fail compilation on the
reassignment. If we do not give up on flow typing, then I would like to
know why the assignment should not be legal. And if it is legal in the
if-branch, then why not in the Closure block?
bye Jochen
Re: STC: Closure shared variable assignment handling
Posted by Paul King <pa...@asert.com.au>.
Comment below.
On Fri, Jun 5, 2020 at 2:55 AM Milles, Eric (TR Tech, Content & Ops) <
eric.milles@thomsonreuters.com> wrote:
> I am suggesting that when a closure is encountered, it is treated like an
> optional branch of the code. So assignments within alter the inferred type
> the same way an assignment within an if with no else would. I am not
> looking to examine the code flow and distinguish between closure executed
> immediately and closure that is executed asynchronously or later or not at
> all. I have this change in hand, but did not want to alter the "second
> pass" checking without discussing it first.
>
> If you are talking about using a union type, I don't think that is the
> safe way to go. Static compilation is enforcing a typecast to the inferred
> type and this is a runtime error for 9344.
>
I think the union type* is the correct thing here and we should make our
bytecode generation be more sophisticated.
* agree this is a poorly understood concept but we need both union and
intersection types but perhaps we should use different names.
Cheers, Paul.
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> @CompileStatic
> void test() {
> def x = new B()
> ({ x = new C() })()
> def z = x // new behavior: typeof(x) and typeof(z) is A
> z.b() // new behavior: STC error because "b()" is not a member of A;
> user would need to use block instead of closure or add a typecast or type
> checking extension
> }
>
> -----Original Message-----
> From: Jochen Theodorou <bl...@gmx.org>
> Sent: Thursday, June 4, 2020 11:35 AM
> To: dev@groovy.apache.org
> Subject: Re: STC: Closure shared variable assignment handling
>
> On 04.06.20 17:07, Milles, Eric (TR Tech, Content & Ops) wrote:
> > There are a couple open bugs related to STC handling of closure shared
> > variables. When a shared variable is assigned a value within the
> > closure, the assigned type is not included in the inferred type
> > outside the closure/lambda and this leads to some problems.
> >
> > If this behavior is changed to save the LUB(A,B) as the inferred type
> > of "x", as is suggested in 9516 and seems required by 9344, the second
> > pass checks for method calls will be replaced by no matching method
> errors.
> > That is "Cannot find matching method java.lang.Object#something().
> > Please check if the declared type is correct and if the method exists."
> > will replace "A closure shared variable [x] has been assigned with
> > various types and the method [charAt(int)] does not exist in the
> > lowest upper bound...".* Is this an acceptable change in STC
> > behavior?*
> >
> > @groovy.transform.CompileStatic
> >
> > void test() {
> >
> > def x = new A();
> >
> > { -> x = new B() } // may also be called immediately
> >
> > x.something() // current STC infers typeof(x) == A
> >
> > }
>
> as this is currently failing you are suggesting to let it pass and use the
> type A?
>
> > I think it was done this way because you could define the closure but
> > not call it (like above). Or call it asynchronously:
> >
> > @groovy.transform.CompileStatic
> >
> > void test() {
> >
> > def x = new A();
> >
> > Thread.start { -> x = new B() }
> >
> > x.something() // current STC infers typeof(x) == A
> >
> > }
>
> would you not agree, just by looking at the realized flow here, that x
> should not be A?
>
> > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> > es.apache.org%2Fjira%2Fbrowse%2FGROOVY-9516&data=02%7C01%7Ceric.mi
> > lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> > 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358509821&sdata=zoylX
> > L4c9Iic%2FHhoVXxE9iXSV2kp4BBQW2m8wEUYeeM%3D&reserved=0
>
> this is
>
> > class A {}
> > class B extends A{ def b() {}}
> > class C extends A{}
> >
> > @CompileStatic
> > static foo() {
> > def x = new B()
> > ({ x = new C() })()
> > def z = x
> > z.b()
> > }
>
> ah.. now I get the problem. but to say z is of type C is also wrong.
> What we really need here is not really the LUB in the sense that
> LUB(B,C) = A, iff B extends A && C extends A && B and C are siblings.
> What we would really need is a type A plus all methods defined on B and C.
> I vaguely remember that our LUB calculation does things like that..
> but maybe I remember wrong. If I go by
>
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.groovy-lang.org%2Flatest%2Fhtml%2Fdocumentation%2Fcore-semantics.html%23section-lub&data=02%7C01%7Ceric.milles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb8646a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=NgKVDLj5YZdz43BRn9Du2%2F5yNxoj78BWVWRzAEXSdlk%3D&reserved=0
> then I am wrong. Of course I am well aware, that such a type is difficult
> to realize. And of course the question is how to do the actual method call
> in bytecode. If I cannot cast z to B or B, then I cannot create the
> required invokevirtual. This means this must use an invokedynamic, but a
> different one than we normally use.
>
> What I am trying to say is that we can handle this case as I imagine it,
> but the required implementation work is not small.
>
> > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> > es.apache.org%2Fjira%2Fbrowse%2FGROOVY-9344&data=02%7C01%7Ceric.mi
> > lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> > 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=qHXA1
> > 4ljlSoFfGd3Y6FskDqkUkqF4fnOt1T7BTwbnQ8%3D&reserved=0
>
> This is
>
> > class A {}
> > class B {}
> >
> > @groovy.transform.CompileStatic
> > def cs() {
> > def var
> > var = new A()
> > def c = {
> > var = new B() // Cannot cast object 'B@4e234c52' with class 'B'
> to class 'A'
> > }
> > c()
> > var.toString()
> > }
> >
> > assert cs() != null
>
> which looks to me like a different problem, var is declared with "def", so
> assigning a B should be valid. Only if the static type does no comply with
> the assigned type it should cause an error. There should be also no cast
> inserted.
>
> > https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> > es.apache.org%2Fjira%2Fbrowse%2FGROOVY-5874&data=02%7C01%7Ceric.mi
> > lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> > 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=oLyIU
> > IbjMuxQCbLXT5RKs0vGxg%2BRH17syiM4dHyWlgg%3D&reserved=0
>
> This is
>
> > @CompileStatic
> > static main(args)
> > {
> > def list = [1, 2, 3]
> >
> > def sum = 0
> > list.each { int i -> sum += i }
> > println(sum)
> >
> > sum = list.inject(0, { int x, int y -> x + y })
> > println(sum)
> > }
>
> what I would imagine here is that in def sum, the flow type for sum is int
> and for list it is List<Integer>. That means sum+=i should change the flow
> type internally to Integer or keep int. The result of the inject should be
> Integer and it should always be allowed to assign it to sum, even if not.
>
> bye Jochen
>
>
RE: STC: Closure shared variable assignment handling
Posted by "Milles, Eric (TR Tech, Content & Ops)" <er...@thomsonreuters.com>.
I am suggesting that when a closure is encountered, it is treated like an optional branch of the code. So assignments within alter the inferred type the same way an assignment within an if with no else would. I am not looking to examine the code flow and distinguish between closure executed immediately and closure that is executed asynchronously or later or not at all. I have this change in hand, but did not want to alter the "second pass" checking without discussing it first.
If you are talking about using a union type, I don't think that is the safe way to go. Static compilation is enforcing a typecast to the inferred type and this is a runtime error for 9344.
class A {}
class B extends A{ def b() {}}
class C extends A{}
@CompileStatic
void test() {
def x = new B()
({ x = new C() })()
def z = x // new behavior: typeof(x) and typeof(z) is A
z.b() // new behavior: STC error because "b()" is not a member of A; user would need to use block instead of closure or add a typecast or type checking extension
}
-----Original Message-----
From: Jochen Theodorou <bl...@gmx.org>
Sent: Thursday, June 4, 2020 11:35 AM
To: dev@groovy.apache.org
Subject: Re: STC: Closure shared variable assignment handling
On 04.06.20 17:07, Milles, Eric (TR Tech, Content & Ops) wrote:
> There are a couple open bugs related to STC handling of closure shared
> variables. When a shared variable is assigned a value within the
> closure, the assigned type is not included in the inferred type
> outside the closure/lambda and this leads to some problems.
>
> If this behavior is changed to save the LUB(A,B) as the inferred type
> of "x", as is suggested in 9516 and seems required by 9344, the second
> pass checks for method calls will be replaced by no matching method errors.
> That is "Cannot find matching method java.lang.Object#something().
> Please check if the declared type is correct and if the method exists."
> will replace "A closure shared variable [x] has been assigned with
> various types and the method [charAt(int)] does not exist in the
> lowest upper bound...".* Is this an acceptable change in STC
> behavior?*
>
> @groovy.transform.CompileStatic
>
> void test() {
>
> def x = new A();
>
> { -> x = new B() } // may also be called immediately
>
> x.something() // current STC infers typeof(x) == A
>
> }
as this is currently failing you are suggesting to let it pass and use the type A?
> I think it was done this way because you could define the closure but
> not call it (like above). Or call it asynchronously:
>
> @groovy.transform.CompileStatic
>
> void test() {
>
> def x = new A();
>
> Thread.start { -> x = new B() }
>
> x.something() // current STC infers typeof(x) == A
>
> }
would you not agree, just by looking at the realized flow here, that x should not be A?
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> es.apache.org%2Fjira%2Fbrowse%2FGROOVY-9516&data=02%7C01%7Ceric.mi
> lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358509821&sdata=zoylX
> L4c9Iic%2FHhoVXxE9iXSV2kp4BBQW2m8wEUYeeM%3D&reserved=0
this is
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> @CompileStatic
> static foo() {
> def x = new B()
> ({ x = new C() })()
> def z = x
> z.b()
> }
ah.. now I get the problem. but to say z is of type C is also wrong.
What we really need here is not really the LUB in the sense that
LUB(B,C) = A, iff B extends A && C extends A && B and C are siblings.
What we would really need is a type A plus all methods defined on B and C. I vaguely remember that our LUB calculation does things like that..
but maybe I remember wrong. If I go by
https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.groovy-lang.org%2Flatest%2Fhtml%2Fdocumentation%2Fcore-semantics.html%23section-lub&data=02%7C01%7Ceric.milles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb8646a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=NgKVDLj5YZdz43BRn9Du2%2F5yNxoj78BWVWRzAEXSdlk%3D&reserved=0
then I am wrong. Of course I am well aware, that such a type is difficult to realize. And of course the question is how to do the actual method call in bytecode. If I cannot cast z to B or B, then I cannot create the required invokevirtual. This means this must use an invokedynamic, but a different one than we normally use.
What I am trying to say is that we can handle this case as I imagine it, but the required implementation work is not small.
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> es.apache.org%2Fjira%2Fbrowse%2FGROOVY-9344&data=02%7C01%7Ceric.mi
> lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=qHXA1
> 4ljlSoFfGd3Y6FskDqkUkqF4fnOt1T7BTwbnQ8%3D&reserved=0
This is
> class A {}
> class B {}
>
> @groovy.transform.CompileStatic
> def cs() {
> def var
> var = new A()
> def c = {
> var = new B() // Cannot cast object 'B@4e234c52' with class 'B' to class 'A'
> }
> c()
> var.toString()
> }
>
> assert cs() != null
which looks to me like a different problem, var is declared with "def", so assigning a B should be valid. Only if the static type does no comply with the assigned type it should cause an error. There should be also no cast inserted.
> https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissu
> es.apache.org%2Fjira%2Fbrowse%2FGROOVY-5874&data=02%7C01%7Ceric.mi
> lles%40thomsonreuters.com%7C7b12f318629043d33b9d08d808a54e51%7C62ccb86
> 46a1a4b5d8e1c397dec1a8258%7C0%7C0%7C637268853358519815&sdata=oLyIU
> IbjMuxQCbLXT5RKs0vGxg%2BRH17syiM4dHyWlgg%3D&reserved=0
This is
> @CompileStatic
> static main(args)
> {
> def list = [1, 2, 3]
>
> def sum = 0
> list.each { int i -> sum += i }
> println(sum)
>
> sum = list.inject(0, { int x, int y -> x + y })
> println(sum)
> }
what I would imagine here is that in def sum, the flow type for sum is int and for list it is List<Integer>. That means sum+=i should change the flow type internally to Integer or keep int. The result of the inject should be Integer and it should always be allowed to assign it to sum, even if not.
bye Jochen
Re: STC: Closure shared variable assignment handling
Posted by Jochen Theodorou <bl...@gmx.org>.
On 04.06.20 17:07, Milles, Eric (TR Tech, Content & Ops) wrote:
> There are a couple open bugs related to STC handling of closure shared
> variables. When a shared variable is assigned a value within the
> closure, the assigned type is not included in the inferred type outside
> the closure/lambda and this leads to some problems.
>
> If this behavior is changed to save the LUB(A,B) as the inferred type of
> "x", as is suggested in 9516 and seems required by 9344, the second pass
> checks for method calls will be replaced by no matching method errors.
> That is "Cannot find matching method java.lang.Object#something().
> Please check if the declared type is correct and if the method exists."
> will replace "A closure shared variable [x] has been assigned with
> various types and the method [charAt(int)] does not exist in the lowest
> upper bound...".* Is this an acceptable change in STC behavior?*
>
> @groovy.transform.CompileStatic
>
> void test() {
>
> def x = new A();
>
> { -> x = new B() } // may also be called immediately
>
> x.something() // current STC infers typeof(x) == A
>
> }
as this is currently failing you are suggesting to let it pass and use
the type A?
> I think it was done this way because you could define the closure but
> not call it (like above). Or call it asynchronously:
>
> @groovy.transform.CompileStatic
>
> void test() {
>
> def x = new A();
>
> Thread.start { -> x = new B() }
>
> x.something() // current STC infers typeof(x) == A
>
> }
would you not agree, just by looking at the realized flow here, that x
should not be A?
> https://issues.apache.org/jira/browse/GROOVY-9516
this is
> class A {}
> class B extends A{ def b() {}}
> class C extends A{}
>
> @CompileStatic
> static foo() {
> def x = new B()
> ({ x = new C() })()
> def z = x
> z.b()
> }
ah.. now I get the problem. but to say z is of type C is also wrong.
What we really need here is not really the LUB in the sense that
LUB(B,C) = A, iff B extends A && C extends A && B and C are siblings.
What we would really need is a type A plus all methods defined on B and
C. I vaguely remember that our LUB calculation does things like that..
but maybe I remember wrong. If I go by
https://docs.groovy-lang.org/latest/html/documentation/core-semantics.html#section-lub
then I am wrong. Of course I am well aware, that such a type is
difficult to realize. And of course the question is how to do the actual
method call in bytecode. If I cannot cast z to B or B, then I cannot
create the required invokevirtual. This means this must use an
invokedynamic, but a different one than we normally use.
What I am trying to say is that we can handle this case as I imagine it,
but the required implementation work is not small.
> https://issues.apache.org/jira/browse/GROOVY-9344
This is
> class A {}
> class B {}
>
> @groovy.transform.CompileStatic
> def cs() {
> def var
> var = new A()
> def c = {
> var = new B() // Cannot cast object 'B@4e234c52' with class 'B' to class 'A'
> }
> c()
> var.toString()
> }
>
> assert cs() != null
which looks to me like a different problem, var is declared with "def",
so assigning a B should be valid. Only if the static type does no comply
with the assigned type it should cause an error. There should be also no
cast inserted.
> https://issues.apache.org/jira/browse/GROOVY-5874
This is
> @CompileStatic
> static main(args)
> {
> def list = [1, 2, 3]
>
> def sum = 0
> list.each { int i -> sum += i }
> println(sum)
>
> sum = list.inject(0, { int x, int y -> x + y })
> println(sum)
> }
what I would imagine here is that in def sum, the flow type for sum is
int and for list it is List<Integer>. That means sum+=i should change
the flow type internally to Integer or keep int. The result of the
inject should be Integer and it should always be allowed to assign it to
sum, even if not.
bye Jochen