[jira] [Commented] (GROOVY-12132) groovyc: print collected warnings on successful compilation (make -w effective)

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GROOVY-12132?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18095455#comment-18095455 ] 

ASF GitHub Bot commented on GROOVY-12132:
-----------------------------------------

Copilot commented on code in PR #2667:
URL: https://github.com/apache/groovy/pull/2667#discussion_r3562835184


##########
src/test/groovy/org/codehaus/groovy/tools/FileSystemCompilerTest.java:
##########
@@ -94,6 +103,55 @@ public void testCommandLine() throws Exception {
         FileSystemCompiler.commandLineCompile(new String[] {"src/test/groovy/groovy/LittleClosureTest.groovy", "-d", dir.getPath()});
     }
 
+    // warnings collected during a SUCCESSFUL compile must reach the command-line user; the
+    // failure path already surfaces them alongside the errors
+    @Test
+    public void testWarningsAreDisplayedOnSuccessfulCompile() throws Exception {
+        String output = compileCapturingStderr("WarnDemo", warningEmittingConfiguration());
+        assertTrue(output.contains("spike warning marker"), "expected the collected warning on stderr but got: " + output);
+        assertTrue(output.contains("1 warning"), "expected a warning summary but got: " + output);
+    }
+
+    @Test
+    public void testCleanCompilePrintsNothingToStderr() throws Exception {
+        String output = compileCapturingStderr("CleanDemo", null);
+        assertTrue(output.isEmpty(), "clean compile should stay silent but got: " + output);
+    }
+
+    private static CompilerConfiguration warningEmittingConfiguration() {
+        CompilerConfiguration configuration = new CompilerConfiguration();
+        configuration.addCompilationCustomizers(new CompilationCustomizer(CompilePhase.CANONICALIZATION) {
+            @Override
+            public void call(SourceUnit source, GeneratorContext context, ClassNode classNode) {
+                source.getErrorCollector().addWarning(WarningMessage.LIKELY_ERRORS, "spike warning marker",
+                        Token.newString(classNode.getName(), 1, 1), source);
+            }
+        });
+        return configuration;
+    }
+
+    private static String compileCapturingStderr(String className, CompilerConfiguration configuration) throws Exception {
+        File dir = new File("build/test-generated-classes/warn");
+        dir.mkdirs();
+        File source = new File(dir, className + ".groovy");
+        Files.write(source.toPath(), ("class " + className + " { }").getBytes());
+
+        if (configuration == null) {
+            configuration = new CompilerConfiguration();
+        }
+        configuration.setTargetDirectory(dir);
+
+        PrintStream originalErr = System.err;
+        ByteArrayOutputStream captured = new ByteArrayOutputStream();
+        try {
+            System.setErr(new PrintStream(captured, true));
+            FileSystemCompiler.doCompilation(configuration, null, new String[]{source.getPath()}, false);
+        } finally {
+            System.setErr(originalErr);
+        }
+        return captured.toString();
+    }

Review Comment:
   The stderr-capturing helper leaks resources and is platform-encoding dependent (`getBytes()`/`toString()` use the default charset). It also reuses a fixed output directory without cleanup, which can cause flaky behavior across repeated/parallel test runs. Prefer a per-test temp directory under `build/`, explicit UTF-8, and try-with-resources for the capturing stream.



##########
src/main/java/org/codehaus/groovy/tools/FileSystemCompiler.java:
##########
@@ -298,6 +300,7 @@ public static void doCompilation(CompilerConfiguration configuration, Compilatio
                         .setResourceLoader(filename -> null);
             }
             compiler.compile(filenames);
+            displayWarnings(compiler.unit);

Review Comment:
   `doCompilation` is used by non-command-line callers (e.g., the Ant `Groovyc` task) and printing warnings directly to `System.err` changes that API’s behavior by bypassing the caller’s logging/diagnostic routing. This can lead to unexpected console output for Ant builds even when they handle errors via `ErrorReporter`.





> groovyc: print collected warnings on successful compilation (make -w effective)
> -------------------------------------------------------------------------------
>
>                 Key: GROOVY-12132
>                 URL: https://issues.apache.org/jira/browse/GROOVY-12132
>             Project: Groovy
>          Issue Type: Improvement
>            Reporter: Paul King
>            Priority: Major
>
> h2. Code changes
> Two files:
> * {{src/main/java/org/codehaus/groovy/tools/FileSystemCompiler.java}} — after a successful {{compile()}} in {{doCompilation(...)}}, a new private {{displayWarnings(unit)}} writes any collected warnings to {{System.err}} via the existing {{ErrorCollector.write(PrintWriter, Janitor)}}, i.e. the identical diagnostic format already used when compilation *fails* (message, source line, caret, {{"N warning(s)"}} summary). The collector's contents are already filtered by the configured warning level at collection time, so no additional gating is needed.
> * {{src/test/groovy/org/codehaus/groovy/tools/FileSystemCompilerTest.java}} — two tests (written failing-first): a warning injected via a {{CompilationCustomizer}} must appear on stderr after a *successful* compile; a clean compile must print nothing.
> Behaviour notes:
> * This makes the long-documented {{-w}}/{{--warningLevel}} option effective: it is described as "the amount of warnings to *print*", but until now nothing ever printed collected warnings on success — the option only changed what was collected and then discarded.
> * {{doCompilation}} is also the entry point for Ant's {{<groovyc>}} in-process compiles, so Ant builds will start seeing collected warnings on stderr.
> * {{GroovyMain}} (the {{groovy}} script runner) has the same discard-on-success behaviour and is deliberately *not* changed here — printing warnings on every script run is a noisier proposition, worth its own discussion.
> * Found in passing, not addressed here: {{groovyc}} constructs {{new CompilerConfiguration()}} and therefore ignores {{groovy.warnings}} and the other documented {{groovy.*}} system properties, while {{groovy}} honours them via {{new CompilerConfiguration(System.getProperties())}}.
> Example ({{@TupleConstructor(force=true)}} generating a duplicate constructor; the warning is level 2, so silent at the default {{-w 1}}):
> {noformat}
> $ groovyc -w 2 Dup.groovy
> warning:    @groovy.transform.TupleConstructor(force = true)
>    ^
> Dup.groovy: 1: @TupleConstructor specifies duplicate constructor: P(java.lang.String)
> 1 warning
> {noformat}
> h2. Assessment of current warning usage
> All warnings funnel into {{ErrorCollector.addWarning(...)}}, which filters by the configured level *at collection time*. {{SourceUnit.addWarning(text, node)}} is a convenience wrapper that hardcodes {{POSSIBLE_ERRORS}} (2), and {{ModuleNode.getContext()}} returns the {{SourceUnit}}, so every {{getContext().addWarning(...)}} caller inherits level 2 whether it intended to or not.
> The inventory is small: ten emission sites in core, none in any subproject, none in the parser, and no compile-time deprecation warnings at all. {{PARANOIA}} (3) has no emitters. Prior to GROOVY-12131, exactly one stock warning was emitted at a level visible by default — and that one passes null context/source, so it renders without file/line information.
> || Site || Trigger || Level today || Proposed || Rationale ||
> | {{ClosureSignatureHint.findClassNode}} | {{@ClosureParams}} hint class name uses generics — "... doesn't support generics" | 1 | 1 | already correct; but passes null context/source (existing TODO) — should gain a proper location before warnings become visible |
> | {{OptimizerVisitor}} (GROOVY-12131) | cached constant node shared across classes | 1 | 1 | true positive implies miscompilation |
> | {{ModuleNode}} (~789) | multiple valid {{main}} variants; lower-priority one unreachable from the Groovy runner | 2 | 1 | a dead entry point is almost certainly unintended |
> | {{Verifier:469}} (GROOVY-11508) | *trait* implemented more than once with different generic arguments | 2 | 1 | the interface equivalent is a hard error; the trait case was only demoted to keep code compiling |
> | {{Verifier:992}} | generated property accessor cannot override a final method | 2 | 1 | the accessor is silently not generated as an override — surprising behaviour |
> | {{Verifier:1236}} | default-argument expansion collides with an explicit constructor | 2 | 1 | a generated-ctor collision is a probable mistake (mirrors the {{@TupleConstructor}} case) |
> | {{TupleConstructorASTTransformation:342}} | {{@TupleConstructor}} generates a duplicate constructor | 2 | 1 | as above |
> | {{ASTTransformationVisitor:329}} | global transform class declared in two descriptor files (first wins) | 2 | 2 | commonly caused by benign duplicate jars on messy classpaths; promoting would be noisy |
> | {{ASTTransformationVisitor:340}} | {{URISyntaxException}} comparing those descriptor URLs | 2 | 2 | rare internal oddity |
> | {{ASTTransformationVisitor:389}} | declared global transform class lacks {{@GroovyASTTransformation}} — "may fail and cause the compilation to fail" | 2 | 1 | the message itself says it is a likely error |
> | {{StaticTypeCheckingVisitor:750}} (GROOVY-11360) | field hidden by a dynamic property under {{@TypeChecked}} | 2 | 1 | user opted into strictness; silent hiding is exactly what they asked to be told about |
> Additional cleanups that pair with recalibration:
> * The three {{ASTTransformationVisitor}} sites and {{ClosureSignatureHint}} pass null context and/or null source unit — they predate anyone reading the output; their rendering should be checked/fixed since they may be among the first warnings users ever see.
> * {{SourceUnit.addWarning(text, node)}} offers no way to choose a level, quietly biasing new warnings toward invisibility; consider an overload taking an importance.
> h2. Compatibility
> No behavioural change for error-free, warning-free compiles (the common case) — stderr stays empty. Builds that treat any stderr output as failure could notice newly visible warnings; Groovy has no {{-Werror}}-style flag, so exit codes are unaffected. If level recalibration lands with this change, the promoted sites above become visible at the default {{-w 1}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)