[
https://issues.apache.org/jira/browse/GROOVY-12132?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12132:
-------------------------------
Description:
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}}.
> 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)
