Skip to content

Commit 6f717ac

Browse files
paulk-asertpaulk-asert
committed
GROOVY-11910: Add ModifiesChecker type checking extension to verify @Modifies frame conditions
1 parent 3f18de2 commit 6f717ac

7 files changed

Lines changed: 659 additions & 0 deletions

File tree

‎src/test/groovy/org/codehaus/groovy/transform/traitx/TraitASTTransformationTest.groovy‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4084,4 +4084,23 @@ final class TraitASTTransformationTest {
40844084
new C().test()
40854085
'''
40864086
}
4087+
4088+
// GROOVY-11XXX
4089+
@Test
4090+
void testTraitWithStaticFieldAndSpockSpecification() {
4091+
assertScript shell, '''
4092+
@Grab('org.spockframework:spock-core:2.4-groovy-5.0')
4093+
@GrabExclude('org.apache.groovy:*')
4094+
import spock.lang.Specification
4095+
4096+
@CompileStatic
4097+
trait MyTrait {
4098+
static String myStaticField
4099+
}
4100+
4101+
@CompileStatic
4102+
abstract class MySpec extends Specification implements MyTrait {
4103+
}
4104+
'''
4105+
}
40874106
}

‎subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/ModifiesASTTransformation.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.codehaus.groovy.ast.ASTNode;
2222
import org.codehaus.groovy.ast.AnnotationNode;
23+
import org.codehaus.groovy.ast.ClassHelper;
2324
import org.codehaus.groovy.ast.ClassNode;
2425
import org.codehaus.groovy.ast.MethodNode;
2526
import org.codehaus.groovy.ast.Parameter;
@@ -75,6 +76,10 @@ public void visit(final ASTNode[] nodes, final SourceUnit source) {
7576

7677
extractFromAnnotation(annotation, methodNode, modifiesSet, source);
7778

79+
// Replace the closure with a class reference so the type checker
80+
// doesn't try to type-check the specification closure
81+
annotation.setMember("value", new org.codehaus.groovy.ast.expr.ClassExpression(ClassHelper.OBJECT_TYPE));
82+
7883
if (!modifiesSet.isEmpty()) {
7984
methodNode.putNodeMetaData(MODIFIES_FIELDS_KEY, modifiesSet);
8085
}

‎subprojects/groovy-contracts/src/spec/doc/contracts-userguide.adoc‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,3 +231,18 @@ The `@Modifies` annotation validates at compile time that:
231231
* Parameter references use the bare parameter name.
232232
* The closure is not executed at runtime — it is only analyzed at compile time.
233233
234+
=== Verifying method bodies with ModifiesChecker
235+
236+
While `@Modifies` alone is a compile-time-checked specification (validating field/parameter
237+
existence and `@Ensures` consistency), it does not verify the method body itself. For that,
238+
the `groovy-typecheckers` module provides the `ModifiesChecker` type checking extension:
239+
240+
[source,groovy]
241+
----
242+
@TypeChecked(extensions = 'groovy.typecheckers.ModifiesChecker')
243+
----
244+
245+
When enabled, the checker verifies that method bodies only modify fields declared in
246+
`@Modifies`, and that method calls on non-modifiable receivers use only non-mutating
247+
operations. See the _Type Checkers_ documentation for details.
248+

‎subprojects/groovy-typecheckers/build.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ dependencies {
2424
implementation rootProject
2525
api projects.groovyMacro
2626
testImplementation projects.groovyTest
27+
testImplementation projects.groovyContracts
2728
testImplementation 'org.jspecify:jspecify:1.0.0'
2829
}
2930

‎subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/ModifiesChecker.groovy‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package groovy.typecheckers
20+
21+
import org.apache.groovy.ast.tools.ImmutablePropertyUtils
22+
import org.apache.groovy.lang.annotation.Incubating
23+
import org.apache.groovy.typecheckers.CheckingVisitor
24+
import org.codehaus.groovy.ast.ClassNode
25+
import org.codehaus.groovy.ast.FieldNode
26+
import org.codehaus.groovy.ast.MethodNode
27+
import org.codehaus.groovy.ast.Variable
28+
import org.codehaus.groovy.ast.expr.BinaryExpression
29+
import org.codehaus.groovy.ast.expr.DeclarationExpression
30+
import org.codehaus.groovy.ast.expr.Expression
31+
import org.codehaus.groovy.ast.expr.MethodCallExpression
32+
import org.codehaus.groovy.ast.expr.PostfixExpression
33+
import org.codehaus.groovy.ast.expr.PrefixExpression
34+
import org.codehaus.groovy.ast.expr.PropertyExpression
35+
import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
36+
import org.codehaus.groovy.ast.expr.VariableExpression
37+
import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport
38+
import org.codehaus.groovy.transform.stc.StaticTypesMarker
39+
40+
import static org.codehaus.groovy.syntax.Types.isAssignment
41+
42+
/**
43+
* A compile-time checker that verifies method bodies comply with their
44+
* {@code @Modifies} frame condition declarations. Checks that:
45+
* <ul>
46+
* <li>Direct field assignments only target fields listed in {@code @Modifies}</li>
47+
* <li>Calls to methods on {@code this} are compatible with the declared frame</li>
48+
* <li>Calls on parameters/variables not in {@code @Modifies} use only non-mutating methods</li>
49+
* </ul>
50+
* <p>
51+
* Non-mutating calls are determined by:
52+
* <ol>
53+
* <li>Receiver type is immutable (String, Integer, etc.)</li>
54+
* <li>Method is annotated {@code @Pure}</li>
55+
* <li>Method name is in a known-safe whitelist (toString, size, get, etc.)</li>
56+
* </ol>
57+
* <p>
58+
* This checker is opt-in:
59+
* <pre>
60+
* {@code @TypeChecked(extensions = 'groovy.typecheckers.ModifiesChecker')}
61+
* </pre>
62+
*
63+
* @since 6.0.0
64+
* @see groovy.contracts.Modifies
65+
*/
66+
@Incubating
67+
class ModifiesChecker extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
68+
69+
// Methods known to be non-mutating on common mutable types
70+
private static final Set<String> SAFE_METHOD_NAMES = Set.of(
71+
// Object fundamentals
72+
'toString', 'hashCode', 'equals', 'compareTo', 'getClass',
73+
// Collection/Map queries
74+
'size', 'length', 'isEmpty', 'contains', 'containsKey', 'containsValue',
75+
'get', 'getAt', 'getOrDefault', 'indexOf', 'lastIndexOf',
76+
'iterator', 'listIterator', 'spliterator',
77+
'stream', 'parallelStream',
78+
'toArray', 'toList', 'toSet', 'toSorted', 'toUnique',
79+
'subList', 'headSet', 'tailSet', 'subSet',
80+
'keySet', 'values', 'entrySet',
81+
'first', 'last', 'head', 'tail', 'init',
82+
'find', 'findAll', 'collect', 'collectEntries',
83+
'any', 'every', 'count', 'sum', 'min', 'max',
84+
'join', 'inject', 'groupBy', 'countBy',
85+
'each', 'eachWithIndex', 'reverseEach',
86+
// String queries
87+
'charAt', 'substring', 'trim', 'strip', 'toLowerCase', 'toUpperCase',
88+
'startsWith', 'endsWith', 'matches', 'split',
89+
// Array queries
90+
'clone'
91+
)
92+
93+
private static final Set<String> PURE_ANNOS = Set.of('Pure')
94+
95+
@Override
96+
Object run() {
97+
afterVisitMethod { MethodNode mn ->
98+
// Key matches ModifiesASTTransformation.MODIFIES_FIELDS_KEY — no hard dependency on groovy-contracts
99+
Set<String> modifiesSet = mn.getNodeMetaData('groovy.contracts.modifiesFields') as Set<String>
100+
if (modifiesSet == null) return // no @Modifies on this method — nothing to check
101+
102+
mn.code?.visit(makeVisitor(modifiesSet, mn))
103+
}
104+
}
105+
106+
private CheckingVisitor makeVisitor(Set<String> modifiesSet, MethodNode methodNode) {
107+
Set<String> paramNames = methodNode.parameters*.name as Set<String>
108+
109+
new CheckingVisitor() {
110+
111+
@Override
112+
void visitDeclarationExpression(DeclarationExpression decl) {
113+
super.visitDeclarationExpression(decl)
114+
// Local variable declarations are always fine
115+
}
116+
117+
@Override
118+
void visitBinaryExpression(BinaryExpression expression) {
119+
super.visitBinaryExpression(expression)
120+
if (isAssignment(expression.operation.type)) {
121+
checkWriteTarget(expression.leftExpression, expression)
122+
}
123+
}
124+
125+
@Override
126+
void visitPostfixExpression(PostfixExpression expression) {
127+
super.visitPostfixExpression(expression)
128+
checkWriteTarget(expression.expression, expression)
129+
}
130+
131+
@Override
132+
void visitPrefixExpression(PrefixExpression expression) {
133+
super.visitPrefixExpression(expression)
134+
checkWriteTarget(expression.expression, expression)
135+
}
136+
137+
@Override
138+
void visitMethodCallExpression(MethodCallExpression call) {
139+
super.visitMethodCallExpression(call)
140+
checkMethodCall(call.objectExpression, call)
141+
}
142+
143+
// ---- helpers ----
144+
145+
private void checkWriteTarget(Expression target, Expression context) {
146+
if (target instanceof PropertyExpression) {
147+
def objExpr = target.objectExpression
148+
if (objExpr instanceof VariableExpression && objExpr.isThisExpression()) {
149+
String fieldName = target.propertyAsString
150+
if (fieldName && !modifiesSet.contains(fieldName)) {
151+
addStaticTypeError("@Modifies violation: assignment to 'this.${fieldName}' but '${fieldName}' is not declared in @Modifies", context)
152+
}
153+
}
154+
} else if (target instanceof VariableExpression) {
155+
// Check if this is actually a field (implicit this)
156+
Variable accessedVar = findTargetVariable(target)
157+
if (accessedVar instanceof FieldNode) {
158+
String fieldName = accessedVar.name
159+
if (!modifiesSet.contains(fieldName)) {
160+
addStaticTypeError("@Modifies violation: assignment to '${fieldName}' but '${fieldName}' is not declared in @Modifies", context)
161+
}
162+
}
163+
// Local variables and parameters being reassigned are fine
164+
}
165+
}
166+
167+
private void checkMethodCall(Expression receiver, MethodCallExpression call) {
168+
if (receiver instanceof VariableExpression && receiver.isThisExpression()) {
169+
checkCallOnThis(call)
170+
} else if (receiver instanceof VariableExpression || receiver instanceof PropertyExpression) {
171+
checkCallOnVariable(receiver, call)
172+
}
173+
}
174+
175+
private void checkCallOnThis(MethodCallExpression call) {
176+
// Check if the callee method has @Modifies
177+
def targetMethod = call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
178+
if (!(targetMethod instanceof MethodNode)) return
179+
180+
Set<String> calleeModifies = targetMethod.getNodeMetaData('groovy.contracts.modifiesFields') as Set<String>
181+
182+
if (calleeModifies != null) {
183+
// Callee has @Modifies — check it's a subset of our frame
184+
for (String field : calleeModifies) {
185+
if (!modifiesSet.contains(field)) {
186+
addStaticTypeError("@Modifies violation: call to '${targetMethod.name}()' modifies '${field}' which is not in this method's @Modifies", call)
187+
}
188+
}
189+
} else if (hasPureAnno(targetMethod)) {
190+
// @Pure methods are safe
191+
} else if (SAFE_METHOD_NAMES.contains(call.methodAsString)) {
192+
// Known-safe method name
193+
} else {
194+
// Unknown effects — warn
195+
addStaticTypeError("@Modifies warning: call to 'this.${call.methodAsString}()' has unknown effects (consider adding @Modifies or @Pure)", call)
196+
}
197+
}
198+
199+
private void checkCallOnVariable(Expression receiver, MethodCallExpression call) {
200+
String receiverName = resolveReceiverName(receiver)
201+
202+
// If receiver is in the modifies set, any call is allowed
203+
if (receiverName && modifiesSet.contains(receiverName)) return
204+
205+
// Check if the method is known to be safe
206+
def targetMethod = call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
207+
208+
// Immutable receiver type — all calls safe
209+
ClassNode receiverType = getType(receiver)
210+
if (receiverType && ImmutablePropertyUtils.isBuiltinImmutable(receiverType.name)) return
211+
212+
// @Pure method
213+
if (targetMethod instanceof MethodNode && hasPureAnno(targetMethod)) return
214+
215+
// Known-safe method name
216+
if (SAFE_METHOD_NAMES.contains(call.methodAsString)) return
217+
218+
// Unknown — warn
219+
if (receiverName) {
220+
addStaticTypeError("@Modifies warning: call to '${receiverName}.${call.methodAsString}()' may modify '${receiverName}' which is not in @Modifies", call)
221+
}
222+
}
223+
224+
private String resolveReceiverName(Expression receiver) {
225+
if (receiver instanceof VariableExpression) {
226+
return receiver.name
227+
}
228+
if (receiver instanceof PropertyExpression) {
229+
def obj = receiver.objectExpression
230+
if (obj instanceof VariableExpression && obj.isThisExpression()) {
231+
return receiver.propertyAsString
232+
}
233+
}
234+
null
235+
}
236+
237+
private static boolean hasPureAnno(MethodNode method) {
238+
method.annotations?.any { it.classNode?.nameWithoutPackage in PURE_ANNOS } ?: false
239+
}
240+
}
241+
}
242+
}

0 commit comments

Comments
 (0)