You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by su...@apache.org on 2019/06/28 17:28:53 UTC
[groovy] branch master updated: GROOVY-9127: check for subclass
access to field for this prop expression
This is an automated email from the ASF dual-hosted git repository.
sunlan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new a44cfc5 GROOVY-9127: check for subclass access to field for this prop expression
a44cfc5 is described below
commit a44cfc5ac24ff6bc0b31a59759e6148909fece3d
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Wed Jun 5 10:00:38 2019 -0500
GROOVY-9127: check for subclass access to field for this prop expression
---
.../transform/stc/StaticTypeCheckingVisitor.java | 29 +++-
src/test/groovy/bugs/Groovy9063.groovy | 44 ++++++
src/test/groovy/bugs/Groovy9063Bug.groovy | 42 ------
src/test/groovy/bugs/Groovy9127.groovy | 164 +++++++++++++++++++++
src/test/groovy/bugs/Groovy9141.groovy | 79 +++++-----
5 files changed, 266 insertions(+), 92 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 16a44f4..6963641 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -126,6 +126,7 @@ import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
@@ -181,6 +182,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.binX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.block;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.castX;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.isOrImplements;
import static org.codehaus.groovy.ast.tools.GeneralUtils.localVarX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
import static org.codehaus.groovy.ast.tools.GenericsUtils.findActualTypeByGenericsPlaceholderName;
@@ -1599,15 +1601,18 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
FieldNode field = current.getDeclaredField(propertyName);
field = allowStaticAccessToMember(field, staticOnly);
- if (storeField(field, isAttributeExpression, pexp, current, visitor, receiver.getData(), !readMode))
+ if (storeField(field, isAttributeExpression, pexp, current, visitor, receiver.getData(), !readMode)) {
+ pexp.removeNodeMetaData(StaticTypesMarker.READONLY_PROPERTY);
return true;
+ }
- boolean isThisExpression = objectExpression instanceof VariableExpression
- && ((VariableExpression) objectExpression).isThisExpression()
- && objectExpressionType.equals(current);
+ boolean isThisExpression = objectExpression instanceof VariableExpression && ((VariableExpression) objectExpression).isThisExpression()
+ && (objectExpressionType.equals(current) || (objectExpressionType.isDerivedFrom(current) && hasAccessToField(field, objectExpressionType)));
- if (storeField(field, isThisExpression, pexp, receiver.getType(), visitor, receiver.getData(), !readMode))
+ if (storeField(field, isThisExpression, pexp, receiver.getType(), visitor, receiver.getData(), !readMode)) {
+ pexp.removeNodeMetaData(StaticTypesMarker.READONLY_PROPERTY);
return true;
+ }
MethodNode getter = findGetter(current, "get" + capName, pexp.isImplicitThis());
getter = allowStaticAccessToMember(getter, staticOnly);
@@ -1723,6 +1728,18 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
return foundGetterOrSetter;
}
+ private static boolean hasAccessToField(FieldNode field, ClassNode objectExpressionType) {
+ if (field != null) {
+ if (field.isPublic() || field.isProtected()) {
+ return true;
+ }
+ if (!field.isPrivate() && Objects.equals(objectExpressionType.getPackageName(), field.getDeclaringClass().getPackageName())) {
+ return true;
+ }
+ }
+ return false;
+ }
+
private MethodNode findGetter(ClassNode current, String name, boolean searchOuterClasses) {
MethodNode getterMethod = current.getGetterMethod(name);
if (getterMethod == null && searchOuterClasses && current instanceof InnerClassNode) {
@@ -2755,7 +2772,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
private void checkNamedParamsAnnotation(Parameter param, MapExpression args) {
- if (!param.getType().isDerivedFrom(ClassHelper.MAP_TYPE)) return;
+ if (!isOrImplements(param.getType(), ClassHelper.MAP_TYPE)) return;
List<MapEntryExpression> entryExpressions = args.getMapEntryExpressions();
Map<Object, Expression> entries = new LinkedHashMap<Object, Expression>();
for (MapEntryExpression entry : entryExpressions) {
diff --git a/src/test/groovy/bugs/Groovy9063.groovy b/src/test/groovy/bugs/Groovy9063.groovy
new file mode 100644
index 0000000..488e870
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy9063.groovy
@@ -0,0 +1,44 @@
+/*
+ * 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
+
+@CompileStatic
+final class Groovy9063 extends GroovyTestCase {
+
+ void testProtectedFieldAccessFromNestedClosure() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class Groovy9063 {
+ protected String message = 'hello'
+
+ int nestedClosures() {
+ { ->
+ { ->
+ message.length()
+ }.call()
+ }.call()
+ }
+ }
+
+ assert new Groovy9063().nestedClosures() == 5
+ '''
+ }
+}
diff --git a/src/test/groovy/bugs/Groovy9063Bug.groovy b/src/test/groovy/bugs/Groovy9063Bug.groovy
deleted file mode 100644
index 9374e82..0000000
--- a/src/test/groovy/bugs/Groovy9063Bug.groovy
+++ /dev/null
@@ -1,42 +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 Groovy9063Bug extends GroovyTestCase {
- void testProtectedFieldInClosureInEnum() {
- assertScript '''
- import groovy.transform.CompileStatic
-
- @CompileStatic
- class Groovy9063 {
- protected String message = "hello"
-
- int nestedClosures() {
- { ->
- { ->
- message.length()
- }.call()
- }.call()
- }
- }
-
- assert new Groovy9063().nestedClosures() == 5
- '''
- }
-}
diff --git a/src/test/groovy/bugs/Groovy9127.groovy b/src/test/groovy/bugs/Groovy9127.groovy
new file mode 100644
index 0000000..96a5efd
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy9127.groovy
@@ -0,0 +1,164 @@
+/*
+ * 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.codehaus.groovy.control.CompilationFailedException
+import org.codehaus.groovy.control.CompilationUnit
+
+import static org.codehaus.groovy.control.Phases.CLASS_GENERATION
+
+@CompileStatic
+final class Groovy9127 extends GroovyTestCase {
+
+ void testReadOnlyPropertyAssignment1() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class Foo {
+ protected String field = 'foo'
+ String getField() { return field }
+ }
+ @groovy.transform.CompileStatic
+ class Bar extends Foo {
+ void changeField() {
+ field = 'bar' // GROOVY-9127: [Static type checking] - Cannot set read-only property: field
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ def bar = new Bar()
+ bar.changeField()
+ assert bar.field == 'value'
+ '''
+ }
+
+ void testReadOnlyPropertyAssignment2() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class Foo {
+ public String field = 'foo'
+ String getField() { return field }
+ }
+ @groovy.transform.CompileStatic
+ class Bar extends Foo {
+ void changeField() {
+ field = 'bar'
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ def bar = new Bar()
+ bar.changeField()
+ assert bar.field == 'value'
+ '''
+ }
+
+ void testReadOnlyPropertyAssignment3() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class Foo {
+ @groovy.transform.PackageScope String field = 'foo'
+ String getField() { return field }
+ }
+ @groovy.transform.CompileStatic
+ class Bar extends Foo {
+ void changeField() {
+ field = 'bar'
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ def bar = new Bar()
+ bar.changeField()
+ assert bar.field == 'value'
+ '''
+ }
+
+ void testReadOnlyPropertyAssignment4() {
+ new CompilationUnit().with {
+ addSource 'Foo.groovy', '''
+ package foo
+
+ @groovy.transform.CompileStatic
+ class Foo {
+ @groovy.transform.PackageScope String field = 'foo'
+ String getField() { return field }
+ }
+ '''
+
+ addSource 'Bar.groovy', '''
+ package bar
+
+ @groovy.transform.CompileStatic
+ class Bar extends foo.Foo {
+ void changeField() {
+ field = 'bar'
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ '''
+
+ def err = shouldFail CompilationFailedException, {
+ compile CLASS_GENERATION
+ }
+ assert err =~ /\[Static type checking\] - Cannot set read-only property: field/
+ }
+ }
+
+ void testReadOnlyPropertyAssignment5() {
+ def err = shouldFail CompilationFailedException, '''
+ @groovy.transform.CompileStatic
+ class Foo {
+ private String field = 'foo'
+ String getField() { return field }
+ }
+ @groovy.transform.CompileStatic
+ class Bar extends Foo {
+ void changeField() {
+ field = 'bar'
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ '''
+ assert err =~ /\[Static type checking\] - Cannot set read-only property: field/
+ }
+
+ void testAttributeAssignmentVariation() {
+ assertScript '''
+ @groovy.transform.CompileStatic
+ class Foo {
+ protected String field = 'foo'
+ String getField() { return field }
+ }
+ @groovy.transform.CompileStatic
+ class Bar extends Foo {
+ void changeField() {
+ this.@field = 'bar'
+ }
+ @Override
+ String getField() { return 'value' }
+ }
+ def bar = new Bar()
+ bar.changeField()
+ assert bar.field == 'value'
+ '''
+ }
+}
diff --git a/src/test/groovy/bugs/Groovy9141.groovy b/src/test/groovy/bugs/Groovy9141.groovy
index c5d707c..b3b6389 100644
--- a/src/test/groovy/bugs/Groovy9141.groovy
+++ b/src/test/groovy/bugs/Groovy9141.groovy
@@ -1,66 +1,57 @@
/*
- * 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
+ * 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
+ * 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.
+ * 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 gls.CompilableTestSupport
import groovy.transform.CompileStatic
+import org.codehaus.groovy.control.CompilationFailedException
import org.codehaus.groovy.control.CompilerConfiguration
-import static org.codehaus.groovy.control.ParserVersion.V_2
+import static org.codehaus.groovy.control.ParserPluginFactory.antlr2
@CompileStatic
-final class Groovy9141 extends CompilableTestSupport {
- private static final String ABSTRACT_METHOD_WITH_BODY = '''
- abstract meth() { }
- '''
-
- // not a language requirement but script-level check takes precedence in current implementation
- void testAbstractMethodWithBodyInScript() {
- def err = shouldNotCompile ABSTRACT_METHOD_WITH_BODY
- assert err =~ / You cannot define an abstract method\[meth] in the script. Try removing the 'abstract' /
- }
+final class Groovy9141 extends GroovyTestCase {
void testAbstractMethodWithBodyInClass() {
- def err = shouldNotCompile """
- class Main {
- $ABSTRACT_METHOD_WITH_BODY
+ def err = shouldFail CompilationFailedException, '''
+ abstract class Main {
+ abstract void meth() {}
}
- """
- // not a language requirement but class level check takes precedence in current implementation
- assert err =~ / Can't have an abstract method in a non-abstract class. /
+ '''
+ assert err =~ / You defined an abstract method\[meth\] with a body. Try removing the method body @ line /
}
- void testAbstractMethodWithBodyInCAbstractlass() {
- def err = shouldNotCompile """
- abstract class Main {
- $ABSTRACT_METHOD_WITH_BODY
- }
- """
- assert err =~ / You defined an abstract method\[meth] with a body. Try removing the method body @ line /
+ // not a language requirement but script-level check takes precedence in current implementation
+ void testAbstractMethodWithBodyInScript() {
+ def err = shouldFail CompilationFailedException, '''
+ abstract void meth() {}
+ '''
+ assert err =~ / You cannot define an abstract method\[meth\] in the script. Try removing the 'abstract' /
}
void testAbstractMethodWithBodyInScript_oldParser() {
- def cc = new CompilerConfiguration(parserVersion: V_2)
- def err = shouldFail {
- new GroovyShell(cc).evaluate ABSTRACT_METHOD_WITH_BODY
+ def shell = new GroovyShell(new CompilerConfiguration(pluginFactory: antlr2()))
+
+ def err = shouldFail CompilationFailedException, {
+ shell.evaluate '''
+ abstract void meth() {}
+ '''
}
- assert err =~ / Abstract methods do not define a body/
+ assert err =~ / Abstract methods do not define a body. /
}
-
}