[
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)
