Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 4A02D200C4F for ; Sat, 1 Apr 2017 20:04:28 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 48526160B9D; Sat, 1 Apr 2017 18:04:28 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 3A476160B78 for ; Sat, 1 Apr 2017 20:04:27 +0200 (CEST) Received: (qmail 72556 invoked by uid 500); 1 Apr 2017 18:04:26 -0000 Mailing-List: contact dev-help@groovy.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@groovy.apache.org Delivered-To: mailing list dev@groovy.apache.org Received: (qmail 72546 invoked by uid 99); 1 Apr 2017 18:04:26 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd3-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 01 Apr 2017 18:04:26 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd3-us-west.apache.org (ASF Mail Server at spamd3-us-west.apache.org) with ESMTP id BAA6D1809FE for ; Sat, 1 Apr 2017 18:04:25 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd3-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: -0.646 X-Spam-Level: X-Spam-Status: No, score=-0.646 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, KAM_LOTSOFHASH=0.25, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-2.796, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd3-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd3-us-west.apache.org [10.40.0.10]) (amavisd-new, port 10024) with ESMTP id 6yNCtpRuvm0i for ; Sat, 1 Apr 2017 18:04:20 +0000 (UTC) Received: from mail-ua0-f171.google.com (mail-ua0-f171.google.com [209.85.217.171]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTPS id D20F05F1A0 for ; Sat, 1 Apr 2017 18:04:19 +0000 (UTC) Received: by mail-ua0-f171.google.com with SMTP id d64so10321412uad.2 for ; Sat, 01 Apr 2017 11:04:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=/UjwsUadqnqcWdhtJsgnb+W9pERfE3td/Jw0vz2DZsY=; b=Se06LqJLzREM0ntVY6ymTmpRxs2RFqBTZ0oQyxhjsgQn41KfcUJOFHTS842/mjfsYf UOFfz+X0kcK0PBfINDjWVgMWrNYeA/Pv2wMidbPLhO0JKlMWcC/AlzhZ8KD42P/9dpri FBxrFBbRFbjkR3GSoCSKimejPYZseZhdnuY44yKyStpHq+1tWn0qKRZubn6+dSUHhKC7 8x+fNS95Ap0ueJU2+wfX3uKQyMXYInPCQR3zxMAoUtZxEa57D64FLT7kOSh/Ib0o1kpp 5/h99enGAPnRt+Zz0XU3OYh9Zt5Xn4L5/h6tDd2BJeWf6q1WcccJdK7NbOIIdkxOG5W4 T5bg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=/UjwsUadqnqcWdhtJsgnb+W9pERfE3td/Jw0vz2DZsY=; b=Se5oWQ6WoBvALaTpoOfEvUa9EebHMoqxLCREYTaSuGS43JbMTX04DPVVYdLNCvtLki QO5nwjOdbDeKHWaTgdAntyqayPd6nvmThuUVliRqPwXMQy5dtVUB3FKNJImiWEi/D5qU qmW5DWySwgCxdgAzpr6troRUjUIeMjMILkrtnYC+V9utdFZylZS8neZrvRDYIbmSRNTR vWxu02VTYoDsCqMP+BAD1SXZiUBYVX2c8xadmEGaZZfr4u9FYggtp7dF8WrFkPjwHgVU mTCG9j6jJK4cbfS3weto/PUCaOXiQOXHWutXdZwXXqz1Ure6UebqBVHkZC5Tr1UH73Jj XQCQ== X-Gm-Message-State: AFeK/H14VoMxmRtNlqGvbzE1ooibmDUwi8PcIkvyLNE2mGeW4Zt/9u3g0zXhZqy4BbHvvTY22qhFPDv28AOIzg== X-Received: by 10.176.24.102 with SMTP id j38mr3842061uag.112.1491069859169; Sat, 01 Apr 2017 11:04:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.176.92.194 with HTTP; Sat, 1 Apr 2017 11:04:18 -0700 (PDT) In-Reply-To: References: <58DE0FA7.5090206@gmx.org> From: John Wagenleitner Date: Sat, 1 Apr 2017 11:04:18 -0700 Message-ID: Subject: Re: Trying to determine if a change in 2.4.8 was deliberate... To: dev@groovy.apache.org Content-Type: multipart/alternative; boundary=f403043c3ef84a8e96054c1ec1ae archived-at: Sat, 01 Apr 2017 18:04:28 -0000 --f403043c3ef84a8e96054c1ec1ae Content-Type: text/plain; charset=UTF-8 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 wrote: > I'll open a JIRA in a bit. Thanks! > > A. > > On Fri, Mar 31, 2017 at 1:13 AM Jochen Theodorou > 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 >> >> --f403043c3ef84a8e96054c1ec1ae Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
I remember looking at this and seeing that the commit that= originally introduced the check was 2f077109 [1].=C2=A0 Subsequent commits= switched to CachedClass and later back to Class for the argument type to t= hat method but things got out of sync between MethodIndexAction and the loc= al MOPIter class leaving the method unused which looked like a regression (= albeit way back in 1.8) and not intentional.

I can rever= t the commit or maybe just remove the skipClass method altogether from MOPI= ter.

On Fri, Mar 31, 2017 at 3:23 AM, Andrew Bayer <andre= w.bayer@gmail.com> wrote:
<= div>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 on= e
> of them easily enough (GlobalClassSet switched to a new collection cla= ss
> for its items field), but the other is hairier. The change in question=
> is
> https://github.com/apache/groovy/commit/0a6789d06cc6451fcfee174ba638c0494f2827ef,
> which went into 2.4.8.=C2=A0 What I'm seeing is that
> https:/= /github.com/cloudbees/groovy-cps/blob/master/src/main/java/com/cl= oudbees/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 ou= r
> transformed code in the inheritor was invoked here. In our CPS context= s,
> that CpsCallableInvocation is the right result, but not here.
>
> Note that this isn't relevant in any other Groovy use case that I&= #39;m
> aware of - it's particular to this usage. Or rather, this is the o= nly
> case I can find where the end result is different. I eventually found<= br class=3D"m_1516887079893253341gmail_msg"> > the specific internal behavior that changed, getting us a different > method to invoke -
> https://github.com/cloudbees/groovy-cps/pull/24#i= ssuecomment-290250394
> has a test case that passes in 2.4.7 and earlier but fails in 2.4.8 an= d
> 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/340ca45ab95af63302bfa7648a9c= 79aa3f874fa4#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<= br class=3D"m_1516887079893253341gmail_msg"> > effect, or a bug? Any thoughts would be much appreciated. =3D)

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


--f403043c3ef84a8e96054c1ec1ae--