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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
