diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 06f8e34c309..b9905a42381 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -342,6 +342,32 @@ which compiles a type checking DSL script — therefore pass `false`. configuration for a nested compilation**, and null-check the `SourceUnit` in any customizer that might be applied to one. +### Groovy 6 — method-level type-checking annotations override class-level SKIP (GROOVY-12292) + +A method (or constructor) whose own `@TypeChecked` or `@CompileStatic` +annotation has the default non-`SKIP` mode is now type checked — and for +`@CompileStatic`, statically compiled — even when its declaring class is +annotated with `@CompileDynamic`, `@CompileStatic(TypeCheckingMode.SKIP)` +or `@TypeChecked(TypeCheckingMode.SKIP)`. Previously the class-level +`SKIP` silently won and the method-level annotation was ignored. Nested +classes already behaved this way (GROOVY-10238); this aligns methods +with them: the most specific annotation wins, and a class-level `SKIP` +is the default only for members without their own annotation. + +**Who is affected.** Code with a method-level opt-in under a class-level +opt-out. Such methods may now raise type-checking errors that were +previously not reported, and their bodies are statically compiled where +they were previously dynamic. Remove the method-level annotation (or +change it to `SKIP` mode) to retain the old behaviour. + +**What is unchanged.** The opt-out direction is untouched: +`@CompileDynamic` / `SKIP`-mode methods inside checked classes are +skipped exactly as before. Cross-family behaviour is also unchanged: a +method-level `@CompileStatic(TypeCheckingMode.SKIP)` disables static +compilation but does not exempt the method from an enclosing class's +`@TypeChecked` checking; only `@TypeChecked(TypeCheckingMode.SKIP)` +does that. + ## The binary-compatibility check The [`subprojects/binary-compatibility/`](subprojects/binary-compatibility) 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 5f2e7861e86..cc9337d55ed 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -603,9 +603,14 @@ protected boolean shouldSkipMethodNode(final MethodNode node) { /** * Indicates whether the annotated node is configured for {@link TypeCheckingMode#SKIP}. + *

+ * The most specific annotation wins: a node whose own type-checking annotation has a + * non-SKIP mode is never skipped, whatever an enclosing class or method declares. + * Without an annotation of its own, a node inherits skip mode from its enclosing scope. */ public boolean isSkipMode(final AnnotatedNode node) { if (node == null) return false; + boolean explicitNonSkip = false; for (ClassNode tca : getTypeCheckingAnnotations()) { List annotations = node.getAnnotations(tca); if (annotations != null) { @@ -618,9 +623,11 @@ public boolean isSkipMode(final AnnotatedNode node) { if (TypeCheckingMode.SKIP.toString().equals(pe.getPropertyAsString())) return true; } } + explicitNonSkip = true; } } } + if (explicitNonSkip) return false; // GROOVY-12292 if (node instanceof MethodNode) { return isSkipMode(node.getDeclaringClass()); } diff --git a/src/spec/doc/core-semantics.adoc b/src/spec/doc/core-semantics.adoc index 5c1d71013d1..6564b49593c 100644 --- a/src/spec/doc/core-semantics.adoc +++ b/src/spec/doc/core-semantics.adoc @@ -1326,6 +1326,13 @@ type checker would normally complain and compilation would fail. Since the metho `TypeCheckingMode.SKIP`, type checking is _skipped_ for this method, so the code will compile, even if the rest of the class is type checked. +The most specific annotation wins: a method or nested class whose own `@TypeChecked` or `@CompileStatic` annotation +has the default (non-`SKIP`) mode is checked even when an enclosing class is marked with `SKIP` mode (or with +`@CompileDynamic`). Conversely, a class-level `SKIP` acts as the default for members that carry no type-checking +annotation of their own. Note that the two annotations opt out of different things: `@CompileStatic(TypeCheckingMode.SKIP)` +(or `@CompileDynamic`) on a method disables static _compilation_ but does not exempt that method from an enclosing +class's `@TypeChecked` checking; only `@TypeChecked(TypeCheckingMode.SKIP)` does that. + The following sections describe the semantics of type checking in Groovy. ==== Type checking assignments diff --git a/src/test/groovy/groovy/transform/stc/TypeCheckingModeTest.groovy b/src/test/groovy/groovy/transform/stc/TypeCheckingModeTest.groovy index ea9666834dc..04d0d681d54 100644 --- a/src/test/groovy/groovy/transform/stc/TypeCheckingModeTest.groovy +++ b/src/test/groovy/groovy/transform/stc/TypeCheckingModeTest.groovy @@ -18,8 +18,11 @@ */ package groovy.transform.stc +import org.codehaus.groovy.control.MultipleCompilationErrorsException import org.junit.jupiter.api.Test +import static groovy.test.GroovyAssert.shouldFail + /** * Unit tests for static type checking : type checking mode. */ @@ -142,4 +145,48 @@ class TypeCheckingModeTest extends StaticTypeCheckingTestCase { } ''' } + + // GROOVY-12292 + @Test + void testTypeCheckedMethodInTypeCheckedSkipClass() { + def err = shouldFail(MultipleCompilationErrorsException) { + new GroovyShell().evaluate ''' + import groovy.transform.TypeChecked + import static groovy.transform.TypeCheckingMode.SKIP + + @TypeChecked(SKIP) + class C { + @TypeChecked + def m() { + "".toStrings() + } + } + new C() + ''' + } + assert err.message.contains('Cannot find matching method java.lang.String#toStrings()') + } + + // GROOVY-12292: @CompileStatic(SKIP) opts out of static compilation only; + // it does not exempt a method from an enclosing class's type checking + @Test + void testCompileStaticSkipMethodInTypeCheckedClassIsStillTypeChecked() { + def err = shouldFail(MultipleCompilationErrorsException) { + new GroovyShell().evaluate ''' + import groovy.transform.CompileStatic + import groovy.transform.TypeChecked + import static groovy.transform.TypeCheckingMode.SKIP + + @TypeChecked + class C { + @CompileStatic(SKIP) + def m() { + "".toStrings() + } + } + new C() + ''' + } + assert err.message.contains('Cannot find matching method java.lang.String#toStrings()') + } } diff --git a/src/test/groovy/org/codehaus/groovy/classgen/asm/sc/CompileDynamicTest.groovy b/src/test/groovy/org/codehaus/groovy/classgen/asm/sc/CompileDynamicTest.groovy index 9f8ad18fe18..4ce5473e5a8 100644 --- a/src/test/groovy/org/codehaus/groovy/classgen/asm/sc/CompileDynamicTest.groovy +++ b/src/test/groovy/org/codehaus/groovy/classgen/asm/sc/CompileDynamicTest.groovy @@ -64,4 +64,63 @@ final class CompileDynamicTest extends StaticTypeCheckingTestCase implements Sta new C() ''' } + + // GROOVY-12292 + @Test + void testCompileStaticMethodInCompileDynamicClass() { + shouldFailWithMessages ''' + @CompileDynamic + class C { + @CompileStatic + def m() { + "".toStrings() + } + } + ''', 'Cannot find matching method java.lang.String#toStrings()' + } + + // GROOVY-12292 + @Test + void testCompileStaticMethodInCompileStaticSkipClass() { + shouldFailWithMessages ''' + @CompileStatic(groovy.transform.TypeCheckingMode.SKIP) + class C { + @CompileStatic + def m() { + "".toStrings() + } + } + ''', 'Cannot find matching method java.lang.String#toStrings()' + } + + // GROOVY-12292 + @Test + void testCompileStaticMethodInTypeCheckedSkipClass() { + shouldFailWithMessages ''' + @groovy.transform.TypeChecked(groovy.transform.TypeCheckingMode.SKIP) + class C { + @CompileStatic + def m() { + "".toStrings() + } + } + ''', 'Cannot find matching method java.lang.String#toStrings()' + } + + // GROOVY-12292 + @Test + void testCompileStaticMethodInCompileDynamicClassIsStaticallyCompiled() { + assertScript ''' + @CompileDynamic + class C { + @CompileStatic + String m() { + 'works'.toUpperCase() + } + } + assert new C().m() == 'WORKS' + ''' + String bytecode = astTrees['C'][1] + assert bytecode.contains('INVOKEVIRTUAL java/lang/String.toUpperCase ()Ljava/lang/String;') + } }