blackdrag commented on PR #3259:
URL: https://github.com/apache/groovy/pull/3259#issuecomment-6754637731
@daniellansun This branch does not target main?
Disclaimer: These are conclusions based on the source, I did not actually
execute any code. So maybe some points are incorrect.
(1)
```Groovy
@TypeChecked
class Example {
@CompileDynamic
def query(List<Map> rows) {
GQ {
from r in rows
where r.value > 10
select r.value
}
}
}
```
The method is still type-checked. Nevertheless, GinqStaticBridge.checked()
returns false, so the bridge does not install its row-checking extension or
wrap the lowered query closure.
The consequence is that the macro-generated code is presented to the checker
without the adaptations required for dynamic GINQ row access.
```Groovy
@TypeChecked
class Example {
@CompileStatic(TypeCheckingMode.SKIP)
def query(List<Map> rows) {
GQ {
from r in rows
select r.value
}
}
}
```
Is a similar case. @CompileDynamic and @CompileStatic(SKIP) must not be
treated as equivalent to @TypeChecked(SKIP).
(2)
```Groovy
@CompileStatic
class Example {
Object value
def query(List<Map> rows) {
GQ {
from r in rows
select r.value
}
}
String unrelated() {
value.mispelledProperty
}
}
```
If I see right, then r.vlue would be accepted.
GinqStaticBridge.attachRowChecks() attaches to the class level, which means it
participates outside the GQ block. Most dynamic resolution is restricted to the
marked GQ closure, but fallback handling for Object receivers is not restricted
to that closure.
The post-visit subscript rewrite also operates outside it, imagine
```Groovy
Object getValue(Object value) {
value[0]
}
```
instead of the method `unrelated`.
(3)
Static GQ needs to preserve the value visible when the expression is
evaluated, even though construction of the underlying query pipeline is
deferred.
```Groovy
int threshold = 10
GQ {
from row in rows
select {
int threshold = 5
row.value > threshold
}
}
```
A name-based analysis has to distinguish the local threshold from the
enclosing one. But the current global declared-name collection cannot represent
that distinction. Similar for more complicated cases involving closure
parameters, nested closures and declarations.
Also I am wondering if class values would be correctly recognized. I am
thinking here of import aliases for example, that may make what seems to be a
variable actually a class.
And `STRATEGY.md` says that the hand-written local-variable walk was
removed, but the bridge still contains a scope collector and separate
captured-variable analysis.
(4)
```Groovy
@GQ
def query(List rows) {
from r in rows
select r
}
```
If I understand right this undergoes return type widening and changes to
Queryable. Also `incompatibleSuperReturn` does not ensure that the candidate
actually represents the same method signature, nor does it inspect implemented
interface methods. This could create a return-type decision without accounting
for all the relevant override relationships.
(5)
DYNAMIC_RESOLUTION causes the entire closure expression to be dynamically
compiled.
That gives other type-checking extensions or AST transformations no chance.
They may want a particular expression to resolve dynamically without
necessarily wanting to switch the compilation mode of the entire generated
closure.
(6)
The new LazyQueryable.defer method does not have a GroovyABI annotation. The
class is also public and has no @Internal annotation, despite being described
in STRATEGY.md as an implementation detail of the compiler integration. The
positioning is not quite clear.
(7)
The extension registration and custom extensions
GinqStaticBridge.addExtension() modifies the existing annotation's
extensions memberand handles a constant expression and a list expression, which
covers the usual source forms. But, it silently does nothing when the existing
expression has another form.
I am unsure of if there are extension-list expressions /produced through
more complicated annotation processing.
(8)
```Groovy
def numbers = [1, 2, 3]
def query = Ginq.from(numbers).where { it > 1 }
def first = query.toList()
numbers.add(4)
def second = query.toList()
assert first != second // will fail?
```
While documented, this might still be surprising, but that is how the
materialization idea works.
```Groovy
def query = GQ { ... }
def s1 = query.stream()
def s2 = query.stream()
assert s1 == s2
s1.count()
s2.count() // <- stream already consumed?
query.toList()
def s3 = query.stream()
assert s3 != s1
s3.count()
def s4 = query.stream()
assert s4 != s3
s4.count()
```
Without a materialization stream() will return the same object, while after
materialization it based off the list and a new stream is created every time.
--
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]
