[groovy] branch GROOVY_3_0_X updated (7e71224 -> 26505bd)

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

[groovy] branch GROOVY_3_0_X updated (7e71224 -> 26505bd)

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

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


    from 7e71224  fix moved maven coordinate
     new c27b2c3  GROOVY-9093: SC: add compile-time error for inaccessible field or getter (port to 3_0_X)
     new 87bc216  GROOVY-9093: fix existing tests
     new 26505bd  extract method for property access error and remove duplicate test cases (port to 3_0_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 | 46 ++++++++++-------
 src/test/groovy/bugs/Groovy7165.groovy             |  2 +-
 .../asm/sc/StaticCompileFieldAccessTest.groovy     | 51 +-----------------
 .../packageScope/DifferentPackageTest.groovy       | 60 +++++++++++-----------
 4 files changed, 60 insertions(+), 99 deletions(-)

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 3_0_X)

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

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

commit c27b2c3e5169ca5afd89ffbb261757e345be31c4
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 3_0_X)
---
 .../groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java   | 13 ++++++++++++-
 src/test/groovy/bugs/Groovy7165.groovy                      |  2 +-
 2 files changed, 13 insertions(+), 2 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 c4adf83..4cedc74 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
@@ -453,7 +453,12 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
         if (makeGetPrivateFieldWithBridgeMethod(receiver, receiverType, property, safe, implicitThis)) return;
         if (makeGetField(receiver, receiverType, property, safe, implicitThis)) return;
 
-        MethodCallExpression call = callX(receiver, "getProperty", args(constX(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.setMethodTarget(GROOVYOBJECT_GETPROPERTY_METHOD);
         call.setSafe(safe);
@@ -876,4 +881,10 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
         }
         return false;
     }*/
+
+    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));
+    }
 }
diff --git a/src/test/groovy/bugs/Groovy7165.groovy b/src/test/groovy/bugs/Groovy7165.groovy
index 4ee9a12..5880057 100644
--- a/src/test/groovy/bugs/Groovy7165.groovy
+++ b/src/test/groovy/bugs/Groovy7165.groovy
@@ -74,7 +74,7 @@ final class Groovy7165 {
             new B().test()
         '''
 
-        assert err =~ /MissingPropertyException: No such property: CONST for class: B/
+        assert err =~ /Access to B#CONST is forbidden/
     }
 
     @Test

Reply | Threaded
Open this post in threaded view
|

[groovy] 02/03: GROOVY-9093: fix existing tests

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_3_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git

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

    GROOVY-9093: fix existing tests
---
 .../packageScope/DifferentPackageTest.groovy       | 60 +++++++++++-----------
 1 file changed, 29 insertions(+), 31 deletions(-)

diff --git a/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy b/src/test/org/codehaus/groovy/transform/packageScope/DifferentPackageTest.groovy
index 42888eb..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.test.NotYetImplemented
 import org.codehaus.groovy.control.*
 import org.codehaus.groovy.tools.GroovyClass
 import org.junit.Test
@@ -187,47 +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
+                            }
                         }
-                    }
-                ''')
-            loader.loadClass('q.Two').half()
+                    ''')
         }
 
-        assert err =~ / MissingPropertyException: No such property: CONST for class: q.Two /
+        assert err.message =~ /Access to q.Two#CONST is forbidden/
     }
 
     @Test
@@ -249,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 3_0_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_3_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit 26505bd92a84a90874672dd8bf4d37665dc794af
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 3_0_X)
---
 .../classgen/asm/sc/StaticTypesCallSiteWriter.java | 33 +++++++-------
 .../asm/sc/StaticCompileFieldAccessTest.groovy     | 51 +---------------------
 2 files changed, 18 insertions(+), 66 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 4cedc74..c94dd4f 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
@@ -256,9 +256,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             return;
         }
 
-        String receiverName = (receiver instanceof ClassExpression ? receiver.getType() : receiverType).toString(false);
-        controller.getSourceUnit().addError(
-                new SyntaxException("Access to " + receiverName + "#" + propertyName + " is forbidden", receiver));
+        addPropertyAccessError(receiver, propertyName, receiverType);
         controller.getMethodVisitor().visitInsn(ACONST_NULL);
         controller.getOperandStack().push(OBJECT_TYPE);
     }
@@ -432,26 +430,27 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
             receiverType = controller.getTypeChooser().resolveType(receiver, receiverType);
         }
 
-        String property = propertyName;
         if (implicitThis && controller.getInvocationWriter() instanceof StaticInvocationWriter) {
             Expression currentCall = ((StaticInvocationWriter) controller.getInvocationWriter()).getCurrentCall();
-            if (currentCall != null && currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER) != null) {
-                property = currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER);
-                String[] props = property.split("\\.");
-                BytecodeExpression thisLoader = bytecodeX(CLOSURE_TYPE, mv -> mv.visitVarInsn(ALOAD, 0));
-                PropertyExpression pexp = propX(thisLoader, constX(props[0]), safe);
-                for (int i = 1, n = props.length; i < n; i += 1) {
-                    pexp.putNodeMetaData(StaticTypesMarker.INFERRED_TYPE, CLOSURE_TYPE);
-                    pexp = propX(pexp, props[i]);
+            if (currentCall != null) {
+                String implicitReceiver = currentCall.getNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER);
+                if (implicitReceiver != null) {
+                    String[] pathElements = implicitReceiver.split("\\.");
+                    BytecodeExpression thisLoader = bytecodeX(CLOSURE_TYPE, mv -> mv.visitVarInsn(ALOAD, 0));
+                    PropertyExpression pexp = propX(thisLoader, constX(pathElements[0]), safe);
+                    for (int i = 1, n = pathElements.length; i < n; i += 1) {
+                        pexp.putNodeMetaData(StaticTypesMarker.INFERRED_TYPE, CLOSURE_TYPE);
+                        pexp = propX(pexp, pathElements[i]);
+                    }
+                    pexp.visit(controller.getAcg());
+                    return;
                 }
-                pexp.visit(controller.getAcg());
-                return;
             }
         }
 
-        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;
 
         boolean isScriptVariable = (receiverType.isScript() && receiver instanceof VariableExpression && ((VariableExpression) receiver).getAccessedVariable() == null);
         if (!isScriptVariable && controller.getClassNode().getOuterClass() == null) { // inner class still needs dynamic property sequence
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
-    }
-
 }