Copilot commented on code in PR #2536: URL: https://github.com/apache/groovy/pull/2536#discussion_r3252231663
########## subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/CombinerChecker.groovy: ########## @@ -0,0 +1,344 @@ +/* + * 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.typecheckers + +import org.apache.groovy.lang.annotation.Incubating +import org.apache.groovy.typecheckers.CheckingVisitor +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.CodeVisitorSupport +import org.codehaus.groovy.ast.MethodNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.MethodPointerExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.codehaus.groovy.transform.stc.StaticTypesMarker + +/** + * A compile-time checker that verifies the combiner passed to an + * order-independent parallel reduction ({@code sumParallel}, + * {@code injectParallel}) carries the associativity contract those methods + * silently require — mirroring the design of {@code PurityChecker}. + * <p> + * Associativity is undecidable in general, so this checker is deliberately + * conservative. It has two modes, selected via the extension option + * {@code mode}: + * <pre> + * // default (lenient): only flag high-confidence problems + * {@code @TypeChecked(extensions = 'groovy.typecheckers.CombinerChecker')} + * + * // strict: additionally require the combiner to be declared @Associative/@Reducer + * {@code @TypeChecked(extensions = "groovy.typecheckers.CombinerChecker(mode: 'strict')")} + * </pre> + * <p> + * What it does, by combiner shape: + * <ul> + * <li><b>Method reference</b> to a method annotated {@link groovy.transform.Associative} + * or {@link groovy.transform.Reducer} — accepted.</li> + * <li><b>Inline closure</b> whose combine expression applies a non-associative + * operator ({@code -}, {@code /}, {@code %}) directly to the two combiner + * parameters (e.g. <code>{ a, b {@literal ->} a - b }</code>) — flagged in + * <em>any</em> mode (high confidence).</li> + * <li><b>{@code injectParallel}</b> seed that contradicts a {@code @Reducer}'s + * declared {@code zero()} (both statically constant) — flagged.</li> + * <li>Un-annotated method references and other inline closures — flagged + * only in <b>strict</b> mode (a declared, verifiable contract is required).</li> + * </ul> Review Comment: The Javadoc says lenient mode flags only `-`, `/`, `%` for inline combiners, but `NON_ASSOCIATIVE_OPS` (and the tests) include `**` as well. Please update the Javadoc description (and/or the operator set) so the documented behavior matches what the checker actually enforces. ########## subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc: ########## @@ -1241,3 +1250,141 @@ Both checkers can be used independently or together: @TypeChecked(extensions = ['groovy.typecheckers.ModifiesChecker', 'groovy.typecheckers.PurityChecker']) ---- + +== Checking @Associative/@Reducer Combiners (Incubating) + +The parallel `Collection` reductions split, reorder, and recombine their input. +That is only correct when the combining function is *associative* -- +`combine(a, combine(b, c))` must equal `combine(combine(a, b), c)` -- and, for +the seeded `injectParallel`, when the seed is an *identity* for the combiner. +A non-associative combiner (subtraction, division, order-sensitive +concatenation) compiles and runs, but produces a different, non-deterministic +result depending on how the work was partitioned. This is among the hardest +bug classes to detect by testing alone. Review Comment: This section describes “order-sensitive concatenation” as non-associative, but concatenation is typically associative (it’s order-dependent/non-commutative, which is a different property). Consider revising the examples/wording to avoid conflating non-associativity with order-dependence, especially since the checker is about associativity/identity rather than commutativity or encounter-order guarantees. ########## subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc: ########## @@ -1241,3 +1250,141 @@ Both checkers can be used independently or together: @TypeChecked(extensions = ['groovy.typecheckers.ModifiesChecker', 'groovy.typecheckers.PurityChecker']) ---- + +== Checking @Associative/@Reducer Combiners (Incubating) + +The parallel `Collection` reductions split, reorder, and recombine their input. +That is only correct when the combining function is *associative* -- +`combine(a, combine(b, c))` must equal `combine(combine(a, b), c)` -- and, for +the seeded `injectParallel`, when the seed is an *identity* for the combiner. +A non-associative combiner (subtraction, division, order-sensitive +concatenation) compiles and runs, but produces a different, non-deterministic +result depending on how the work was partitioned. This is among the hardest +bug classes to detect by testing alone. + +`@Associative` and `@Reducer` (in `groovy.transform`) let a combiner *declare* +the contract; `CombinerChecker` verifies, at compile time, that combiners +reaching `sumParallel`/`injectParallel` carry it. + +[source,groovy] +---- +import groovy.transform.Associative +import groovy.transform.Reducer + +class Maths { + @Associative static int add(int a, int b) { a + b } + @Reducer(zero = '0') static int plus(int a, int b) { a + b } +} +---- + +=== Modes + +Associativity is undecidable in general, so the checker is deliberately +conservative and false-positive averse (a type-checking extension has only an +error channel). + +`default` (lenient):: flags only high-confidence problems -- a non-associative +operator (`-`, `/`, `%`) applied directly to the two combiner parameters, and a +`injectParallel` seed that contradicts a `@Reducer`'s declared `zero()`. An +ordinary associative inline closure such as `{ a, b -> a + b }` is never Review Comment: The “default (lenient)” mode description lists `-`, `/`, `%` as the non-associative operators flagged, but the checker/test suite also flags `**`. Update this section so users don’t miss that exponentiation is included in the denylist. ########## subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/CombinerChecker.groovy: ########## @@ -0,0 +1,344 @@ +/* + * 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.typecheckers + +import org.apache.groovy.lang.annotation.Incubating +import org.apache.groovy.typecheckers.CheckingVisitor +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.CodeVisitorSupport +import org.codehaus.groovy.ast.MethodNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.MethodPointerExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.codehaus.groovy.transform.stc.StaticTypesMarker + +/** + * A compile-time checker that verifies the combiner passed to an + * order-independent parallel reduction ({@code sumParallel}, + * {@code injectParallel}) carries the associativity contract those methods + * silently require — mirroring the design of {@code PurityChecker}. + * <p> + * Associativity is undecidable in general, so this checker is deliberately + * conservative. It has two modes, selected via the extension option + * {@code mode}: + * <pre> + * // default (lenient): only flag high-confidence problems + * {@code @TypeChecked(extensions = 'groovy.typecheckers.CombinerChecker')} + * + * // strict: additionally require the combiner to be declared @Associative/@Reducer + * {@code @TypeChecked(extensions = "groovy.typecheckers.CombinerChecker(mode: 'strict')")} + * </pre> + * <p> + * What it does, by combiner shape: + * <ul> + * <li><b>Method reference</b> to a method annotated {@link groovy.transform.Associative} + * or {@link groovy.transform.Reducer} — accepted.</li> + * <li><b>Inline closure</b> whose combine expression applies a non-associative + * operator ({@code -}, {@code /}, {@code %}) directly to the two combiner + * parameters (e.g. <code>{ a, b {@literal ->} a - b }</code>) — flagged in + * <em>any</em> mode (high confidence).</li> + * <li><b>{@code injectParallel}</b> seed that contradicts a {@code @Reducer}'s + * declared {@code zero()} (both statically constant) — flagged.</li> + * <li>Un-annotated method references and other inline closures — flagged + * only in <b>strict</b> mode (a declared, verifiable contract is required).</li> + * </ul> + * The default mode never errors on an ordinary associative inline closure such + * as <code>{ a, b {@literal ->} a + b }</code>; the single error channel of a + * type-checking extension makes false-positive aversion essential. + * + * @since 6.0.0 + * @see groovy.transform.Associative + * @see groovy.transform.Reducer + */ +@Incubating +class CombinerChecker extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + /** GDK extension class providing the parallel reductions we guard. */ + private static final String PARALLEL_EXT = 'org.codehaus.groovy.runtime.ParallelCollectionExtensions' + + /** Reductions whose correctness depends on an associative combiner. */ + private static final Set<String> SEEDED_REDUCTIONS = Set.of('injectParallel') + private static final Set<String> UNSEEDED_REDUCTIONS = Set.of('sumParallel') + + private static final Set<String> COMBINER_ANNOS = Set.of('Associative', 'Reducer') + + /** + * Non-associative binary operators (also non-commutative). Matched purely + * by operator token, assuming conventional operator meaning: it does not + * resolve overloaded operators. Notably, overloading a normally-associative + * operator (e.g. {@code +}, {@code *}) to be non-associative is poor style + * and an explicit non-goal — not detected by design. The declaration paths + * (@Associative/@Reducer, Monoid/Semigroup) remain authoritative. + */ + private static final Set<String> NON_ASSOCIATIVE_OPS = Set.of('-', '/', '%', '**') + + // Simple type names that *carry* the associativity contract by construction. + // A Monoid additionally carries an identity; a Semigroup does not. These are + // matched by simple name (no dependency on Functional Java / Palatable / + // Purefun — all converge on these names), and are extensible via the + // 'monoids'/'semigroups' extension options. + private static final Set<String> DEFAULT_MONOID_TYPES = Set.of('Monoid') + private static final Set<String> DEFAULT_SEMIGROUP_TYPES = Set.of('Semigroup') + + private boolean strict + private Set<String> monoidTypes + private Set<String> semigroupTypes + + @Override + Object run() { + strict = (options?.mode as String)?.equalsIgnoreCase('strict') + monoidTypes = DEFAULT_MONOID_TYPES + parsePipe(options?.monoids as String) + semigroupTypes = DEFAULT_SEMIGROUP_TYPES + parsePipe(options?.semigroups as String) + // Mirror PurityChecker's proven architecture: visit method bodies with a + // CheckingVisitor AIC and do the work there. The afterMethodCall event + // hook has an unreliable dispatch context, and a primitive-typed + // makeVisitor(boolean) arg also fails dynamic dispatch in the rewritten + // DSL context — so makeVisitor is no-arg and reads a field. Spike finding. + afterVisitMethod { MethodNode mn -> + mn.code?.visit(makeVisitor()) + } + } + + private CheckingVisitor makeVisitor() { + boolean strict = this.strict + Set<String> monoidTypes = this.monoidTypes + Set<String> semigroupTypes = this.semigroupTypes + new CheckingVisitor() { + + /** Owner type of a method-ref combiner, or of the receiver a thin + * delegating closure forwards to; null otherwise. Uses getType, + * which resolves inside the AIC (cf. PurityChecker). */ + private ClassNode carrierOwner(Expression combiner) { + if (combiner instanceof MethodPointerExpression) { + return safeType(((MethodPointerExpression) combiner).expression) + } + if (combiner instanceof ClosureExpression) { + ClosureExpression c = (ClosureExpression) combiner + Set<String> ps = (c.parameters ?: []).collect { it.name } as Set<String> + if (ps.size() == 2 && c.code != null) { + final MethodCallExpression[] hit = [null] + c.code.visit(new CodeVisitorSupport() { + @Override + void visitMethodCallExpression(MethodCallExpression mce) { + super.visitMethodCallExpression(mce) + def a = (mce.arguments instanceof ArgumentListExpression) ? + ((ArgumentListExpression) mce.arguments).expressions : [] + if (hit[0] == null && a.size() == 2 && + CombinerChecker.isParamRef(a[0], ps) && + CombinerChecker.isParamRef(a[1], ps)) { + hit[0] = mce + } + } + }) + if (hit[0] != null) return safeType(hit[0].objectExpression) + } + } + null + } + + private ClassNode safeType(Expression e) { + try { getType(e) } catch (ignored) { null } + } + + @Override + void visitMethodCallExpression(MethodCallExpression call) { + super.visitMethodCallExpression(call) + String name = call.methodAsString + def target = call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET) + String dc = (target instanceof MethodNode) ? + ((MethodNode) target).declaringClass?.name : null + + // GDK parallel reductions: name-based (extension methods do not + // expose a resolvable DIRECT_METHOD_CALL_TARGET, cf. PurityChecker + // matching by simple name). If a target IS resolvable and belongs + // to an unrelated user class, skip (avoid hijacking same-named). + boolean isGdk = (name in SEEDED_REDUCTIONS) || (name in UNSEEDED_REDUCTIONS) + if (isGdk && dc && dc != PARALLEL_EXT && !dc.startsWith('java.') && + !dc.startsWith('org.codehaus.groovy.runtime')) return + + // JDK stream reduction: java.util.stream.{Stream,IntStream,...}#reduce. + // Unlike GDK extensions these resolve to a real interface method, + // so a receiver-type gate works (and is needed — 'reduce' is a + // generic name). reduce's contract requires associativity even + // for sequential streams, so flagging is spec-correct regardless + // of .parallel(). cf. RegexChecker gating on java.util.regex. + boolean isStreamReduce = name == 'reduce' && dc != null && + dc.startsWith('java.util.stream.') + if (!isGdk && !isStreamReduce) return + + List<Expression> args = (call.arguments instanceof ArgumentListExpression) ? + ((ArgumentListExpression) call.arguments).expressions : [] + if (args.isEmpty()) return + + // GDK injectParallel always carries a seed; sumParallel never. + // Stream.reduce has an identity iff >= 2 args (reduce(id, acc) / + // reduce(id, acc, combiner)). The associativity-critical function + // is always the last argument (the merge BinaryOperator for the + // 3-arg form; the 2-arg accumulator BiFunction is not analysed). + boolean seeded = isGdk ? (name in SEEDED_REDUCTIONS) : (args.size() >= 2) + + Expression combiner = args[-1] + Expression seed = seeded ? args[0] : null + + String carrier = classify(carrierOwner(combiner), monoidTypes, semigroupTypes) + String msg = diagnose(combiner, seed, name, strict, carrier, seeded) + if (msg) addStaticTypeError(msg, call) + } + } + } + + /** Pure static analysis: returns a diagnostic message, or null if acceptable. */ + private static String diagnose(Expression combiner, Expression seed, String name, + boolean strict, String carrier, boolean seeded) { + // A Monoid/Semigroup instance *is* the associativity contract (same + // trust level as @Associative — an assertion carrier, not a proof). + if (carrier == 'SEMIGROUP') { + if (seeded) { + return "CombinerChecker: '${name}' is seeded but the combiner is a Semigroup, " + + "which carries no identity element. A seeded parallel reduction needs a " + + "Monoid (its identity must match the seed)." + } + return null // Semigroup + unseeded sumParallel: associativity carried, OK + } + if (carrier == 'MONOID') { + return null // associativity + identity carried by the Monoid + } + if (combiner instanceof ClosureExpression) { + return closureMsg((ClosureExpression) combiner, name, strict) + } Review Comment: `diagnose` short-circuits to accept MONOID/SEMIGROUP carriers before running any inline-closure analysis. That means a closure combiner that happens to call a Monoid/Semigroup method with both parameters can bypass the high-confidence non-associative operator check (e.g. it could still contain `a - b`). Consider running `closureMsg`’s bad-operator scan even when a carrier is detected, or tightening carrier detection so only a truly “thin delegating closure” qualifies for the carrier fast-path. ########## subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/CombinerChecker.groovy: ########## @@ -0,0 +1,344 @@ +/* + * 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.typecheckers + +import org.apache.groovy.lang.annotation.Incubating +import org.apache.groovy.typecheckers.CheckingVisitor +import org.codehaus.groovy.ast.ClassNode +import org.codehaus.groovy.ast.CodeVisitorSupport +import org.codehaus.groovy.ast.MethodNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.MethodPointerExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport +import org.codehaus.groovy.transform.stc.StaticTypesMarker + +/** + * A compile-time checker that verifies the combiner passed to an + * order-independent parallel reduction ({@code sumParallel}, + * {@code injectParallel}) carries the associativity contract those methods + * silently require — mirroring the design of {@code PurityChecker}. + * <p> + * Associativity is undecidable in general, so this checker is deliberately + * conservative. It has two modes, selected via the extension option + * {@code mode}: + * <pre> + * // default (lenient): only flag high-confidence problems + * {@code @TypeChecked(extensions = 'groovy.typecheckers.CombinerChecker')} + * + * // strict: additionally require the combiner to be declared @Associative/@Reducer + * {@code @TypeChecked(extensions = "groovy.typecheckers.CombinerChecker(mode: 'strict')")} + * </pre> + * <p> + * What it does, by combiner shape: + * <ul> + * <li><b>Method reference</b> to a method annotated {@link groovy.transform.Associative} + * or {@link groovy.transform.Reducer} — accepted.</li> + * <li><b>Inline closure</b> whose combine expression applies a non-associative + * operator ({@code -}, {@code /}, {@code %}) directly to the two combiner + * parameters (e.g. <code>{ a, b {@literal ->} a - b }</code>) — flagged in + * <em>any</em> mode (high confidence).</li> + * <li><b>{@code injectParallel}</b> seed that contradicts a {@code @Reducer}'s + * declared {@code zero()} (both statically constant) — flagged.</li> + * <li>Un-annotated method references and other inline closures — flagged + * only in <b>strict</b> mode (a declared, verifiable contract is required).</li> + * </ul> + * The default mode never errors on an ordinary associative inline closure such + * as <code>{ a, b {@literal ->} a + b }</code>; the single error channel of a + * type-checking extension makes false-positive aversion essential. + * + * @since 6.0.0 + * @see groovy.transform.Associative + * @see groovy.transform.Reducer + */ +@Incubating +class CombinerChecker extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL { + + /** GDK extension class providing the parallel reductions we guard. */ + private static final String PARALLEL_EXT = 'org.codehaus.groovy.runtime.ParallelCollectionExtensions' + + /** Reductions whose correctness depends on an associative combiner. */ + private static final Set<String> SEEDED_REDUCTIONS = Set.of('injectParallel') + private static final Set<String> UNSEEDED_REDUCTIONS = Set.of('sumParallel') + + private static final Set<String> COMBINER_ANNOS = Set.of('Associative', 'Reducer') + + /** + * Non-associative binary operators (also non-commutative). Matched purely + * by operator token, assuming conventional operator meaning: it does not + * resolve overloaded operators. Notably, overloading a normally-associative + * operator (e.g. {@code +}, {@code *}) to be non-associative is poor style + * and an explicit non-goal — not detected by design. The declaration paths + * (@Associative/@Reducer, Monoid/Semigroup) remain authoritative. + */ + private static final Set<String> NON_ASSOCIATIVE_OPS = Set.of('-', '/', '%', '**') + + // Simple type names that *carry* the associativity contract by construction. + // A Monoid additionally carries an identity; a Semigroup does not. These are + // matched by simple name (no dependency on Functional Java / Palatable / + // Purefun — all converge on these names), and are extensible via the + // 'monoids'/'semigroups' extension options. + private static final Set<String> DEFAULT_MONOID_TYPES = Set.of('Monoid') + private static final Set<String> DEFAULT_SEMIGROUP_TYPES = Set.of('Semigroup') + + private boolean strict + private Set<String> monoidTypes + private Set<String> semigroupTypes + + @Override + Object run() { + strict = (options?.mode as String)?.equalsIgnoreCase('strict') + monoidTypes = DEFAULT_MONOID_TYPES + parsePipe(options?.monoids as String) + semigroupTypes = DEFAULT_SEMIGROUP_TYPES + parsePipe(options?.semigroups as String) + // Mirror PurityChecker's proven architecture: visit method bodies with a + // CheckingVisitor AIC and do the work there. The afterMethodCall event + // hook has an unreliable dispatch context, and a primitive-typed + // makeVisitor(boolean) arg also fails dynamic dispatch in the rewritten + // DSL context — so makeVisitor is no-arg and reads a field. Spike finding. + afterVisitMethod { MethodNode mn -> + mn.code?.visit(makeVisitor()) + } + } + + private CheckingVisitor makeVisitor() { + boolean strict = this.strict + Set<String> monoidTypes = this.monoidTypes + Set<String> semigroupTypes = this.semigroupTypes + new CheckingVisitor() { + + /** Owner type of a method-ref combiner, or of the receiver a thin + * delegating closure forwards to; null otherwise. Uses getType, + * which resolves inside the AIC (cf. PurityChecker). */ + private ClassNode carrierOwner(Expression combiner) { + if (combiner instanceof MethodPointerExpression) { + return safeType(((MethodPointerExpression) combiner).expression) + } + if (combiner instanceof ClosureExpression) { + ClosureExpression c = (ClosureExpression) combiner + Set<String> ps = (c.parameters ?: []).collect { it.name } as Set<String> + if (ps.size() == 2 && c.code != null) { + final MethodCallExpression[] hit = [null] + c.code.visit(new CodeVisitorSupport() { + @Override + void visitMethodCallExpression(MethodCallExpression mce) { + super.visitMethodCallExpression(mce) + def a = (mce.arguments instanceof ArgumentListExpression) ? + ((ArgumentListExpression) mce.arguments).expressions : [] + if (hit[0] == null && a.size() == 2 && + CombinerChecker.isParamRef(a[0], ps) && + CombinerChecker.isParamRef(a[1], ps)) { + hit[0] = mce + } + } + }) + if (hit[0] != null) return safeType(hit[0].objectExpression) + } + } + null + } + + private ClassNode safeType(Expression e) { + try { getType(e) } catch (ignored) { null } + } + + @Override + void visitMethodCallExpression(MethodCallExpression call) { + super.visitMethodCallExpression(call) + String name = call.methodAsString + def target = call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET) + String dc = (target instanceof MethodNode) ? + ((MethodNode) target).declaringClass?.name : null + + // GDK parallel reductions: name-based (extension methods do not + // expose a resolvable DIRECT_METHOD_CALL_TARGET, cf. PurityChecker + // matching by simple name). If a target IS resolvable and belongs + // to an unrelated user class, skip (avoid hijacking same-named). + boolean isGdk = (name in SEEDED_REDUCTIONS) || (name in UNSEEDED_REDUCTIONS) + if (isGdk && dc && dc != PARALLEL_EXT && !dc.startsWith('java.') && + !dc.startsWith('org.codehaus.groovy.runtime')) return + + // JDK stream reduction: java.util.stream.{Stream,IntStream,...}#reduce. + // Unlike GDK extensions these resolve to a real interface method, + // so a receiver-type gate works (and is needed — 'reduce' is a + // generic name). reduce's contract requires associativity even + // for sequential streams, so flagging is spec-correct regardless + // of .parallel(). cf. RegexChecker gating on java.util.regex. + boolean isStreamReduce = name == 'reduce' && dc != null && + dc.startsWith('java.util.stream.') + if (!isGdk && !isStreamReduce) return + + List<Expression> args = (call.arguments instanceof ArgumentListExpression) ? + ((ArgumentListExpression) call.arguments).expressions : [] + if (args.isEmpty()) return + + // GDK injectParallel always carries a seed; sumParallel never. + // Stream.reduce has an identity iff >= 2 args (reduce(id, acc) / + // reduce(id, acc, combiner)). The associativity-critical function + // is always the last argument (the merge BinaryOperator for the + // 3-arg form; the 2-arg accumulator BiFunction is not analysed). + boolean seeded = isGdk ? (name in SEEDED_REDUCTIONS) : (args.size() >= 2) + + Expression combiner = args[-1] + Expression seed = seeded ? args[0] : null + + String carrier = classify(carrierOwner(combiner), monoidTypes, semigroupTypes) + String msg = diagnose(combiner, seed, name, strict, carrier, seeded) + if (msg) addStaticTypeError(msg, call) + } + } + } + + /** Pure static analysis: returns a diagnostic message, or null if acceptable. */ + private static String diagnose(Expression combiner, Expression seed, String name, + boolean strict, String carrier, boolean seeded) { + // A Monoid/Semigroup instance *is* the associativity contract (same + // trust level as @Associative — an assertion carrier, not a proof). + if (carrier == 'SEMIGROUP') { + if (seeded) { + return "CombinerChecker: '${name}' is seeded but the combiner is a Semigroup, " + + "which carries no identity element. A seeded parallel reduction needs a " + + "Monoid (its identity must match the seed)." + } + return null // Semigroup + unseeded sumParallel: associativity carried, OK + } + if (carrier == 'MONOID') { + return null // associativity + identity carried by the Monoid + } + if (combiner instanceof ClosureExpression) { + return closureMsg((ClosureExpression) combiner, name, strict) + } + if (combiner instanceof MethodPointerExpression) { + return methodRefMsg((MethodPointerExpression) combiner, seed, name, strict) + } + return strict ? "CombinerChecker: combiner passed to '${name}' cannot be statically " + + "verified; use a method annotated @Associative/@Reducer" : null + } + + // ---- inline closure combiner ---- + + private static String closureMsg(ClosureExpression closure, String name, boolean strict) { + Set<String> params = (closure.parameters ?: []).collect { it.name } as Set<String> + + // High-confidence only: a non-associative operator applied directly to + // the two combiner parameters (e.g. { a, b -> a - b }). Array holder: + // an anonymous visitor cannot mutate an enclosing local. + final boolean[] badOp = [false] + if (params.size() == 2 && closure.code != null) { + closure.code.visit(new CodeVisitorSupport() { + @Override + void visitBinaryExpression(BinaryExpression be) { + super.visitBinaryExpression(be) + if (be.operation.text in NON_ASSOCIATIVE_OPS && + isParamRef(be.leftExpression, params) && + isParamRef(be.rightExpression, params)) { + badOp[0] = true + } + } + }) + } + + if (badOp[0]) { + return "CombinerChecker: combiner passed to '${name}' applies a non-associative " + + "operator to its arguments; parallel reduction will be non-deterministic. " + + "Use an associative combiner." + } + return strict ? "CombinerChecker (strict): inline combiner passed to '${name}' is not " + + "declared @Associative/@Reducer; extract a named, annotated method." : null + } + + private static boolean isParamRef(Expression e, Set<String> params) { + e instanceof VariableExpression && ((VariableExpression) e).name in params + } + + // ---- method-reference combiner ---- + + private static String methodRefMsg(MethodPointerExpression ref, Expression seed, + String name, boolean strict) { + String refName = ref.methodName?.text + // Spike scope: class-qualified refs (Foo.&bar / Foo::bar) only; the + // owner type is read straight off the ClassExpression to avoid relying + // on getType() from a static context. Instance-bound refs fall through + // to the strict-only path. + def ownerType = (ref.expression instanceof ClassExpression) ? + ((ClassExpression) ref.expression).type?.redirect() : null + def overloads = (refName && ownerType) ? ownerType.getMethods(refName) : null + MethodNode resolved = overloads ? (overloads.find { hasCombinerAnno(it) } ?: overloads[0]) : null + + if (resolved != null && hasCombinerAnno(resolved)) { + return seedMismatchMsg(resolved, seed, name) + } + return strict ? "CombinerChecker (strict): combiner '${refName}' passed to '${name}' " + + "is not declared @Associative/@Reducer." : null + } Review Comment: `methodRefMsg` only resolves annotations for class-qualified method pointers (`Foo.&bar` / `Foo::bar`). Instance-bound method pointers to annotated instance methods fall through and will be rejected in strict mode. Either support instance-bound refs (e.g. via `getType(ref.expression)` and method lookup) or document this limitation explicitly (the class-level docs currently describe “method reference” in general). ########## subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/CombinerCheckerTest.groovy: ########## @@ -0,0 +1,270 @@ +/* + * 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.typecheckers + +import org.codehaus.groovy.control.CompilerConfiguration +import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer +import org.junit.jupiter.api.BeforeAll +import org.junit.jupiter.api.Test + +import static groovy.test.GroovyAssert.assertScript +import static groovy.test.GroovyAssert.shouldFail + +/** + * Spike tests for {@link CombinerChecker}. Mirrors the harness style of + * {@code PurityCheckerTest}. + */ +final class CombinerCheckerTest { + + private static GroovyShell lenientShell + private static GroovyShell strictShell + + @BeforeAll + static void setUp() { + lenientShell = makeShell(null) + strictShell = makeShell('strict') + } + + private static GroovyShell makeShell(String mode) { + String ext = mode ? "groovy.typecheckers.CombinerChecker(mode: '${mode}')" : 'groovy.typecheckers.CombinerChecker' + new GroovyShell(new CompilerConfiguration().tap { + def customizer = new ASTTransformationCustomizer(groovy.transform.TypeChecked) + customizer.annotationParameters = [extensions: ext] + addCompilationCustomizers(customizer) + }) + } + + // === lenient mode: only high-confidence problems are flagged === + + // NB: closure params are explicitly typed. Under @TypeChecked the param + // types of an inline combiner are not reliably inferred from the + // BinaryOperator SAM, so untyped params produce unrelated STC errors + // (Object#plus/#div) before this checker runs. Typing them isolates the + // checker's behaviour. (Spike finding: the annotated-method-reference + // path is the robust one; inline analysis is best-effort.) + + @Test + void associative_inline_closure_passes() { + assertScript lenientShell, ''' + assert [1, 2, 3, 4].sumParallel { int a, int b -> a + b } == 10 + assert [1, 2, 3, 4].injectParallel(0) { int a, int b -> a + b } == 10 + ''' + } + + @Test + void subtraction_inline_closure_fails() { + def err = shouldFail lenientShell, ''' + [1, 2, 3, 4].injectParallel(0) { int a, int b -> a - b } + ''' + assert err.message.contains('non-associative') + } + + @Test + void modulo_inline_closure_fails() { + def err = shouldFail lenientShell, ''' + [12, 7, 5].sumParallel { int a, int b -> a % b } + ''' + assert err.message.contains('non-associative') + } + + @Test + void power_inline_closure_fails() { + // also confirms '**' is a BinaryExpression with token text '**' + def err = shouldFail lenientShell, ''' + [2, 3, 2].injectParallel(1) { int a, int b -> (a ** b) as int } + ''' + assert err.message.contains('non-associative') + } + + @Test + void annotated_method_reference_passes() { + assertScript lenientShell, ''' + import groovy.transform.Associative + + class Maths { + @Associative + static int add(int a, int b) { a + b } + } + + assert [1, 2, 3, 4].injectParallel(0, Maths.&add) == 10 + ''' + } + + @Test + void reducer_seed_mismatch_fails() { + def err = shouldFail lenientShell, ''' + import groovy.transform.Reducer + + class Maths { + @Reducer(zero = '0') + static int add(int a, int b) { a + b } + } + + [1, 2, 3].injectParallel(5, Maths.&add) + ''' + assert err.message.contains('does not match') + } + + @Test + void reducer_seed_match_passes() { + assertScript lenientShell, ''' + import groovy.transform.Reducer + + class Maths { + @Reducer(zero = '0') + static int add(int a, int b) { a + b } + } + + assert [1, 2, 3].injectParallel(0, Maths.&add) == 6 + ''' + } + + // === strict mode: a declared, verifiable contract is required === + + @Test + void strict_unannotated_inline_closure_fails_even_when_associative() { + def err = shouldFail strictShell, ''' + [1, 2, 3].injectParallel(0) { a, b -> a + b } Review Comment: The header comment notes that untyped inline combiner parameters can trigger unrelated STC errors before this checker runs, but this strict-mode test uses an untyped closure (`{ a, b -> a + b }`). To keep the test stable and focused on CombinerChecker’s diagnostic, consider explicitly typing `a`/`b` (or otherwise asserting on the specific error you expect if STC fails first). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
