groovy-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From John Wagenleitner <john.wagenleit...@gmail.com>
Subject Re: Trying to determine if a change in 2.4.8 was deliberate...
Date Sat, 01 Apr 2017 18:04:18 GMT
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 <andrew.bayer@gmail.com>
wrote:

> I'll open a JIRA in a bit. Thanks!
>
> A.
>
> On Fri, Mar 31, 2017 at 1:13 AM Jochen Theodorou <blackdrag@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
>>
>>

Mime
View raw message