[
https://issues.apache.org/jira/browse/GROOVY-12306?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12306:
-------------------------------
Description:
The compiler's error tolerance -- the number of non-fatal errors accepted
before compilation bails out -- is only partially wired up. Three defects, all
verified against 6.0.0-beta-2:
h2. 1. Tolerance is not applied to static type checking errors
{{-t}} / {{--tolerance}} has no effect whatsoever on type checking errors, so
there is no way to ask for fail-fast behaviour on the most common error class
in {{@CompileStatic}} code.
{code:java}
@groovy.transform.CompileStatic
class Z {
def m0() { new Object().nope0() }
def m1() { new Object().nope1() }
// ... 14 such methods
}
{code}
||Command||Expected||Actual||
|{{groovyc -t 1 Z.groovy}}|1 error|14 errors|
|{{groovyc -t 3 Z.groovy}}|3 errors|14 errors|
Cause: only {{ErrorCollector.addError(Message)}} performs the {{errors.size()
>= configuration.getTolerance()}} check. {{ClassCodeVisitorSupport.addError}}
calls {{addErrorAndContinue}} instead, so every diagnostic raised through a
visitor bypasses the threshold, and {{StaticTypeCheckingVisitor}} overrides
{{addError}} to call the collector directly as well.
Errors raised through {{SourceUnit.addError}} *are* capped correctly, which
produces a confusing split: the same flag governs class generation errors but
silently does nothing for type checking errors.
h2. 2. {{-t 0}} is a silent no-op, and there is no "unlimited" setting
{{FileSystemCompiler}} guards the assignment with {{if (tolerance > 0)}}, so
{{-t 0}} leaves the default of 10 in place with no diagnostic. There is also no
spelling for "report everything" -- a caller wanting all errors has to guess a
sufficiently large number. Related: {{--help}} does not state the default, so
the option reads as unbounded-by-default when it is in fact 10.
h2. 3. Tolerance is not exposed by the Ant task
The Ant {{<groovyc>}} task has no tolerance attribute, so build-tool users have
no direct route to the setting. (Gradle users can already reach it through
{{groovyOptions.configurationScript}} with {{configuration.tolerance = 100}},
and embedded callers have {{setTolerance()}}.)
h2. Fix
# Tolerance is applied uniformly. {{ClassCodeVisitorSupport.addError}} now
routes through the tolerance-aware {{ErrorCollector.addError}}, and
{{StaticTypeCheckingVisitor}} does the same for the source unit's own collector
-- but not for the temporary collectors it pushes for speculative checks, whose
errors are routinely discarded once a candidate is ruled in or out.
# A tolerance of zero or less means unlimited. The command-line option is held
in a boxed {{Integer}} so an explicit {{0}} is distinguishable from the option
being absent, the default is named as
{{CompilerConfiguration.DEFAULT_TOLERANCE}} rather than repeated as a literal,
and {{--help}} states it.
# The Ant {{<groovyc>}} task gains a {{tolerance}} attribute. Both the forked
and in-process paths run the same assembled argument list through the
{{FileSystemCompiler}} parser, so emitting the option once covers both.
The {{-t}} option has been undocumented since it was added in GROOVY-11194, so
it is now in the groovyc option table, alongside the new Ant attribute in that
task's table.
h2. Behavioural change
Type checking errors now count towards the tolerance, and the default of 10
therefore caps them where they were previously unbounded. A compilation
reporting 40 type checking errors will report 10 and stop. Use {{-t 0}} (or
{{configuration.tolerance = 0}}) to restore full reporting. The default is
deliberately left at 10 so that the documented contract, already honoured by
parse and class generation errors, now holds for every error kind.
This affects any caller reporting more than the tolerance through a
{{ClassCodeVisitorSupport}} subclass, not only the compiler front ends. In
particular {{SourceUnit.create(String, String)}} selects a tolerance of *1*, so
a visitor driven over a source unit from that factory now stops at the first
error; the three-argument overload takes an explicit tolerance.
h2. Notes
*Completeness is per-phase.* Even with unlimited tolerance, a single type
checking error anywhere in the compilation suppresses every class generation
error, because {{failIfErrors()}} runs at the end of each phase. Worth being
aware of when reasoning about "report all errors", but a separate concern from
this issue.
*The {{groovy.errors.tolerance}} system property behaves inconsistently, but is
best left alone.* It has existed since the original 2004 commit (a0a831f4e3) as
a key of the {{CompilerConfiguration(Properties)}} bag, so whether it is
reachable as a system property depends purely on the entry point:
{{GroovyMain}} uses {{new CompilerConfiguration(System.getProperties())}} and
honours it, whereas {{FileSystemCompiler}} uses the no-arg constructor, which
never consults the property.
{noformat}
groovy -Dgroovy.errors.tolerance=50 ... -> 14 errors
groovyc -Dgroovy.errors.tolerance=50 ... -> 10 errors
{noformat}
With the fixes above every path has a first-class route to the setting, so the
property adds little. Promoting it to the no-arg constructor would also freeze
it into {{CompilerConfiguration.DEFAULT}} at class-init, making it JVM-global
and sticky across every compilation in a Gradle daemon. Documenting the current
behaviour is preferred over changing it.
was:
The compiler's error tolerance -- the number of non-fatal errors accepted
before compilation bails out -- is only partially wired up. Two defects plus
one gap, all verified against 6.0.0-beta-2:
h2. 1. Tolerance is not applied to static type checking errors
{{-t}} / {{--tolerance}} has no effect whatsoever on type checking errors, so
there is no way to ask for fail-fast behaviour on the most common error class
in {{@CompileStatic}} code.
{code:java}
@groovy.transform.CompileStatic
class Z {
def m0() { new Object().nope0() }
def m1() { new Object().nope1() }
// ... 14 such methods
}
{code}
||Command||Expected||Actual||
|{{groovyc -t 1 Z.groovy}}|1 error|14 errors|
|{{groovyc -t 3 Z.groovy}}|3 errors|14 errors|
Cause: {{StaticTypeCheckingVisitor.addStaticTypeError}} (line 6855) routes
through {{ClassCodeVisitorSupport.addError}} (line 490), which calls
{{ErrorCollector.addErrorAndContinue}} directly. Only
{{ErrorCollector.addError(Message)}} (line 126) performs the {{errors.size() >=
configuration.getTolerance()}} check, so every diagnostic raised via a
{{ClassCodeVisitorSupport}} subclass bypasses the threshold.
Errors raised through {{SourceUnit.addError}} *are* capped correctly, which
produces a confusing split. Using the {{Access to P#M is forbidden}} error from
{{StaticTypesCallSiteWriter}} as a contrasting example, with 14 offending
accesses in a single file:
||Command||Result||
|{{groovyc}} (default)|10 errors|
|{{groovyc -t 50}}|14 errors|
|{{groovyc -t 1}}|1 error|
So the same flag governs class generation errors but silently does nothing for
type checking errors.
h2. 2. {{-t 0}} is a silent no-op, and there is no "unlimited" setting
{{FileSystemCompiler}} line 588 guards the assignment:
{code:java}
if (tolerance > 0) {
configuration.setTolerance(tolerance);
}
{code}
{{-t 0}} therefore leaves the default of 10 in place with no diagnostic. There
is also no spelling for "report everything" -- a caller wanting all errors has
to guess a sufficiently large number. {{0}} = unlimited would be the natural
convention.
Related: {{--help}} does not state the default, so the option reads as
unbounded-by-default when it is in fact 10.
h2. 3. Tolerance is not exposed by the Ant task
The Ant {{<groovyc>}} task has no tolerance attribute, so build-tool users have
no direct route to the setting. (Gradle users can already reach it through
{{groovyOptions.configurationScript}} with {{configuration.tolerance = 100}},
and embedded callers have {{setTolerance()}}.)
h2. Suggested fixes
# Apply tolerance uniformly across error kinds, so {{-t 1}} genuinely means
"stop after the first error" regardless of which phase raised it. This is the
highest-value part.
# Treat {{-t 0}} as unlimited rather than silently ignoring it, and document
the default of 10 in {{--help}}.
# Add a tolerance attribute to the Ant {{<groovyc>}} task, once the semantics
above are settled.
h2. Notes
*Completeness is per-phase.* Even with unlimited tolerance, a single type
checking error anywhere in the compilation suppresses every class generation
error, because {{failIfErrors()}} runs at the end of each phase. Worth being
aware of when reasoning about "report all errors", but a separate concern from
this issue.
*The {{groovy.errors.tolerance}} system property behaves inconsistently, but is
probably best left alone.* It has existed since the original 2004 commit
(a0a831f4e3) as a key of the {{CompilerConfiguration(Properties)}} bag, so
whether it is reachable as a system property depends purely on the entry point:
{{GroovyMain}} (line 110) uses {{new
CompilerConfiguration(System.getProperties())}} and honours it, whereas
{{FileSystemCompiler}} (line 571) uses the no-arg constructor, which hardcodes
{{tolerance = 10}} (line 564) and never consults the property.
{noformat}
groovy -Dgroovy.errors.tolerance=50 ... -> 14 errors
groovyc -Dgroovy.errors.tolerance=50 ... -> 10 errors
{noformat}
Once the fixes above land, every path has a first-class route to the setting
and the property adds little. Promoting it to the no-arg constructor would also
freeze it into {{CompilerConfiguration.DEFAULT}} at class-init, making it
JVM-global and sticky across every compilation in a Gradle daemon. Suggest
documenting the current behaviour rather than changing it.
> Error tolerance is not applied to type checking errors and has no unlimited
> setting
> -----------------------------------------------------------------------------------
>
> Key: GROOVY-12306
> URL: https://issues.apache.org/jira/browse/GROOVY-12306
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> The compiler's error tolerance -- the number of non-fatal errors accepted
> before compilation bails out -- is only partially wired up. Three defects,
> all verified against 6.0.0-beta-2:
> h2. 1. Tolerance is not applied to static type checking errors
> {{-t}} / {{--tolerance}} has no effect whatsoever on type checking errors, so
> there is no way to ask for fail-fast behaviour on the most common error class
> in {{@CompileStatic}} code.
> {code:java}
> @groovy.transform.CompileStatic
> class Z {
> def m0() { new Object().nope0() }
> def m1() { new Object().nope1() }
> // ... 14 such methods
> }
> {code}
> ||Command||Expected||Actual||
> |{{groovyc -t 1 Z.groovy}}|1 error|14 errors|
> |{{groovyc -t 3 Z.groovy}}|3 errors|14 errors|
> Cause: only {{ErrorCollector.addError(Message)}} performs the {{errors.size()
> >= configuration.getTolerance()}} check. {{ClassCodeVisitorSupport.addError}}
> calls {{addErrorAndContinue}} instead, so every diagnostic raised through a
> visitor bypasses the threshold, and {{StaticTypeCheckingVisitor}} overrides
> {{addError}} to call the collector directly as well.
> Errors raised through {{SourceUnit.addError}} *are* capped correctly, which
> produces a confusing split: the same flag governs class generation errors but
> silently does nothing for type checking errors.
> h2. 2. {{-t 0}} is a silent no-op, and there is no "unlimited" setting
> {{FileSystemCompiler}} guards the assignment with {{if (tolerance > 0)}}, so
> {{-t 0}} leaves the default of 10 in place with no diagnostic. There is also
> no spelling for "report everything" -- a caller wanting all errors has to
> guess a sufficiently large number. Related: {{--help}} does not state the
> default, so the option reads as unbounded-by-default when it is in fact 10.
> h2. 3. Tolerance is not exposed by the Ant task
> The Ant {{<groovyc>}} task has no tolerance attribute, so build-tool users
> have no direct route to the setting. (Gradle users can already reach it
> through {{groovyOptions.configurationScript}} with {{configuration.tolerance
> = 100}}, and embedded callers have {{setTolerance()}}.)
> h2. Fix
> # Tolerance is applied uniformly. {{ClassCodeVisitorSupport.addError}} now
> routes through the tolerance-aware {{ErrorCollector.addError}}, and
> {{StaticTypeCheckingVisitor}} does the same for the source unit's own
> collector -- but not for the temporary collectors it pushes for speculative
> checks, whose errors are routinely discarded once a candidate is ruled in or
> out.
> # A tolerance of zero or less means unlimited. The command-line option is
> held in a boxed {{Integer}} so an explicit {{0}} is distinguishable from the
> option being absent, the default is named as
> {{CompilerConfiguration.DEFAULT_TOLERANCE}} rather than repeated as a
> literal, and {{--help}} states it.
> # The Ant {{<groovyc>}} task gains a {{tolerance}} attribute. Both the forked
> and in-process paths run the same assembled argument list through the
> {{FileSystemCompiler}} parser, so emitting the option once covers both.
> The {{-t}} option has been undocumented since it was added in GROOVY-11194,
> so it is now in the groovyc option table, alongside the new Ant attribute in
> that task's table.
> h2. Behavioural change
> Type checking errors now count towards the tolerance, and the default of 10
> therefore caps them where they were previously unbounded. A compilation
> reporting 40 type checking errors will report 10 and stop. Use {{-t 0}} (or
> {{configuration.tolerance = 0}}) to restore full reporting. The default is
> deliberately left at 10 so that the documented contract, already honoured by
> parse and class generation errors, now holds for every error kind.
> This affects any caller reporting more than the tolerance through a
> {{ClassCodeVisitorSupport}} subclass, not only the compiler front ends. In
> particular {{SourceUnit.create(String, String)}} selects a tolerance of *1*,
> so a visitor driven over a source unit from that factory now stops at the
> first error; the three-argument overload takes an explicit tolerance.
> h2. Notes
> *Completeness is per-phase.* Even with unlimited tolerance, a single type
> checking error anywhere in the compilation suppresses every class generation
> error, because {{failIfErrors()}} runs at the end of each phase. Worth being
> aware of when reasoning about "report all errors", but a separate concern
> from this issue.
> *The {{groovy.errors.tolerance}} system property behaves inconsistently, but
> is best left alone.* It has existed since the original 2004 commit
> (a0a831f4e3) as a key of the {{CompilerConfiguration(Properties)}} bag, so
> whether it is reachable as a system property depends purely on the entry
> point: {{GroovyMain}} uses {{new
> CompilerConfiguration(System.getProperties())}} and honours it, whereas
> {{FileSystemCompiler}} uses the no-arg constructor, which never consults the
> property.
> {noformat}
> groovy -Dgroovy.errors.tolerance=50 ... -> 14 errors
> groovyc -Dgroovy.errors.tolerance=50 ... -> 10 errors
> {noformat}
> With the fixes above every path has a first-class route to the setting, so
> the property adds little. Promoting it to the no-arg constructor would also
> freeze it into {{CompilerConfiguration.DEFAULT}} at class-init, making it
> JVM-global and sticky across every compilation in a Gradle daemon.
> Documenting the current behaviour is preferred over changing it.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
