[
https://issues.apache.org/jira/browse/GROOVY-12691?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138374#comment-18138374
]
ASF GitHub Bot commented on GROOVY-12691:
-----------------------------------------
codeconsole opened a new pull request, #3212:
URL: https://github.com/apache/groovy/pull/3212
JIRA: [GROOVY-12691](https://issues.apache.org/jira/browse/GROOVY-12691)
### The bug
Static type checking drops an error when an earlier error was reported at
certain other source positions, without any message. Four undefined method
calls that start in column 47 give two errors under `@TypeChecked` or
`@CompileStatic`:
```groovy
@groovy.transform.TypeChecked
void m() {
missing1()
missing2()
missing3()
missing4()
}
```
`groovyc` reports `missing1()` and `missing2()` and ends with `2 errors`.
With the calls in column 5, all four are reported.
With 32 undefined calls in one column, one per line, the compiler reports
all 32 in columns 5, 20, 32, 40, 48, 60 and 65, but 16 in column 44, 8 in
column 45, 4 in column 46 and 2 in column 47. Long lines are not needed: two
calls on one line 64 columns apart (columns 5 and 69), or a call at line 3,
column 9 and another at line 6, column 8, also give only the first error.
The "field is hidden by a dynamic property" warning (GROOVY-11360) is
dropped the same way when an unrelated error was reported at a colliding
position.
### Cause
`StaticTypeCheckingVisitor.addError` skips an error whose key is already in
`TypeCheckingContext.reportedErrors`, and builds the key as
```java
Long err = ((long) node.getLineNumber()) << 16 + node.getColumnNumber();
```
Java parses this as `line << (16 + column)`, and a `long` shift uses only
the low six bits of its distance. Up to column 47 the key keeps only the low
`48 - column` bits of the line number (at column 47 only whether the line is
odd or even), columns 64 apart give the same shift, and line `L` at column `c`
gives the same key as line `2L` at column `c - 1`. The hidden-field warning in
`visitVariableExpression` checks `reportedErrors` with the same expression.
### Fix
Both places now build the key as `(((long) line) << 32) + column`. This
keeps the intended "one error per position" rule and gives every line and
column pair its own key. I used 32 rather than 16 bits for the line: a column
can exceed 65,535 on a single long line, and `(line << 16) + column` would let
column `c + 65536` of one line share a key with column `c` of the next.
`grep -rn "<< 16" src subprojects --include='*.java' --include='*.groovy'`
finds no other position key built this way, and these two expressions are the
only places that build a key for `reportedErrors`. `MopWriter.MopKey` has the
same precedence pattern in a hash code (`name.hashCode() << 2 +
params.length`), but `equals` compares name and parameters, so it only weakens
the hash and is left alone here.
### Tests
- `MiscSTCTest.testErrorsAtDistinctPositionsAreAllReported`: four calls in
column 47, two calls on one line at columns 5 and 69, and calls at line 2,
column 9 and line 4, column 8. Every error must be reported, and no others.
- `ClosuresSTCTest.testLexicalScopeVersusGetDynamicPropertyAfterError`: the
GROOVY-11360 setup with an unrelated error two lines above the field reference,
in the same column 47. The compilation must report the error and the
hidden-field warning.
Both tests also run under `@CompileStatic` through `MiscStaticCompileTest`
and `ClosuresStaticCompileTest`. In `MiscSTCTest` and `ClosuresSTCTest` both
failed on master: the first reported 2 of 4 errors, and the second had no
warning. Each of the three cases in the first test fails on master on its own
(2 of 4, 1 of 2, 1 of 2 errors). Both pass with the change.
```text
./gradlew :test --tests
groovy.transform.stc.MiscSTCTest.testErrorsAtDistinctPositionsAreAllReported \
--tests
groovy.transform.stc.ClosuresSTCTest.testLexicalScopeVersusGetDynamicPropertyAfterError
master: 2 failed;
with the change: green
./gradlew :test --tests 'groovy.transform.stc.*' --tests
'org.codehaus.groovy.transform.stc.*' \
--tests 'org.codehaus.groovy.classgen.asm.sc.*' 4,729 tests, 0
failures, 4 skipped
./gradlew :test 19,320 tests, 0
failures, 68 skipped
./gradlew :checkstyleMain no new violations
```
### Affected versions
Reproduced with JDK 21 on the Groovy 6.0.0, 6.0.0-beta-1, 5.0.8 and 5.0.0
distributions and on master (7.0.0-SNAPSHOT at 5e9212ce1e). The same expression
is in `StaticTypeCheckingVisitor` on `GROOVY_2_5_X`, `GROOVY_3_0_X` and
`GROOVY_4_0_X`.
### Backport
Suggested target: `GROOVY_6_0_X`. Both changed lines are the same there,
`MiscSTCTest` is identical, and the commit's diff applies to that branch's
files with line offsets only.
> Static type checking silently drops some errors that are at distinct source
> positions
> -------------------------------------------------------------------------------------
>
> Key: GROOVY-12691
> URL: https://issues.apache.org/jira/browse/GROOVY-12691
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> Under {{@TypeChecked}} or {{@CompileStatic}}, a type checking error is
> silently dropped when an earlier error was reported at certain other
> positions. Four undefined method calls that start in column 47 give only two
> errors.
> {{Col47.groovy}} (each call is preceded by 46 spaces, so it starts in column
> 47):
> {code:groovy}
> @groovy.transform.TypeChecked
> void m() {
> missing1()
> missing2()
> missing3()
> missing4()
> }
> {code}
> {code:sh}
> groovyc Col47.groovy
> {code}
> Actual: exit 1 with two errors.
> {code:text}
> org.codehaus.groovy.control.MultipleCompilationErrorsException: startup
> failed:
> Col47.groovy: 3: [Static type checking] - Cannot find matching method
> Col47#missing1(). Please check if the declared type is correct and if the
> method exists.
> @ line 3, column 47.
> missing1()
> ^
> Col47.groovy: 4: [Static type checking] - Cannot find matching method
> Col47#missing2(). Please check if the declared type is correct and if the
> method exists.
> @ line 4, column 47.
> missing2()
> ^
> 2 errors
> {code}
> Expected: four errors, one for each call. When the same four calls start in
> column 5, all four are reported.
> The number of errors that survive depends on the column. With 32 undefined
> calls, one per line, all starting in the same column:
> ||column||errors reported||
> |5, 20, 32, 40|32 of 32|
> |44|16 of 32|
> |45|8 of 32|
> |46|4 of 32|
> |47|2 of 32|
> |48, 60, 65|32 of 32|
> Long lines are not needed. Each of these pairs of calls also gives only the
> first error:
> * two calls on the same line 64 columns apart, for example columns 5 and 69
> of line 3;
> * a call at line 3, column 9 and a call at line 6, column 8 (in general, line
> L column c and line 2L column c - 1).
> The warning "The field: ... is hidden by a dynamic property" (GROOVY-11360)
> is dropped in the same way: in a class with {{private static final value}}, a
> reference to {{value}} inside {{d.with \{ ... \}}} at line 7, column 47 gets
> no warning when an unrelated error was reported at line 5, column 47. It gets
> the warning when that error is at column 5.
> Affected versions: 6.0.0, 6.0.0-beta-1, 5.0.8 and 5.0.0 give the counts
> above, and so does current master (7.0.0-SNAPSHOT at 5e9212ce1e). The same
> expression is in {{StaticTypeCheckingVisitor}} on {{GROOVY_2_5_X}},
> {{GROOVY_3_0_X}} and {{GROOVY_4_0_X}}; those were not run.
> Cause: {{StaticTypeCheckingVisitor.addError}} skips an error whose key is
> already in {{TypeCheckingContext.reportedErrors}}, and builds the key as
> {{((long) node.getLineNumber()) << 16 + node.getColumnNumber()}}. Because
> {{+}} binds tighter than {{<<}}, that is {{line << (16 + column)}}, and a
> shift of a {{long}} uses only the low six bits of the distance. So up to
> column 47 the key keeps only the low 48 - column bits of the line number, two
> columns 64 apart on one line give the same key, and line L at column c gives
> the same key as line 2L at column c - 1. The check for the hidden-field
> warning in {{visitVariableExpression}} builds the key the same way.
> Fix outline: build the key as {{(((long) line) << 32) + column}}, so that
> each line and column pair has its own key; {{(line << 16) + column}} would
> still let column c + 65536 of one line collide with column c of the next.
> A pull request with regression tests follows.
> Environment: Groovy 6.0.0 on BellSoft JDK 21.0.12, macOS 26.5 (arm64);
> reproduced on master at 5e9212ce1e with the same JDK.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
