From commits-return-9859-archive-asf-public=cust-asf.ponee.io@groovy.apache.org Sat Nov 16 13:34:57 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id AA1FF180648 for ; Sat, 16 Nov 2019 14:34:56 +0100 (CET) Received: (qmail 67974 invoked by uid 500); 16 Nov 2019 13:34:55 -0000 Mailing-List: contact commits-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 commits@groovy.apache.org Received: (qmail 67965 invoked by uid 99); 16 Nov 2019 13:34:55 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 16 Nov 2019 13:34:55 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 959B98B688; Sat, 16 Nov 2019 13:34:55 +0000 (UTC) Date: Sat, 16 Nov 2019 13:34:55 +0000 To: "commits@groovy.apache.org" Subject: [groovy] branch GROOVY_2_5_X updated: GROOVY-7996: check for get(String)/set(String, Object) or propertyMissing MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <157391129545.28049.5830591854746188624@gitbox.apache.org> From: paulk@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: groovy X-Git-Refname: refs/heads/GROOVY_2_5_X X-Git-Reftype: branch X-Git-Oldrev: 59c782b56dc60929aab8b2be3a37cbaff52e146c X-Git-Newrev: cd0ca19b4b99cd859bc28cf48944a8aa69d69fa4 X-Git-Rev: cd0ca19b4b99cd859bc28cf48944a8aa69d69fa4 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. paulk pushed a commit to branch GROOVY_2_5_X in repository https://gitbox.apache.org/repos/asf/groovy.git The following commit(s) were added to refs/heads/GROOVY_2_5_X by this push: new cd0ca19 GROOVY-7996: check for get(String)/set(String,Object) or propertyMissing cd0ca19 is described below commit cd0ca19b4b99cd859bc28cf48944a8aa69d69fa4 Author: Eric Milles AuthorDate: Fri Nov 15 14:03:13 2019 -0600 GROOVY-7996: check for get(String)/set(String,Object) or propertyMissing roll back initial changes (Groovy 2.5 backport) --- .../transform/stc/StaticTypeCheckingVisitor.java | 50 +++--- src/test/groovy/bugs/Groovy7996.groovy | 187 +++++++++++++++++++++ src/test/groovy/bugs/Groovy7996Bug.groovy | 88 ---------- 3 files changed, 213 insertions(+), 112 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java index 1bfc997..eab3714 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -513,21 +513,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } - /** - * Checks valid cases for accessing a field from an inner class. - */ - private String checkOrMarkInnerPropertyOwnerAccess(PropertyExpression source, boolean lhsOfAssignment, String delegationData) { - // check for reference to method, closure, for loop, try with, or catch block parameter from a non-nested closure - if (typeCheckingContext.getEnclosingClosureStack().size() == 1 && !"this".equals(source.getPropertyAsString())) { - if (!(source.getObjectExpression() instanceof VariableExpression && - ((VariableExpression) source.getObjectExpression()).getAccessedVariable() instanceof Parameter)) { - delegationData = "owner"; - source.getObjectExpression().putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData); - } - } - return delegationData; - } - private MethodNode findValidGetter(ClassNode classNode, String name) { MethodNode getterMethod = classNode.getGetterMethod(name); if (getterMethod != null && (getterMethod.isPublic() || getterMethod.isProtected())) { @@ -1571,7 +1556,8 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { queue.add(current.getUnresolvedSuperClass()); } } - // GROOVY-5568, the property may be defined by DGM + + // GROOVY-5568: the property may be defined by DGM List dgmReceivers = new ArrayList(2); dgmReceivers.add(testClass); if (isPrimitiveType(testClass)) dgmReceivers.add(getWrapper(testClass)); @@ -1594,6 +1580,25 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } } + + // GROOVY-7996: check if receiver implements get(String)/set(String,Object) or propertyMissing(String) + if (!testClass.isArray() && !isPrimitiveType(getUnwrapper(testClass)) + && pexp.isImplicitThis() && typeCheckingContext.getEnclosingClosure() != null) { + MethodNode mopMethod; + if (readMode) { + mopMethod = testClass.getMethod("get", new Parameter[]{new Parameter(STRING_TYPE, "name")}); + } else { + mopMethod = testClass.getMethod("set", new Parameter[]{new Parameter(STRING_TYPE, "name"), new Parameter(OBJECT_TYPE, "value")}); + } + if (mopMethod == null) mopMethod = testClass.getMethod("propertyMissing", new Parameter[]{new Parameter(STRING_TYPE, "propertyName")}); + + if (mopMethod != null) { + pexp.putNodeMetaData(StaticTypesMarker.DYNAMIC_RESOLUTION, Boolean.TRUE); + pexp.removeNodeMetaData(StaticTypesMarker.DECLARATION_INFERRED_TYPE); + pexp.removeNodeMetaData(StaticTypesMarker.INFERRED_TYPE); + return true; + } + } } for (Receiver receiver : receivers) { @@ -1738,21 +1743,18 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { private boolean storeField(FieldNode field, boolean returnTrueIfFieldExists, PropertyExpression expressionToStoreOn, ClassNode receiver, ClassCodeVisitorSupport visitor, String delegationData, boolean lhsOfAssignment) { if (field == null || !returnTrueIfFieldExists) return false; if (visitor != null) visitor.visitField(field); - storeWithResolve(field.getOriginType(), receiver, field.getDeclaringClass(), field.isStatic(), expressionToStoreOn); checkOrMarkPrivateAccess(expressionToStoreOn, field, lhsOfAssignment); - if (field != null && !field.isStatic() && !field.isPrivate() && !"delegate".equals(delegationData)) { - delegationData = checkOrMarkInnerPropertyOwnerAccess(expressionToStoreOn, lhsOfAssignment, delegationData); - } + storeWithResolve(field.getOriginType(), receiver, field.getDeclaringClass(), field.isStatic(), expressionToStoreOn); if (delegationData != null) { expressionToStoreOn.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData); } return true; } - private boolean storeProperty(PropertyNode propertyNode, PropertyExpression expressionToStoreOn, ClassNode receiver, ClassCodeVisitorSupport visitor, String delegationData) { - if (propertyNode == null) return false; - if (visitor != null) visitor.visitProperty(propertyNode); - storeWithResolve(propertyNode.getOriginType(), receiver, propertyNode.getDeclaringClass(), propertyNode.isStatic(), expressionToStoreOn); + private boolean storeProperty(PropertyNode property, PropertyExpression expressionToStoreOn, ClassNode receiver, ClassCodeVisitorSupport visitor, String delegationData) { + if (property == null) return false; + if (visitor != null) visitor.visitProperty(property); + storeWithResolve(property.getOriginType(), receiver, property.getDeclaringClass(), property.isStatic(), expressionToStoreOn); if (delegationData != null) { expressionToStoreOn.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData); } diff --git a/src/test/groovy/bugs/Groovy7996.groovy b/src/test/groovy/bugs/Groovy7996.groovy new file mode 100644 index 0000000..39de261 --- /dev/null +++ b/src/test/groovy/bugs/Groovy7996.groovy @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy.bugs + +import groovy.transform.CompileStatic +import org.junit.Test + +import static groovy.test.GroovyAssert.assertScript +import static groovy.test.GroovyAssert.shouldFail + +@CompileStatic +final class Groovy7996 { + + @Test + void testFieldAccessFromClosure1() { + def err = shouldFail ''' + class Foo { + def build(@DelegatesTo(value=Foo, strategy=Closure.DELEGATE_FIRST) Closure block) { + this.with(block) + } + + def propertyMissing(String name) { + 'whatever' + } + } + + @groovy.transform.CompileStatic + class Bar { + protected List bar = [] + + boolean doStuff() { + Foo foo = new Foo() + foo.build { + bar.isEmpty() // ClassCastException: java.lang.String cannot be cast to java.util.List + } + } + } + + new Bar().doStuff() + ''' + + assert err =~ /Cannot find matching method java.lang.Object#isEmpty\(\)/ + } + + @Test + void testFieldAccessFromClosure2() { + assertScript ''' + class Foo { + def build(@DelegatesTo(value=Foo, strategy=Closure.DELEGATE_FIRST) Closure block) { + this.with(block) + } + + def propertyMissing(String name) { + 'whatever' + } + } + + @groovy.transform.CompileStatic + class Bar { + protected List bar = [] + + boolean doStuff() { + Foo foo = new Foo() + foo.build { + owner.bar.isEmpty() + } + } + } + + assert new Bar().doStuff() + ''' + } + + @Test + void testFieldAccessFromClosure3() { + assertScript ''' + class Foo { + def build(@DelegatesTo(value=Foo, strategy=Closure.DELEGATE_FIRST) Closure block) { + this.with(block) + } + + def propertyMissing(String name) { + 'whatever' + } + } + + @groovy.transform.CompileStatic + class Bar { + protected List bar = [] + + boolean doStuff() { + Foo foo = new Foo() + foo.build { + thisObject.bar.isEmpty() + } + } + } + + assert new Bar().doStuff() + ''' + } + + @Test + void testFieldAccessFromClosure4() { + assertScript ''' + class Foo { + def build(@DelegatesTo(value=Foo, strategy=Closure.OWNER_FIRST) Closure block) { + block.delegate = this + return block.call() + } + + def propertyMissing(String name) { + 'whatever' + } + } + + @groovy.transform.CompileStatic + class Bar { + protected List bar = [] + + boolean doStuff() { + Foo foo = new Foo() + foo.build { + bar.isEmpty() + } + } + } + + assert new Bar().doStuff() + ''' + } + + @Test // GROOVY-7687 + void testFieldAccessFromNestedClosure1() { + assertScript ''' + @groovy.transform.CompileStatic + class BugTest { + static class Foo { + public List messages = ['hello', 'world'] + } + + void interactions(Foo foo, @DelegatesTo(Foo) Closure closure) { + closure.delegate = foo + closure() + } + + void execute() { + interactions(new Foo()) { + messages.each { it.contains('o') } + } + } + } + new BugTest().execute() + ''' + } + + @Test // GROOVY-8073 + void testDelegatePropertyAccessFromClosure() { + assertScript ''' + @groovy.transform.CompileStatic + class Main { + static main(args) { + def map = [a: 1, b: 2] + map.with { + assert a == 1 + } + } + } + ''' + } +} diff --git a/src/test/groovy/bugs/Groovy7996Bug.groovy b/src/test/groovy/bugs/Groovy7996Bug.groovy deleted file mode 100644 index cc6a3d0..0000000 --- a/src/test/groovy/bugs/Groovy7996Bug.groovy +++ /dev/null @@ -1,88 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package groovy.bugs - -class Groovy7996Bug extends GroovyTestCase { - void testPropertyAccessFromInnerClass() { - assertScript ''' - class Foo7996 { - Object propertyMissing(String name) { - return "stuff" - } - - def build(Closure callable) { - this.with(callable) - } - } - - @groovy.transform.CompileStatic - class Bar7996 { - protected List bar = [] - - boolean doStuff() { - Foo7996 foo = new Foo7996() - foo.build { - bar.isEmpty() - } - } - } - - assert new Bar7996().doStuff() - ''' - } - - // GROOVY-7687 - void testCompileStaticWithNestedClosuresBug() { - assertScript ''' - @groovy.transform.CompileStatic - class BugTest { - static class Foo { - public List messages = Arrays.asList("hello", "world") - } - - void interactions(Foo foo, @DelegatesTo(Foo) Closure closure) { - closure.delegate = foo - closure() - } - - void execute() { - interactions(new Foo()) { - messages.each{ it.contains('o') } - } - } - } - new BugTest().execute() - ''' - } - - // GROOVY-8073 - void testCompileStaticMapInsideWithBug() { - assertScript ''' - @groovy.transform.CompileStatic - class Main { - static void main(String[] args) { - def map = [a: 1, b: 2] - map.with { - assert a == 1 - } - } - } - ''' - } -}