You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@grails.apache.org by "matrei (via GitHub)" <gi...@apache.org> on 2026/10/06 09:05:48 UTC
[PR] Resolve nested classes from source when generating the tag library index [grails-core]
matrei opened a new pull request, #16538:
URL: https://github.com/apache/grails-core/pull/16538
Fixes #16537
Before compilation, the tag library index generator resolves the project types a tag library refers to from source. A nested class could not be resolved this way, so any tag library that reached one, even indirectly through a service, was skipped and its namespace was recorded as incomplete.
Groovy asks for a nested class by its binary name (`Outer$Inner`), and `SourceRootClassNodeResolver` turned that name straight into a file name. The resolver now looks for a nested class in its outermost class's source file. It returns that file only if the file actually declares the requested class. That check matters because Groovy also tries names that only might be nested: a type `Other` named inside `Helper` is tried as `Helper$Other` before `Other`. Without the check, `Helper.groovy` would be returned for a class it does not declare. The check reads the file's syntax tree up to the conversion phase, as `declaredNamespace` already does, and caches the result per file.
Tests added to `SourceResolvedIndexGeneratorSpec`:
- a nested class reached through another project class is resolved (fails without this change);
- a class and its nested class used side by side are read from a single source;
- a type named inside another class is not mistaken for a nested class of it;
- a misspelled nested class still fails, and its tag library is left out.
`SourceResolvedIndexGeneratorSpec`, `TagLibraryIndexGeneratorSpec`, the other index specs, the `grails-taglib` tests and `codeStyle` for both modules pass.
Also verified on an application that hit this: the pre-compile descriptor for its tag library now matches the one `packageTagLibraryIndex` writes after compilation.
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve nested classes from source when generating the tag library index [grails-core]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16538:
URL: https://github.com/apache/grails-core/pull/16538#issuecomment-6013829286
## [Codecov](https://app.codecov.io/gh/apache/grails-core/pull/16538?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.45455%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 59.5523%. Comparing base ([`aa54db7`](https://app.codecov.io/gh/apache/grails-core/commit/aa54db705697582f726da4b48c02408a7ce3607b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`33f804f`](https://app.codecov.io/gh/apache/grails-core/commit/33f804f3d28d291dcc741dd23c6bcc15e18c022a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16538?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../grails/taglib/index/TagLibraryIndexGenerator.java](https://app.codecov.io/gh/apache/grails-core/pull/16538?src=pr&el=tree&filepath=grails-gsp%2Fgrails-taglib%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Ftaglib%2Findex%2FTagLibraryIndexGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWdzcC9ncmFpbHMtdGFnbGliL3NyYy9tYWluL2dyb292eS9vcmcvZ3JhaWxzL3RhZ2xpYi9pbmRleC9UYWdMaWJyYXJ5SW5kZXhHZW5lcmF0b3IuamF2YQ==) | 95.4546% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/grails-core/pull/16538?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/grails-core/pull/16538?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 8.0.x #16538 +/- ##
==================================================
+ Coverage 59.5410% 59.5523% +0.0113%
- Complexity 25020 25021 +1
==================================================
Files 2175 2175
Lines 108636 108654 +18
Branches 19823 19826 +3
==================================================
+ Hits 64683 64706 +23
+ Misses 34999 34993 -6
- Partials 8954 8955 +1
```
| [Files with missing lines](https://app.codecov.io/gh/apache/grails-core/pull/16538?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../grails/taglib/index/TagLibraryIndexGenerator.java](https://app.codecov.io/gh/apache/grails-core/pull/16538?src=pr&el=tree&filepath=grails-gsp%2Fgrails-taglib%2Fsrc%2Fmain%2Fgroovy%2Forg%2Fgrails%2Ftaglib%2Findex%2FTagLibraryIndexGenerator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Z3JhaWxzLWdzcC9ncmFpbHMtdGFnbGliL3NyYy9tYWluL2dyb292eS9vcmcvZ3JhaWxzL3RhZ2xpYi9pbmRleC9UYWdMaWJyYXJ5SW5kZXhHZW5lcmF0b3IuamF2YQ==) | `80.2395% <95.4546%> (+1.7160%)` | :arrow_up: |
... and [3 files with indirect coverage changes](https://app.codecov.io/gh/apache/grails-core/pull/16538/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve nested classes from source when generating the tag library index [grails-core]
Posted by "testlens-app[bot] (via GitHub)" <gi...@apache.org>.
testlens-app[bot] commented on PR #16538:
URL: https://github.com/apache/grails-core/pull/16538#issuecomment-6014566184
## ✅ All tests passed ✅
🏷️ Commit: 33f804f3d28d291dcc741dd23c6bcc15e18c022a
▶️ Tests: 108847 executed
⚪️ Checks: 87/87 completed
---
_Learn more about TestLens at [testlens.app/docs](https://testlens.app/docs/features/pr-comment/)._
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve nested classes from source when generating the tag library index [grails-core]
Posted by "jdaugherty (via GitHub)" <gi...@apache.org>.
jdaugherty merged PR #16538:
URL: https://github.com/apache/grails-core/pull/16538
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Resolve nested classes from source when generating the tag library index [grails-core]
Posted by "codeconsole (via GitHub)" <gi...@apache.org>.
codeconsole commented on code in PR #16538:
URL: https://github.com/apache/grails-core/pull/16538#discussion_r4199647495
##########
grails-gsp/grails-taglib/src/main/groovy/org/grails/taglib/index/TagLibraryIndexGenerator.java:
##########
@@ -354,15 +364,41 @@ public LookupResult resolveName(String name, CompilationUnit compilationUnit) {
}
private File findSource(String name) {
- String relativePath = name.replace('.', File.separatorChar) + ".groovy";
+ int nested = name.indexOf('$');
+ String outermost = nested < 0 ? name : name.substring(0, nested);
+ String relativePath = outermost.replace('.', File.separatorChar) + ".groovy";
for (File root : roots) {
File candidate = new File(root, relativePath);
if (candidate.isFile()) {
- return candidate;
+ return nested < 0 || declares(candidate, name) ? candidate : null;
}
}
return null;
}
Review Comment:
`declares` now runs for almost every source this resolver adds, not only ones with nested classes. For each simple type name inside a class, Groovy first tries `Current$Name` (the probing the Javadoc describes), and `findSource` now maps that to the class's own file, which exists. I logged what `declares` parses while running this spec. Besides `Connection.groovy`, it parses `BookService`, `Attrs`, `BaseTagLib`, `GreetingTags`, `Channels` and `Helper`. So every collaborator is parsed a second time, even in a project with no nested classes. In the fallback path, each per-source `compile()` builds a new resolver, so the cache starts empty every time.
When the outermost class is already parsed into the unit, its nested classes went in with it, and Groovy checks the unit before it asks the resolver. A nested name that gets here is then one it doesn't declare, and the unit can say so without a parse:
```java
@Override
public LookupResult resolveName(String name, CompilationUnit compilationUnit) {
// ...
File source = findSource(name, compilationUnit);
// ...
}
private File findSource(String name, CompilationUnit compilationUnit) {
int nested = name.indexOf('$');
String outermost = nested < 0 ? name : name.substring(0, nested);
if (nested >= 0 && isParsed(outermost, compilationUnit)) {
// Its nested classes were added to the unit with it, and this name is not among them.
return null;
}
// ...
}
private static boolean isParsed(String className, CompilationUnit compilationUnit) {
ClassNode parsed = compilationUnit.getAST().getClass(className);
// A class that is only queued for compilation has no module yet, and still has to be read.
return parsed != null && parsed.getModule() != null;
}
```
With that change, all the index specs in `grails-web-taglib` still pass (this one, `TagLibraryIndexGeneratorSpec` and `SingleIndexProducerSpec`), and `declares` only parses a file whose nested class is asked for before the file is in the unit. Not blocking.
##########
grails-gsp/grails-web-taglib/src/test/groovy/org/grails/taglib/index/SourceResolvedIndexGeneratorSpec.groovy:
##########
@@ -177,6 +177,135 @@ class SourceResolvedIndexGeneratorSpec extends Specification {
descriptor('StarredTagLib').tags == 'show'
}
+ void 'a nested class this project declares is read from its outer class\'s source'() {
+ given: 'the compiler asks for it by its binary name, which no source file is named after'
+ appSource('com/example/Connection.groovy', '''
+ package com.example
+ class Connection {
+ static class Subscription {
+ String stream
+ }
+ }
+ ''')
+ appSource('com/example/Channels.groovy', '''
+ package com.example
+ class Channels {
+ Connection.Subscription authorize(String identifier) { null }
+ }
+ ''')
+ taglib('Nesting.groovy', '''
+ import com.example.Channels
+ import grails.gsp.TagLib
+ @TagLib
+ class NestingTagLib {
+ static namespace = 'nesting'
+ Channels channels
+ def show(Map attrs) { }
+ }
+ ''')
+
+ when:
+ generate()
+
+ then:
+ descriptor('NestingTagLib').tags == 'show'
+ indexOf().isNamespaceComplete('nesting')
+ }
+
+ void 'a class and its nested class referred to together are read from one source'() {
+ given: 'reading the source once per name would declare both classes twice'
+ appSource('com/example/Connection.groovy', '''
+ package com.example
+ class Connection {
+ static class Subscription {
+ }
+ }
+ ''')
+ appSource('com/example/Channels.groovy', '''
+ package com.example
+ class Channels {
+ void subscribe(Connection connection, Connection.Subscription subscription) { }
+ }
+ ''')
+ taglib('Paired.groovy', '''
+ import com.example.Channels
+ import grails.gsp.TagLib
+ @TagLib
+ class PairedTagLib {
+ static namespace = 'paired'
+ Channels channels
+ def show(Map attrs) { }
+ }
+ ''')
+
+ when:
+ generate()
+
+ then:
+ descriptor('PairedTagLib').tags == 'show'
+ indexOf().isNamespaceComplete('paired')
+ }
+
+ void 'a type named inside another is not mistaken for a nested class of it'() {
+ given: 'the compiler tries Helper$Other before com.example.Other, and Helper.groovy declares no such class'
+ appSource('com/example/Other.groovy', '''
+ package com.example
+ class Other {
+ }
+ ''')
+ appSource('com/example/Helper.groovy', '''
+ package com.example
+ class Helper {
+ Other other
+ }
+ ''')
+ taglib('Helped.groovy', '''
+ import com.example.Helper
+ import grails.gsp.TagLib
+ @TagLib
+ class HelpedTagLib {
+ static namespace = 'helped'
+ Helper helper
+ def show(Map attrs) { }
+ }
+ ''')
+
+ when:
+ generate()
+
+ then:
+ descriptor('HelpedTagLib').tags == 'show'
+ indexOf().isNamespaceComplete('helped')
+ }
+
+ void 'a misspelled nested class is not invented'() {
+ given:
+ appSource('com/example/Connection.groovy', '''
+ package com.example
+ class Connection {
+ static class Subscription {
+ }
+ }
+ ''')
+ taglib('Mistaken.groovy', '''
+ import com.example.Connection
+ import grails.gsp.TagLib
+ @TagLib
+ class MistakenTagLib {
+ static namespace = 'mistaken'
+ Connection.Subscriptoin subscription
+ def show(Map attrs) { }
+ }
+ ''')
+
+ when:
+ generate()
+
+ then:
+ manifest().isEmpty()
+ !indexOf().isNamespaceComplete('mistaken')
+ }
Review Comment:
Both of these fail on 8.0.x and pass with this change, and each takes a path the four tests above don't cover:
- An import of the nested class itself never asks for `Connection`, so Groovy reaches its source only through `com.example.Connection$Subscription`. That's a common way to use a nested class from another package.
- `Outer$Middle$Inner` is the only case where `indexOf('$')` and `lastIndexOf('$')` give different answers. I swapped them, and all 18 tests in this spec still passed. Only this one fails.
```suggestion
}
void 'a nested class imported by its own name is read from its outer class\'s source'() {
given: 'nothing asks for Connection itself, so its source is only reached through the nested name'
appSource('com/example/Connection.groovy', '''
package com.example
class Connection {
static class Subscription {
}
}
''')
appSource('com/example/cable/Channels.groovy', '''
package com.example.cable
import com.example.Connection.Subscription
class Channels {
Subscription authorize(String identifier) { null }
}
''')
taglib('Importing.groovy', '''
import com.example.cable.Channels
import grails.gsp.TagLib
@TagLib
class ImportingTagLib {
static namespace = 'importing'
Channels channels
def show(Map attrs) { }
}
''')
when:
generate()
then:
descriptor('ImportingTagLib').tags == 'show'
indexOf().isNamespaceComplete('importing')
}
void 'a class nested more than one level deep is read from its outermost class\'s source'() {
given: 'its binary name is Outer$Middle$Inner, and only the part before the first $ names a source file'
appSource('com/example/Outer.groovy', '''
package com.example
class Outer {
static class Middle {
static class Inner {
}
}
}
''')
appSource('com/example/Holder.groovy', '''
package com.example
class Holder {
Outer.Middle.Inner inner
}
''')
taglib('Deep.groovy', '''
import com.example.Holder
import grails.gsp.TagLib
@TagLib
class DeepTagLib {
static namespace = 'deep'
Holder holder
def show(Map attrs) { }
}
''')
when:
generate()
then:
descriptor('DeepTagLib').tags == 'show'
indexOf().isNamespaceComplete('deep')
}
```
--
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: notifications-unsubscribe@grails.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org