[
https://issues.apache.org/jira/browse/GROOVY-12401?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18115022#comment-18115022
]
ASF GitHub Bot commented on GROOVY-12401:
-----------------------------------------
paulk-asert commented on PR #2924:
URL: https://github.com/apache/groovy/pull/2924#issuecomment-5660256046
Initial AI read:
> **Verdict: two real bugs correctly fixed, with tests that genuinely fail
without the change. Merge after covering the third fix.**
>
> Verified locally: 1580 tests pass across `groovy.util.*` and
`*ScriptEngine*` (including `GroovyScriptEngineReloadingTest`), `lintGate`
green, clean trial merge.
>
> **The one thing to add before merge.** The PR makes four changes; the JIRA
describes three. The fourth — `createCompilationUnit` resolving through
`rc.getResourceConnection(...)` instead of the engine's own roots-based method
— **has no test**. I reverted that hunk alone and the entire suite stayed
green. It's a real bug (with a connector-only engine, `roots` is empty, so the
engine's method can only throw) and its failure mode is silent, because the
loop does `catch (ResourceException e) { /* ignore */ }`. Dependencies just
quietly don't get added to the compilation unit.
>
> I checked each new test by reverting the main source: two genuinely fail,
one is characterisation rather than regression, and the forward-slash one can't
fail on Unix — though `groovy-build-test.yml` does run `windows-latest` on
`pull_request`, so CI covers it.
>
> **Two claims that are wider than the code supports** — worth rewording,
not reverting:
>
> - **The leak.** On the paths this PR touches, the built-in connector
returns an *unconnected* `URLConnection` — `openConnection` verifies with a
first connection, then returns a fresh second one. `AbstractHttpServlet`
likewise ends in `return url.openConnection()`. Nothing holds a descriptor, so
nothing leaks. And `forceClose` doesn't release a descriptor, it *opens* one in
order to close it — the PR's own test asserts exactly that (`assert
tracking.inputStreamOpened`). So on the default path the change adds an
open/close per lookup. It's still right for third-party connectors that return
connected connections, which is the population the PR is aimed at.
> - **Windows.** Both `ResourceConnector`s Groovy ships already normalise
`\` → `/` before use, so no shipped code was broken. The change benefits
third-party connectors and removes the obligation to normalise — a good reason,
just not the stated one. Since the contract is now load-bearing,
`ResourceConnector.getResourceConnection` should say so in javadoc.
>
> **One thing the change depends on and gets right**, worth a code comment:
enabling `setResLoader()` on the `GroovyClassLoader`-parent path raises the
question of which `CompilerConfiguration` the lambda reads, since that
constructor inherits the *parent's*. It binds to the enclosing engine's,
because `GroovyClassLoader` has no `getConfig()` at all. I confirmed by probe.
A future `getConfig()` added to `GroovyClassLoader` would silently flip it.
> Improve remaining parser diagnostics and authored AST source positions
> ----------------------------------------------------------------------
>
> Key: GROOVY-12401
> URL: https://issues.apache.org/jira/browse/GROOVY-12401
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h3. Problem
> After GROOVY-12353, several everyday syntax mistakes still surface as generic
> *Unexpected input*, and several authored AST nodes still carry line/column
> {{-1}} even though they came from source. Tooling uses {{lineNumber > 0}} to
> tell authored code from generated code ({{SecureASTCustomizer.isFromSource}}).
> h3. Diagnostics (before)
> * Array creation without a size, an extra empty dimension, or a size combined
> with an initializer:
> {noformat}
> new int[]
> new int[][2]
> new int[2] { 1 }
> {noformat}
> reports *Unexpected input*.
> * A type argument list written as a comparison (meant {noformat}List<Integer>
> name{noformat}):
> {noformat}
> List<Integer name
> {noformat}
> reports *Unexpected input*.
> * A method header in a statement position (for example inside a closure
> invoked as a statement) reports *Unexpected input* instead of naming the
> misplaced method.
> * {{import}} inside a method reports {{Unexpected input: 'import'}}.
> * {{do}} with two statements and a later {{while}}:
> {noformat}
> do
> println 1
> println 2
> while (false)
> {noformat}
> reports {{Missing 'while'}} on the second {{println}} even though {{while}}
> is already on the next line.
> * Enum constants followed by members without a semicolon:
> {noformat}
> enum E { A
> void m() {}
> }
> {noformat}
> reports *Unexpected input* instead of asking for {{;}} after the last
> constant.
> * An extra closer or semicolon reports {{Unexpected input: ')'}} (or the
> matching {{]}} / {{\}}} / {{;}}) instead of naming the extra token.
> * A second {{default}} in a switch points the caret at the first {{default}},
> not the duplicate.
> * Integer overflow wording does not match javac's {{Integer number too
> large}}.
> h3. Source positions (before)
> At CONVERSION these authored nodes dump as {{-1}}:
> * Interface field {{int a;}} — {{FieldNode.lastLineNumber}} is copied from a
> synthetic {{ConstantExpression(0)}}.
> {{DefaultGroovyMethods.asBoolean(Object)}} is non-null, so the synthetic
> value is treated as a real stop.
> * Classic for-control {noformat}for (int i = 0; i < n; i++){noformat} — the
> outer {{ClosureListExpression}} is never given a source span. Empty
> init/cond/update slots ({{EmptyExpression.INSTANCE}}) should stay {{-1}}.
> * Authored empty parentheses:
> {noformat}
> new Object()
> new Runnable() { }
> super()
> {noformat}
> * GROOVY-11492 annotation default — the {{listX}} rewrite drops the brace
> span:
> {noformat}
> String[] x() default { }
> {noformat}
> * Single-spread index wrapper (the multi-index path was already stamped):
> {noformat}
> a[*[1, 2]]
> {noformat}
> Not a bug — these have no span in the source and should stay {{-1}}:
> {{ModuleNode}}, a script's wrapping {{BlockStatement}}, implicit {{return
> null}}, TWR desugaring (names such as {noformat}__$$t0{noformat}), synthetic
> {{@Trait}}, the class initializer {noformat}<clinit>{noformat}, and
> {{EmptyExpression.INSTANCE}}.
> h3. Goal
> * javac-aligned sentences for the remaining recognition failures, with the
> caret on the right token
> * authored AST nodes carry a real source span; generated nodes stay at {{-1}}
> * error-path only — no grammar error alternatives (GROOVY-9588)
> h3. Approach
> * {{AbstractFriendlyErrorStrategy}} refines the fallback sentence: array
> creation, unclosed type argument, method header in statement position,
> misplaced {{import}}, unmatched {{do}}/{{while}}, enum semicolon, extra
> closers, annotation-style {{default}} after a method header.
> {{MissingDelimiterDiagnostic}} still relocates the caret for an unclosed
> {{)}} / {{]}} / {{\}}}. Locate/refine stay in a defensive try; listener
> dispatch is one call afterwards.
> * {{AstBuilder}} / {{ModifierManager}} / {{Numbers}}: record headers, missing
> method body vs abstract, index used as a method call, overflow wording;
> duplicate {{default}} points at the second {{default}}.
> * {{PositionConfigureUtils}}: copy an end position only when {{lastLineNumber
> > 0}}. Stamp the classic-for control list, authored empty parentheses, the
> GROOVY-11492 {{listX}} result, and the single-spread index wrapper.
> h3. Expected result (after)
> * {noformat}new int[]{noformat} → {noformat}Array dimension missing; specify
> a size or add an initializer '{}'{noformat}
> * {noformat}List<Integer name{noformat} → {{Missing '>'}}
> * method header in a statement position → {{Method definition not expected
> here}}
> * {{import}} inside a method → {{'import' is only allowed at the beginning of
> a compilation unit}}
> * {{do}} / two statements / later {{while}} → {noformat}do-while body must be
> a single statement; wrap multiple statements in '{ }'{noformat}; a true
> omission is still {{Missing 'while'}}
> * enum constants then members, no semicolon → {{';' expected after the last
> enum constant}}
> * extra {{)}} → {{Unexpected ')'}}
> * interface {{int a;}}, empty {noformat}for (;;) { }{noformat}, {{new
> Object()}}, annotation {noformat}default { }{noformat}, and {noformat}a[*[1,
> 2]]{noformat} carry real line/column on the authored node; synthetic children
> stay {{-1}}
> Valid programs are unchanged.
> h3. Related
> GROOVY-12353, GROOVY-11492, GROOVY-12169, GROOVY-9588
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
