This is an automated email from the ASF dual-hosted git repository.
paulk 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 2c25b58737 GROOVY-11910: Add ModifiesChecker type checking extension
to verify @Modifies frame conditions (recognize JetBrains and checker framework
annotations too)
2c25b58737 is described below
commit 2c25b58737586d8a540c8c2b5a7bf684a97bd7e4
Author: Paul King <[email protected]>
AuthorDate: Fri Apr 10 10:55:02 2026 +1000
GROOVY-11910: Add ModifiesChecker type checking extension to verify
@Modifies frame conditions (recognize JetBrains and checker framework
annotations too)
---
.../groovy/typecheckers/ModifiesChecker.groovy | 164 ++++++++++++-
.../src/spec/doc/typecheckers.adoc | 51 ++++
.../groovy/typecheckers/ModifiesCheckerTest.groovy | 258 +++++++++++++++++++++
3 files changed, 462 insertions(+), 11 deletions(-)
diff --git
a/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/ModifiesChecker.groovy
b/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/ModifiesChecker.groovy
index 1189607c6f..7412fc215c 100644
---
a/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/ModifiesChecker.groovy
+++
b/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/ModifiesChecker.groovy
@@ -91,25 +91,82 @@ class ModifiesChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
)
private static final Set<String> PURE_ANNOS = Set.of('Pure')
+ private static final Set<String> SIDE_EFFECT_FREE_ANNOS =
Set.of('SideEffectFree')
+ private static final Set<String> CONTRACT_ANNOS = Set.of('Contract')
@Override
Object run() {
afterVisitMethod { MethodNode mn ->
// Key matches ModifiesASTTransformation.MODIFIES_FIELDS_KEY — no
hard dependency on groovy-contracts
Set<String> modifiesSet =
mn.getNodeMetaData('groovy.contracts.modifiesFields') as Set<String>
- if (modifiesSet == null && hasPureAnno(mn)) {
- modifiesSet = Collections.emptySet() // @Pure implies
@Modifies({})
+ if (modifiesSet == null && isPureEquivalent(mn)) {
+ modifiesSet = Collections.emptySet() //
@Pure/@SideEffectFree/@Contract(pure=true) implies @Modifies({})
}
- if (modifiesSet == null) return // no @Modifies or @Pure on this
method — nothing to check
+ if (modifiesSet == null) return // no @Modifies or purity
annotation — nothing to check
mn.code?.visit(makeVisitor(modifiesSet, mn))
}
}
+ /** Checks for @Pure, @SideEffectFree, or @Contract(pure=true). */
+ private static boolean isPureEquivalent(MethodNode method) {
+ hasPureAnno(method) || hasSideEffectFreeAnno(method) ||
hasContractPureAnno(method)
+ }
+
private static boolean hasPureAnno(MethodNode method) {
method.annotations?.any { it.classNode?.nameWithoutPackage in
PURE_ANNOS } ?: false
}
+ private static boolean hasSideEffectFreeAnno(MethodNode method) {
+ method.annotations?.any { it.classNode?.nameWithoutPackage in
SIDE_EFFECT_FREE_ANNOS } ?: false
+ }
+
+ /**
+ * Checks for {@code @Contract(pure = true)} (JetBrains annotations).
+ */
+ private static boolean hasContractPureAnno(MethodNode method) {
+ method.annotations?.any { anno ->
+ anno.classNode?.nameWithoutPackage in CONTRACT_ANNOS &&
+ anno.getMember('pure')?.text == 'true'
+ } ?: false
+ }
+
+ /**
+ * Parses {@code @Contract(mutates = "this,param1")} into a set of
mutation targets.
+ * Returns null if no mutates attribute is present.
+ * Maps "this" to field names (all fields) and "param1","param2" etc. to
parameter names.
+ */
+ private static Set<String> parseContractMutates(MethodNode callee) {
+ for (anno in callee.annotations) {
+ if (anno.classNode?.nameWithoutPackage in CONTRACT_ANNOS) {
+ def mutatesExpr = anno.getMember('mutates')
+ if (mutatesExpr != null) {
+ String mutatesStr = mutatesExpr.text
+ if (mutatesStr == null || mutatesStr.isEmpty()) return
Collections.emptySet()
+ Set<String> result = new LinkedHashSet<>()
+ def params = callee.parameters
+ for (String token : mutatesStr.split(',')) {
+ token = token.trim()
+ if (token == 'this') {
+ // "this" means the callee mutates its own receiver
+ result.add('this')
+ } else if (token.startsWith('param')) {
+ // "param1" = first parameter (1-based)
+ try {
+ int idx = Integer.parseInt(token.substring(5))
- 1
+ if (idx >= 0 && idx < params.length) {
+ result.add(params[idx].name)
+ }
+ } catch (NumberFormatException ignored) {}
+ }
+ }
+ return result
+ }
+ }
+ }
+ null
+ }
+
private CheckingVisitor makeVisitor(Set<String> modifiesSet, MethodNode
methodNode) {
Set<String> paramNames = methodNode.parameters*.name as Set<String>
@@ -147,6 +204,12 @@ class ModifiesChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
checkMethodCall(call.objectExpression, call)
}
+ @Override
+ void visitStaticMethodCallExpression(StaticMethodCallExpression
call) {
+ super.visitStaticMethodCallExpression(call)
+ checkStaticCall(call)
+ }
+
// ---- helpers ----
private void checkWriteTarget(Expression target, Expression
context) {
@@ -193,13 +256,26 @@ class ModifiesChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
addStaticTypeError("@Modifies violation: call to
'${targetMethod.name}()' modifies '${field}' which is not in this method's
@Modifies", call)
}
}
- } else if (hasPureAnno(targetMethod)) {
- // @Pure methods are safe
- } else if (SAFE_METHOD_NAMES.contains(call.methodAsString)) {
- // Known-safe method name
+ } else if (isPureEquivalent(targetMethod)) {
+ // @Pure, @SideEffectFree, @Contract(pure=true) methods
are safe
} else {
- // Unknown effects — warn
- addStaticTypeError("@Modifies warning: call to
'this.${call.methodAsString}()' has unknown effects (consider adding @Modifies
or @Pure)", call)
+ // Check @Contract(mutates=...) on callee
+ Set<String> contractMutates =
parseContractMutates(targetMethod)
+ if (contractMutates != null) {
+ if (contractMutates.isEmpty()) return // mutates
nothing — pure
+ // Check parameter mutations (arguments that map to
mutated params)
+ checkContractParamMutations(contractMutates,
targetMethod, call)
+ // "this" in mutates means the callee mutates its
receiver
+ if (contractMutates.contains('this')) {
+ addStaticTypeError("@Modifies warning: call to
'this.${call.methodAsString}()' declares @Contract(mutates=\"this\") — may
modify fields not in @Modifies", call)
+ }
+ return
+ } else if
(SAFE_METHOD_NAMES.contains(call.methodAsString)) {
+ // Known-safe method name
+ } else {
+ // Unknown effects — warn
+ addStaticTypeError("@Modifies warning: call to
'this.${call.methodAsString}()' has unknown effects (consider adding @Modifies
or @Pure)", call)
+ }
}
}
@@ -216,8 +292,29 @@ class ModifiesChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
ClassNode receiverType = getType(receiver)
if (receiverType &&
ImmutablePropertyUtils.isBuiltinImmutable(receiverType.name)) return
- // @Pure method
- if (targetMethod instanceof MethodNode &&
hasPureAnno(targetMethod)) return
+ // @Pure, @SideEffectFree, @Contract(pure=true) method
+ if (targetMethod instanceof MethodNode &&
isPureEquivalent(targetMethod)) return
+
+ // @Contract(mutates=...) — check if this call mutates the
receiver
+ if (targetMethod instanceof MethodNode) {
+ Set<String> contractMutates =
parseContractMutates(targetMethod)
+ if (contractMutates != null) {
+ if (contractMutates.isEmpty()) return // mutates
nothing
+ // Check if the callee mutates "this" (the receiver
variable)
+ // and map paramN to the actual argument expressions
+ if (!contractMutates.contains('this')) {
+ // Callee doesn't mutate its receiver — safe for
our variable
+ // But check if any mutated params map to our
non-modifiable variables
+ checkContractParamMutations(contractMutates,
targetMethod, call)
+ return
+ }
+ // Callee mutates "this" = our receiver variable
+ if (receiverName) {
+ addStaticTypeError("@Modifies warning: call to
'${receiverName}.${call.methodAsString}()' may modify '${receiverName}' which
is not in @Modifies", call)
+ }
+ return
+ }
+ }
// Known-safe method name
if (SAFE_METHOD_NAMES.contains(call.methodAsString)) return
@@ -228,6 +325,51 @@ class ModifiesChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
}
}
+ private void checkStaticCall(StaticMethodCallExpression call) {
+ def targetMethod =
call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
+ if (!(targetMethod instanceof MethodNode)) return
+
+ // Check @Contract(mutates=...) on static callee
+ Set<String> contractMutates =
parseContractMutates(targetMethod)
+ if (contractMutates != null && !contractMutates.isEmpty()) {
+ // Static methods can't mutate "this", but can mutate
params
+ def args = call.arguments
+ if (args instanceof
org.codehaus.groovy.ast.expr.TupleExpression) {
+ def argList = args.expressions
+ def params = targetMethod.parameters
+ for (int i = 0; i < params.length && i <
argList.size(); i++) {
+ if (contractMutates.contains(params[i].name)) {
+ String argName =
resolveReceiverName(argList[i])
+ if (argName && !modifiesSet.contains(argName))
{
+ addStaticTypeError("@Modifies warning:
argument '${argName}' passed to '${call.methodAsString}()' parameter
'${params[i].name}' which is declared as mutated", call)
+ }
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * For @Contract(mutates="param1,param2"), check if the actual
arguments
+ * at the call site are variables not in our modifies set.
+ */
+ private void checkContractParamMutations(Set<String>
contractMutates, MethodNode callee, MethodCallExpression call) {
+ def args = call.arguments
+ if (!(args instanceof
org.codehaus.groovy.ast.expr.TupleExpression)) return
+ def argList = args.expressions
+ def params = callee.parameters
+
+ for (int i = 0; i < params.length && i < argList.size(); i++) {
+ if (contractMutates.contains(params[i].name)) {
+ // This parameter is mutated — check what argument was
passed
+ String argName = resolveReceiverName(argList[i])
+ if (argName && !modifiesSet.contains(argName)) {
+ addStaticTypeError("@Modifies warning: argument
'${argName}' passed to '${call.methodAsString}()' parameter '${params[i].name}'
which is declared as mutated", call)
+ }
+ }
+ }
+ }
+
private String resolveReceiverName(Expression receiver) {
if (receiver instanceof VariableExpression) {
return receiver.name
diff --git a/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
b/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
index c508af2bb7..b47a271e1c 100644
--- a/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
+++ b/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
@@ -761,6 +761,57 @@ void process(List list1, List list2) {
}
----
+=== Supported annotations from other libraries
+
+The `ModifiesChecker` recognises purity and frame condition annotations from
multiple
+libraries by simple name:
+
+[cols="2,2,3",options="header"]
+|===
+| Annotation | Library | Semantics
+
+| `@Pure`
+| Groovy, Checker Framework, Xtext
+| Implies `@Modifies({})` -- no field mutations
+
+| `@SideEffectFree`
+| Checker Framework
+| Implies `@Modifies({})` -- no field mutations
+
+| `@Contract(pure = true)`
+| JetBrains Annotations
+| Implies `@Modifies({})` -- works with CLASS retention
+
+| `@Contract(mutates = "...")`
+| JetBrains Annotations
+| Parsed as a frame condition on callees (see below)
+|===
+
+==== @Contract(mutates) support
+
+When a callee has `@Contract(mutates = "...")`, the checker parses the mutates
string
+to determine what the callee modifies:
+
+* `mutates = ""` -- callee mutates nothing (equivalent to `@Pure`)
+* `mutates = "this"` -- callee mutates its receiver
+* `mutates = "param1"` -- callee mutates its first parameter
+* `mutates = "this,param1"` -- callee mutates both
+
+The checker maps `param1`, `param2`, etc. to the actual arguments at the call
site
+and warns if a mutated argument is not in the caller's `@Modifies` set:
+
+[source,groovy]
+----
+@Modifies({ this.items })
+void process() {
+ addToList(items, 'x') // OK: items is in @Modifies
+ addToList(other, 'x') // WARNING: other is not in @Modifies
+}
+
+@Contract(mutates = "param1")
+static void addToList(List list, String item) { list.add(item) }
+----
+
=== Interaction with other contract annotations
The `ModifiesChecker` works alongside the other design-by-contract annotations
to provide
diff --git
a/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/ModifiesCheckerTest.groovy
b/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/ModifiesCheckerTest.groovy
index b0d15ec837..59169aeb37 100644
---
a/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/ModifiesCheckerTest.groovy
+++
b/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/ModifiesCheckerTest.groovy
@@ -304,4 +304,262 @@ final class ModifiesCheckerTest {
assert a.count == 1
'''
}
+
+ // === @SideEffectFree implies @Modifies({}) ===
+
+ @Test
+ void side_effect_free_method_checked_for_field_writes() {
+ def err = shouldFail shell, '''
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.METHOD)
+ @interface SideEffectFree {}
+
+ class A {
+ int count = 0
+
+ @SideEffectFree
+ int broken() {
+ count++
+ return count
+ }
+ }
+ '''
+ assert err.message.contains('@Modifies violation')
+ }
+
+ @Test
+ void side_effect_free_method_without_writes_passes() {
+ assertScript shell, '''
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.METHOD)
+ @interface SideEffectFree {}
+
+ class A {
+ int value = 42
+
+ @SideEffectFree
+ int doubled() { return value * 2 }
+ }
+ assert new A().doubled() == 84
+ '''
+ }
+
+ // === @Contract(pure=true) implies @Modifies({}) ===
+
+ @Test
+ void contract_pure_implies_empty_modifies() {
+ def err = shouldFail shell, '''
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ int count = 0
+
+ @Contract(pure = true)
+ int broken() {
+ count++
+ return count
+ }
+ }
+ '''
+ assert err.message.contains('@Modifies violation')
+ }
+
+ // === @Contract(mutates=...) on callees ===
+
+ @Test
+ void contract_mutates_empty_is_safe() {
+ assertScript shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ int count = 0
+
+ @Modifies({ this.count })
+ void increment() {
+ count = helper(count)
+ }
+
+ @Contract(mutates = "")
+ int helper(int x) { return x + 1 }
+ }
+ def a = new A()
+ a.increment()
+ assert a.count == 1
+ '''
+ }
+
+ @Test
+ void contract_mutates_this_warns_on_callee() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ int count = 0
+ int cache = 0
+
+ @Modifies({ this.count })
+ void increment() {
+ count++
+ updateCache()
+ }
+
+ @Contract(mutates = "this")
+ void updateCache() { cache = count * 2 }
+ }
+ '''
+ assert err.message.contains('@Modifies warning')
+ assert err.message.contains('mutates')
+ }
+
+ @Test
+ void contract_mutates_param_warns_when_arg_not_in_modifies() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ List items = []
+ List other = []
+
+ @Modifies({ this.items })
+ void process() {
+ addToList(other, 'x')
+ }
+
+ @Contract(mutates = "param1")
+ static void addToList(List list, String item) { list.add(item)
}
+ }
+ '''
+ assert err.message.contains('@Modifies warning')
+ assert err.message.contains("'other'")
+ }
+
+ @Test
+ void contract_mutates_param_passes_when_arg_in_modifies() {
+ assertScript shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ List items = []
+
+ @Modifies({ this.items })
+ void process() {
+ addToList(items, 'x')
+ }
+
+ @Contract(mutates = "param1")
+ static void addToList(List list, String item) { list.add(item)
}
+ }
+ def a = new A()
+ a.process()
+ assert a.items == ['x']
+ '''
+ }
+
+ // === Callee recognition of @SideEffectFree and @Contract(pure=true) ===
+
+ @Test
+ void callee_with_side_effect_free_is_safe() {
+ assertScript shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.RUNTIME)
+ @Target(ElementType.METHOD)
+ @interface SideEffectFree {}
+
+ class A {
+ int count = 0
+
+ @Modifies({ this.count })
+ void increment() {
+ count = helper(count)
+ }
+
+ @SideEffectFree
+ int helper(int x) { return x + 1 }
+ }
+ def a = new A()
+ a.increment()
+ assert a.count == 1
+ '''
+ }
+
+ @Test
+ void callee_with_contract_pure_is_safe() {
+ assertScript shell, '''
+ import groovy.contracts.*
+ import java.lang.annotation.*
+
+ @Retention(RetentionPolicy.CLASS)
+ @Target(ElementType.METHOD)
+ @interface Contract {
+ String value() default ""
+ boolean pure() default false
+ String mutates() default ""
+ }
+
+ class A {
+ int count = 0
+
+ @Modifies({ this.count })
+ void increment() {
+ count = helper(count)
+ }
+
+ @Contract(pure = true)
+ int helper(int x) { return x + 1 }
+ }
+ def a = new A()
+ a.increment()
+ assert a.count == 1
+ '''
+ }
}
