[groovy] branch GROOVY_2_5_X updated (e48ca47 -> 1af7790)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[groovy] branch GROOVY_2_5_X updated (e48ca47 -> 1af7790)

paulk
This is an automated email from the ASF dual-hosted git repository.

paulk pushed a change to branch GROOVY_2_5_X
in repository https://gitbox.apache.org/repos/asf/groovy.git.


    from e48ca47  GROOVY-9758: groovsh import of non existing class throws null pointer instead of syntax error (avoid NPE)
     new 6532318  GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 2_5_X)
     new c8560a7  GROOVY-9093: fix existing tests (port to 2_5_X)
     new 1af7790  extract method for property access error and remove duplicate test cases (port to 2_5_X)

The 3 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails.  The revisions
listed as "add" were already present in the repository and have only
been added to this reference.


Summary of changes:
 .../classgen/asm/sc/StaticTypesCallSiteWriter.java | 70 +++++++++-------
 src/test/groovy/bugs/Groovy7165.groovy             | 97 ++++++++++++++++++++++
 .../asm/sc/StaticCompileFieldAccessTest.groovy     | 51 +-----------
 .../packageScope/DifferentPackageTest.groovy       | 61 +++++++-------
 4 files changed, 166 insertions(+), 113 deletions(-)
 create mode 100644 src/test/groovy/bugs/Groovy7165.groovy

Reply | Threaded
Open this post in threaded view
|

[groovy] 01/03: GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 2_5_X)

paulk
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

commit 65323180504cba18d50e5474f6496a810641ba66
Author: Eric Milles <[hidden email]>
AuthorDate: Fri Oct 23 20:53:03 2020 -0500

    GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 2_5_X)
---
 .../classgen/asm/sc/StaticTypesCallSiteWriter.java | 35 +++++---
 src/test/groovy/bugs/Groovy7165.groovy             | 97 ++++++++++++++++++++++
 2 files changed, 121 insertions(+), 11 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
index 575a365..ac6862c 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
@@ -81,6 +81,9 @@ import static org.codehaus.groovy.ast.ClassHelper.getWrapper;
 import static org.codehaus.groovy.ast.ClassHelper.int_TYPE;
 import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType;
 import static org.codehaus.groovy.ast.ClassHelper.make;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
 import static org.codehaus.groovy.classgen.AsmClassGenerator.samePackages;
 import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.chooseBestMethod;
 import static org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.findDGMMethodsByNameAndArguments;
@@ -451,13 +454,13 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             receiverType = classNode;
         }
         
-        String property = methodName;
+        String propertyName = methodName;
         if (implicitThis) {
             if (controller.getInvocationWriter() instanceof StaticInvocationWriter) {
                 MethodCallExpression currentCall = ((StaticInvocationWriter) controller.getInvocationWriter()).getCurrentCall();
                 if (currentCall != null && currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER) != null) {
-                    property = currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER);
-                    String[] props = property.split("\\.");
+                    propertyName = currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER);
+                    String[] props = propertyName.split("\\.");
                     BytecodeExpression thisLoader = new BytecodeExpression() {
                         @Override
                         public void visit(final MethodVisitor mv) {
@@ -477,15 +480,16 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             }
         }
 
-        if (makeGetPropertyWithGetter(receiver, receiverType, property, safe, implicitThis)) return;
-        if (makeGetPrivateFieldWithBridgeMethod(receiver, receiverType, property, safe, implicitThis)) return;
-        if (makeGetField(receiver, receiverType, property, safe, implicitThis)) return;
+        if (makeGetPropertyWithGetter(receiver, receiverType, propertyName, safe, implicitThis)) return;
+        if (makeGetPrivateFieldWithBridgeMethod(receiver, receiverType, propertyName, safe, implicitThis)) return;
+        if (makeGetField(receiver, receiverType, propertyName, safe, implicitThis)) return;
 
-        MethodCallExpression call = new MethodCallExpression(
-                receiver,
-                "getProperty",
-                new ArgumentListExpression(new ConstantExpression(property))
-        );
+        boolean isScriptVariable = (receiverType.isScript() && receiver instanceof VariableExpression && ((VariableExpression) receiver).getAccessedVariable() == null);
+        if (!isScriptVariable && controller.getClassNode().getOuterClass() == null) { // inner class still needs dynamic property sequence
+            addPropertyAccessError(receiver, propertyName, receiverType);
+        }
+
+        MethodCallExpression call = callX(receiver, "getProperty", args(constX(propertyName)));
         call.setImplicitThis(implicitThis);
         call.setSafe(safe);
         call.setMethodTarget(GROOVYOBJECT_GETPROPERTY_METHOD);
@@ -939,5 +943,14 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             return true;
         }
         return false;
+<<<<<<< HEAD
+=======
+    }*/
+
+    private void addPropertyAccessError(final Expression receiver, final String propertyName, final ClassNode receiverType) {
+        String receiverName = (receiver instanceof ClassExpression ? receiver.getType() : receiverType).toString(false);
+        String message = "Access to " + receiverName + "#" + propertyName + " is forbidden";
+        controller.getSourceUnit().addError(new SyntaxException(message, receiver));
+>>>>>>> c27b2c3e51... GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 3_0_X)
     }
 }
diff --git a/src/test/groovy/bugs/Groovy7165.groovy b/src/test/groovy/bugs/Groovy7165.groovy
new file mode 100644
index 0000000..5880057
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy7165.groovy
@@ -0,0 +1,97 @@
+/*
+ *  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 Groovy7165 {
+
+    @Test
+    void testPrivateStaticSuperField1() {
+        assertScript '''
+            import java.util.function.Function
+
+            @groovy.transform.CompileStatic
+            class Bug {
+                private static List<String> staticfield = []
+
+                def test() {
+                    List<Function<List<String>, String>> runners = []
+                    runners.add(new Called())
+                    List<String> results = ['hello']
+                    runners.each {
+                        results.addAll(it.apply(staticfield))
+                    }
+                    return results.join(' ')
+                }
+
+                static class Called implements Function<List<String>, String> {
+                    @Override
+                    String apply(List<String> args) {
+                        'world'
+                    }
+                }
+            }
+
+            def out = new Bug().test()
+            assert out == 'hello world'
+        '''
+    }
+
+    @Test
+    void testPrivateStaticSuperField2() {
+        def err = shouldFail '''
+            class A {
+                private static final String CONST = 'value'
+            }
+            class B extends A {
+                @groovy.transform.CompileStatic
+                def test() {
+                    return CONST // search excludes private members from supers
+                }
+            }
+            new B().test()
+        '''
+
+        assert err =~ /Access to B#CONST is forbidden/
+    }
+
+    @Test
+    void testPrivateStaticSuperField3() {
+        def err = shouldFail '''
+            class A {
+                private static final String CONST = 'value'
+            }
+            class B extends A {
+                @groovy.transform.CompileStatic
+                def test() {
+                    return A.CONST
+                }
+            }
+            assert false : 'compilation should fail'
+        '''
+
+        assert err =~ /Access to A#CONST is forbidden/
+    }
+}

Reply | Threaded
Open this post in threaded view
|

[groovy] 02/03: GROOVY-9093: fix existing tests (port to 2_5_X)

paulk
In reply to this post by paulk
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

commit c8560a7dee07ceef5c43ecb23fc2e73dbe9b0227
Author: Paul King <[hidden email]>
AuthorDate: Sat Nov 21 15:11:46 2020 +1000

    GROOVY-9093: fix existing tests (port to 2_5_X)
---
 .../packageScope/DifferentPackageTest.groovy       | 61 ++++++++++------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy b/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy
index 408955c..4030f62 100644
--- a/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy
@@ -18,7 +18,6 @@
  */
 package org.codehaus.groovy.transform.packageScope
 
-import groovy.transform.NotYetImplemented
 import org.codehaus.groovy.control.*
 import org.codehaus.groovy.tools.GroovyClass
 import org.junit.Test
@@ -187,48 +186,46 @@ final class DifferentPackageTest {
         assert loader.loadClass('p.Peer').half() == 21
     }
 
-    @Test @NotYetImplemented // GROOVY-9093
+    @Test
+    // GROOVY-9093
     void testDifferentPackageShouldNotSeeInstanceProps() {
         def err = shouldFail CompilationFailedException, {
-            def loader = addSources(
-                One: P_DOT_ONE,
-                Two: '''
-                    package q
-
-                    @groovy.transform.CompileStatic
-                    class Two extends p.One {
-                        int valueSize() {
-                            value.size() // not visible
+            addSources(
+                    One: P_DOT_ONE,
+                    Two: '''
+                        package q
+
+                        @groovy.transform.CompileStatic
+                        class Two extends p.One {
+                            int valueSize() {
+                                value.size() // not visible
+                            }
                         }
-                    }
-                ''')
-            // TODO: Don't need this once compiler errors
-            assert loader.loadClass('q.Two').newInstance().valueSize() == 5
+                    ''')
         }
 
-        assert err =~ / Access to ... value is forbidden /
+        assert err.message =~ /Access to q.Two#value is forbidden/
     }
 
-    @Test @NotYetImplemented // GROOVY-9093
+    @Test
+    // GROOVY-9093
     void testDifferentPackageShouldNotSeeStaticProps1() {
         def err = shouldFail CompilationFailedException, {
-            def loader = addSources(
-                One: P_DOT_ONE,
-                Two: '''
-                    package q
-
-                    @groovy.transform.CompileStatic
-                    class Two extends p.One {
-                        static def half() {
-                            (CONST / 2) // not visible
+            addSources(
+                    One: P_DOT_ONE,
+                    Two: '''
+                        package q
+
+                        @groovy.transform.CompileStatic
+                        class Two extends p.One {
+                            static def half() {
+                                (CONST / 2) // not visible
+                            }
                         }
-                    }
-                ''')
-            // TODO: Don't need this once compiler errors
-            assert loader.loadClass('q.Two').half() == 21
+                    ''')
         }
 
-        assert err =~ / Access to p.One#CONST is forbidden /
+        assert err.message =~ /Access to q.Two#CONST is forbidden/
     }
 
     @Test
@@ -250,6 +247,6 @@ final class DifferentPackageTest {
                 ''')
         }
 
-        assert err =~ / Access to p.One#CONST is forbidden /
+        assert err.message =~ /Access to p.One#CONST is forbidden/
     }
 }

Reply | Threaded
Open this post in threaded view
|

[groovy] 03/03: extract method for property access error and remove duplicate test cases (port to 2_5_X)

paulk
In reply to this post by paulk
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

commit 1af7790d8eb5c8d5576d9afc993b5dee1493228a
Author: Eric Milles <[hidden email]>
AuthorDate: Fri Oct 23 20:47:55 2020 -0500

    extract method for property access error and remove duplicate test cases (port to 2_5_X)
---
 .../classgen/asm/sc/StaticTypesCallSiteWriter.java | 43 ++++++++----------
 .../asm/sc/StaticCompileFieldAccessTest.groovy     | 51 +---------------------
 2 files changed, 20 insertions(+), 74 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
index ac6862c..82b8316 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
@@ -126,10 +126,10 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
     }
 
     @Override
-    public void makeGetPropertySite(Expression receiver, final String methodName, final boolean safe, final boolean implicitThis) {
+    public void makeGetPropertySite(Expression receiver, final String propertyName, final boolean safe, final boolean implicitThis) {
         Object dynamic = receiver.getNodeMetaData(StaticCompilationMetadataKeys.RECEIVER_OF_DYNAMIC_PROPERTY);
         if (dynamic !=null) {
-            makeDynamicGetProperty(receiver, methodName, safe);
+            makeDynamicGetProperty(receiver, propertyName, safe);
             return;
         }
         TypeChooser typeChooser = controller.getTypeChooser();
@@ -163,7 +163,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
 
         MethodVisitor mv = controller.getMethodVisitor();
 
-        if (receiverType.isArray() && methodName.equals("length")) {
+        if (receiverType.isArray() && propertyName.equals("length")) {
             receiver.visit(controller.getAcg());
             ClassNode arrayGetReturnType = typeChooser.resolveType(receiver, classNode);
             controller.getOperandStack().doGroovyCast(arrayGetReturnType);
@@ -172,7 +172,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             return;
         } else if (
                 (receiverType.implementsInterface(COLLECTION_TYPE)
-                        || COLLECTION_TYPE.equals(receiverType)) && ("size".equals(methodName) || "length".equals(methodName))) {
+                        || COLLECTION_TYPE.equals(receiverType)) && ("size".equals(propertyName) || "length".equals(propertyName))) {
             MethodCallExpression expr = new MethodCallExpression(
                     receiver,
                     "size",
@@ -190,26 +190,26 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
 
         if (!isStaticProperty && (receiverType.implementsInterface(MAP_TYPE) || MAP_TYPE.equals(receiverType))) {
             // for maps, replace map.foo with map.get('foo')
-            writeMapDotProperty(receiver, methodName, mv, safe);
+            writeMapDotProperty(receiver, propertyName, mv, safe);
             return;
         }
-        if (makeGetPropertyWithGetter(receiver, receiverType, methodName, safe, implicitThis)) return;
-        if (makeGetField(receiver, receiverType, methodName, safe, implicitThis)) return;
+        if (makeGetPropertyWithGetter(receiver, receiverType, propertyName, safe, implicitThis)) return;
+        if (makeGetField(receiver, receiverType, propertyName, safe, implicitThis)) return;
         if (receiver instanceof ClassExpression) {
-            if (makeGetField(receiver, receiver.getType(), methodName, safe, implicitThis)) return;
-            if (makeGetPropertyWithGetter(receiver, receiver.getType(), methodName, safe, implicitThis)) return;
-            if (makeGetPrivateFieldWithBridgeMethod(receiver, receiver.getType(), methodName, safe, implicitThis)) return;
+            if (makeGetField(receiver, receiver.getType(), propertyName, safe, implicitThis)) return;
+            if (makeGetPropertyWithGetter(receiver, receiver.getType(), propertyName, safe, implicitThis)) return;
+            if (makeGetPrivateFieldWithBridgeMethod(receiver, receiver.getType(), propertyName, safe, implicitThis)) return;
         }
         if (isClassReceiver) {
             // we are probably looking for a property of the class
-            if (makeGetPropertyWithGetter(receiver, CLASS_Type, methodName, safe, implicitThis)) return;
-            if (makeGetField(receiver, CLASS_Type, methodName, safe, false)) return;
+            if (makeGetPropertyWithGetter(receiver, CLASS_Type, propertyName, safe, implicitThis)) return;
+            if (makeGetField(receiver, CLASS_Type, propertyName, safe, false)) return;
         }
-        if (makeGetPrivateFieldWithBridgeMethod(receiver, receiverType, methodName, safe, implicitThis)) return;
+        if (makeGetPrivateFieldWithBridgeMethod(receiver, receiverType, propertyName, safe, implicitThis)) return;
 
         // GROOVY-5580, it is still possible that we're calling a superinterface property
-        String getterName = "get" + MetaClassHelper.capitalize(methodName);
-        String altGetterName = "is" + MetaClassHelper.capitalize(methodName);
+        String getterName = "get" + MetaClassHelper.capitalize(propertyName);
+        String altGetterName = "is" + MetaClassHelper.capitalize(propertyName);
         if (receiverType.isInterface()) {
             Set<ClassNode> allInterfaces = receiverType.getAllInterfaces();
             MethodNode getterMethod = null;
@@ -263,15 +263,11 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
         }
 
         if (!isStaticProperty && (receiverType.implementsInterface(LIST_TYPE) || LIST_TYPE.equals(receiverType))) {
-            writeListDotProperty(receiver, methodName, mv, safe);
+            writeListDotProperty(receiver, propertyName, mv, safe);
             return;
         }
 
-        controller.getSourceUnit().addError(
-                new SyntaxException("Access to "+
-                                                (receiver instanceof ClassExpression ?receiver.getType():receiverType).toString(false)
-                                                +"#"+methodName+" is forbidden", receiver.getLineNumber(), receiver.getColumnNumber(), receiver.getLastLineNumber(), receiver.getLastColumnNumber())
-        );
+        addPropertyAccessError(receiver, propertyName, receiverType);
         controller.getMethodVisitor().visitInsn(ACONST_NULL);
         controller.getOperandStack().push(OBJECT_TYPE);
     }
@@ -943,14 +939,11 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             return true;
         }
         return false;
-<<<<<<< HEAD
-=======
-    }*/
+    }
 
     private void addPropertyAccessError(final Expression receiver, final String propertyName, final ClassNode receiverType) {
         String receiverName = (receiver instanceof ClassExpression ? receiver.getType() : receiverType).toString(false);
         String message = "Access to " + receiverName + "#" + propertyName + " is forbidden";
         controller.getSourceUnit().addError(new SyntaxException(message, receiver));
->>>>>>> c27b2c3e51... GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 3_0_X)
     }
 }
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFieldAccessTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFieldAccessTest.groovy
index 15feadd..d18d9d3 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFieldAccessTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFieldAccessTest.groovy
@@ -23,7 +23,8 @@ import org.codehaus.groovy.classgen.asm.AbstractBytecodeTestCase
 /**
  * Unit tests for static compilation: access field nodes.
  */
-class StaticCompileFieldAccessTest extends AbstractBytecodeTestCase {
+final class StaticCompileFieldAccessTest extends AbstractBytecodeTestCase {
+
     void testAccessProperty() {
         compile(method:'m', '''
             class A {
@@ -126,26 +127,6 @@ class StaticCompileFieldAccessTest extends AbstractBytecodeTestCase {
         clazz.newInstance().run()
     }
 
-    void testReturnProtectedFieldInDifferentPackage() {
-        compile(method:'m', '''
-            import org.codehaus.groovy.classgen.asm.sc.StaticCompileFieldAccessTest.StaticCompileFieldAccessSupport1 as A
-
-            A a = new A()
-            @groovy.transform.CompileStatic
-            int m(A a) {
-                return a.x
-            }
-            assert m(a) == 10
-        ''')
-        assert sequence.hasStrictSequence([
-                'ALOAD',
-                'LDC "x"',
-                'INVOKEINTERFACE groovy/lang/GroovyObject.getProperty (Ljava/lang/String;)Ljava/lang/Object;'
-        ])
-
-        clazz.newInstance().run()
-    }
-
     void testReturnPublicFieldFromNonGroovyObject() {
         compile(method:'m', '''
             java.awt.Point a = [100,200]
@@ -215,32 +196,4 @@ class StaticCompileFieldAccessTest extends AbstractBytecodeTestCase {
             ''')
         }
     }
-
-    /*
-     * this test should fail, but passes because it generates ScriptBytecodeAdapter.getGroovyObjectField
-     * instead of direct field access
-    void testShouldFailToReturnPrivateField() {
-        shouldFail {
-            compile(method: 'm', '''
-            package test
-
-            class A {
-                int x = 10
-            }
-
-            A a = new A()
-            @groovy.transform.CompileStatic
-            int m(A a) {
-                return a.@x // x is a private member, direct access should not be allowed
-            }
-            assert m(a) == 10
-        ''')
-            println sequence
-        }
-    }*/
-
-    public static class StaticCompileFieldAccessSupport1 {
-        protected int x = 10
-    }
-
 }