matrei commented on PR #16463: URL: https://github.com/apache/grails-core/pull/16463#issuecomment-5954420234
Post-merge review: the constant-text idea works well for the dynamic-restriction idioms. While reviewing, though, I found a few gaps that I've written up in #16481 and fixed in #16482: - **Some locals holding a value still pass as constant text.** This happens when a local is assigned somewhere the walk cannot place in order: a multiple assignment, a ternary or `&&`/`||` operand, a method argument, a `do`/`while` condition, or a closure called later. It also happens when a loop variable or closure parameter reuses the name of an earlier constant local. Each case was a build error before this PR. - **Compile time grows exponentially with nesting.** Every loop and closure body is walked at least twice, inside each walk of its enclosing body. 24 nested closures take about 33 s to compile, and this check runs on every class compiled with `grails-datamapping-core` on the classpath. - **The docs and the error message overstate what is reported.** Text joined from a `List` or returned from a helper method is not reported when it is passed to the query method on its own. The error message also recommends `StringBuilder`, which the check cannot follow. #16482 tracks locals per declaration and settles the locals the walk cannot order with a per-method pre-scan. It walks a body that assigns nothing outside it only once, and corrects the message and the guide. Reviews welcome. -- 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]
