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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
