matrei opened a new pull request, #16482:
URL: https://github.com/apache/grails-core/pull/16482
Fixes #16481
A follow-up to #16463. The constant-text tracking that PR added let some
locals holding a parameter value pass as constant text. Each case listed in the
issue was a build error before #16463, compiles cleanly on `8.0.x`, and is a
build error again with this change. The fixpoint walk that #16463 added also
made compile time grow exponentially with nesting depth.
### Changes
- **Locals are tracked per declaration, not per name.** The tracking state
is keyed by the declaration the compiler resolved each reference to. A loop
variable, closure parameter or later local that reuses a name no longer
inherits the constant-text status of an earlier, out-of-scope local. This also
replaces the bookkeeping that re-armed a suppressed name when it was declared
again.
- **Assignments the walk cannot place in order no longer count as constant
text.** Some assignments sit in places the walk cannot order:
- inside another expression, such as a ternary or Elvis branch, an
`&&`/`||` operand, a method argument or a loop condition, including a
`do`/`while` condition
- in a multiple assignment
- inside a closure, for a local declared outside it
A pre-scan of each method finds these. A local assigned in one of these
ways, or declared outside a closure and read inside it, is constant text only
if every assignment to it in the method is constant text. The
dynamic-restriction idioms still compile cleanly, including constant text
appended inside a closure, a constant chosen by a ternary assignment, and a
constant fragment read inside a closure. New no-warning tests cover each of
these.
- **Bodies that cannot change the state are walked once.** A loop or closure
body that assigns no local declared outside it cannot change the state at its
head, so it no longer gets a fixpoint. A nested body that has already settled
silently is not walked again just to report. With 24 levels of nested closures
or loops, compiling the test class takes 0.06–0.34 s, against about 33 s on
`8.0.x`.
- **Constructors, field initialisers and object initialisers get a fresh
state each.** They are analysed like methods, so
`@SuppressWarnings("GormUnsafeQueryString")` on a constructor now takes effect
as it does on a method.
- **The error message no longer recommends `StringBuilder`**, which the
check cannot follow.
- **The guide is updated.** It now explains when a local that a closure
touches counts as constant text. It also says plainly that text joined from a
`List`, built with a `StringBuilder` or returned from a helper is treated as
data: it is reported when it is interpolated or concatenated, and not at all
when it is passed to the query method on its own.
### Testing
- `GormQuerySafetyTransformerSpec` has 22 new cases:
- every case from the issue, plus a safe-navigation argument and a closure
called after the local it reads was reassigned
- guards that a value flattened inside a closure is still caught after the
closure, on a later run, and after a `return`
- no-warning counterparts for the constant-text idioms above
- compile-time bounds for 24 levels of nested closures and loops, with and
without an outer assignment
- a flattened query in a constructor, with and without the suppression on
the constructor
- With this spec run against the transformer on `8.0.x`, exactly 15 cases
fail: the 10 false negatives, the 4 nesting bounds and the constructor
suppression. Everything else passes on both versions.
- The full `grails-datamapping-core` suite passes: 1493 tests, 1 skipped, 0
failures. Every module's main and test classes compile with the check active,
and `aggregateViolations` is clean.
--
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]
