Re: Trying to determine if a change in 2.4.8 was deliberate...

[John Wagenleitner <jo...@gmail.com>]

I remember looking at this and seeing that the commit that originally
introduced the check was 2f077109 [1].  Subsequent commits switched to
CachedClass and later back to Class for the argument type to that method
but things got out of sync between MethodIndexAction and the local MOPIter
class leaving the method unused which looked like a regression (albeit way
back in 1.8) and not intentional.

I can revert the commit or maybe just remove the skipClass method
altogether from MOPIter.

[1]
https://github.com/apache/groovy/commit/2f077109#diff-c1354c0d7df171ad8cafcb3d1bf63857R223

On Fri, Mar 31, 2017 at 3:23 AM, Andrew Bayer <an...@gmail.com>
wrote:

> I'll open a JIRA in a bit. Thanks!
>
> A.
>
> On Fri, Mar 31, 2017 at 1:13 AM Jochen Theodorou <bl...@gmx.org>
> wrote:
>
>> On 30.03.2017 19:16, Andrew Bayer wrote:
>> > I'm working on getting Jenkins' groovy-cps transforming magic working
>> > with 2.5.0-alpha-1-SNAPSHOT and hit a few problems. I worked around one
>> > of them easily enough (GlobalClassSet switched to a new collection class
>> > for its items field), but the other is hairier. The change in question
>> > is
>> > https://github.com/apache/groovy/commit/0a6789d06cc6451fcfee174ba638c0
>> 494f2827ef,
>> > which went into 2.4.8.  What I'm seeing is that
>> > https://github.com/cloudbees/groovy-cps/blob/master/src/
>> main/java/com/cloudbees/groovy/cps/sandbox/DefaultInvoker.java#L32
>> > is invoking the super's method through the inheritor. As far as I can
>> > tell - we end up with a CpsCallableInvocation being thrown, meaning our
>> > transformed code in the inheritor was invoked here. In our CPS contexts,
>> > that CpsCallableInvocation is the right result, but not here.
>> >
>> > Note that this isn't relevant in any other Groovy use case that I'm
>> > aware of - it's particular to this usage. Or rather, this is the only
>> > case I can find where the end result is different. I eventually found
>> > the specific internal behavior that changed, getting us a different
>> > method to invoke -
>> > https://github.com/cloudbees/groovy-cps/pull/24#issuecomment-290250394
>> > has a test case that passes in 2.4.7 and earlier but fails in 2.4.8 and
>> > later.
>>
>> The example can be fixed by using A instead of B. But that looks wrong
>> to me. I think we have to restore the old behaviour.
>>
>> > I do have what *seems* to be a workaround at
>> > https://github.com/abayer/groovy-cps/commit/
>> 340ca45ab95af63302bfa7648a9c79aa3f874fa4#diff-
>> 31c385af329657115ee24713b9ba928c
>> > but that feels dirty to me. Is there a better way to do this? Was this
>> > an intended result of the change, an unintentional but desirable side
>> > effect, or a bug? Any thoughts would be much appreciated. =)
>>
>> I think this was not really intended, no. Maybe the fix means a piece of
>> code outlifed itself and we did not notice, reactivating it is not right.
>>
>> For me this is a critical bug
>>
>> bye Jochen
>>
>>

Re: Trying to determine if a change in 2.4.8 was deliberate...

[John Wagenleitner <jo...@gmail.com>]

I created https://issues.apache.org/jira/browse/GROOVY-8140 for this and
will remove the method.

On Sat, Apr 1, 2017 at 2:14 PM, Jochen Theodorou <bl...@gmx.org> wrote:

> On 01.04.2017 20:04, John Wagenleitner wrote:
>
>> I remember looking at this and seeing that the commit that originally
>> introduced the check was 2f077109 [1].  Subsequent commits switched to
>> CachedClass and later back to Class for the argument type to that method
>> but things got out of sync between MethodIndexAction and the local
>> MOPIter class leaving the method unused which looked like a regression
>> (albeit way back in 1.8) and not intentional.
>>
>> I can revert the commit or maybe just remove the skipClass method
>> altogether from MOPIter.
>>
>> [1]
>> https://github.com/apache/groovy/commit/2f077109#diff-c1354c
>> 0d7df171ad8cafcb3d1bf63857R223
>>
>
> It is very possible that changes back then introduced something that was
> not intended. But we did built on top of it. I think removing the skipClass
> method would be best.
>
> bye Jochen
>

Re: Trying to determine if a change in 2.4.8 was deliberate...

[Jochen Theodorou <bl...@gmx.org>]

On 01.04.2017 20:04, John Wagenleitner wrote:
> I remember looking at this and seeing that the commit that originally
> introduced the check was 2f077109 [1].  Subsequent commits switched to
> CachedClass and later back to Class for the argument type to that method
> but things got out of sync between MethodIndexAction and the local
> MOPIter class leaving the method unused which looked like a regression
> (albeit way back in 1.8) and not intentional.
>
> I can revert the commit or maybe just remove the skipClass method
> altogether from MOPIter.
>
> [1]
> https://github.com/apache/groovy/commit/2f077109#diff-c1354c0d7df171ad8cafcb3d1bf63857R223

It is very possible that changes back then introduced something that was 
not intended. But we did built on top of it. I think removing the 
skipClass method would be best.

bye Jochen