[
https://issues.apache.org/jira/browse/GROOVY-12724?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18141962#comment-18141962
]
ASF GitHub Bot commented on GROOVY-12724:
-----------------------------------------
daniellansun commented on PR #3259:
URL: https://github.com/apache/groovy/pull/3259#issuecomment-6767767394
The PR now targets master.
I ran the cases from the review. Notes on each, in the same order.
(1) Confirmed. `@CompileDynamic` and `@CompileStatic(SKIP)` skip the
static-compilation visitor only. A `@TypeChecked` class still type-checks that
method. `@TypeChecked(SKIP)` is the annotation that skips both visitors.
`checked()` was treating every skip as “do not adapt this method”, so the
`@TypeChecked` class above saw the lowered query with no row-check extension.
It now follows `isSkipMode`: the type-checked visitor looks only at
`@TypeChecked`, and the static-compilation visitor looks at `@TypeChecked` and
`@CompileStatic`. On a `@TypeChecked` class, a `@CompileDynamic` or
`@CompileStatic(SKIP)` method gets the extension and the query wrapper. On a
`@CompileStatic` class, that method is not type-checked, and the wrapper is not
installed. A method with its own non-skip `@TypeChecked` is still checked.
(2) Confirmed for the two methods in the review. The extension has to be
registered on the class annotation, because that is the member the checker
reads, but the hooks no longer treat the rest of the class as a query.
`value.mispelledProperty` and `value[0]` on an enclosing-method parameter
are compile errors again. Inside the query, an unknown row property stays
dynamic: `r.vlue` on a list of maps is a missing key, not a type error. Outside
the query the only receivers treated as rows are the query result itself
(`result[0].toList()`, `get` / `getAt` on that result). A field and a parameter
of the enclosing method are not.
(3) The snippet with `int threshold = 5` inside the query does not compile,
with or without the bridge. `VariableScopeVisitor` rejects a local that is
already declared in an enclosing scope, including a closure parameter that
reuses a local name. I did not special-case that into something Groovy
otherwise forbids.
What the bridge does copy is decided by that visitor, not by a set of names.
The lowered query is spliced into the method, the visitor links each reference
to its declaration, and the original call is put back. A read-only local
declared outside the query is snapshotted when the expression runs. A local the
query assigns is shared, so the write is visible to the caller. A
`DynamicVariable` is not copied: at conversion that is what an import alias
still is, and the alias then resolves to the class. A field is read when the
closure runs, not copied. `STRATEGY.md` matches this. The old name walk is not
still in the bridge.
The snapshot is taken only when the result is deferred. `@GQ(List)` and
`GQL` run at the call, and a later assignment is not something those queries
observe.
(4) Confirmed. Widening was not limited to a real override.
Only a `def` return is widened. An explicit return type is left alone. The
override check requires the same name, the same static-ness, and
`ParameterUtils.parametersEqual` on the parameters. Private methods are
ignored. Superclasses and `getAllInterfaces()` are both walked. A super return
of `Object` does not block. A super return the widened type does not extend or
implement does, so `String load(List)` on an interface stays `String` and the
verifier reports that conflict instead of a `Queryable` return the method never
declared. A same-named private method, and a static method with different
parameters, do not block the widening.
(5) The whole lowered query closure is still compiled dynamically. I tried
not to do that.
`makeDynamic` on each operator is not enough for codegen.
`StaticTypesCallSiteWriter` throws on an operator it cannot prove, and a row
comparison is that operator. Rewriting the operators to `ScriptBytecodeAdapter`
calls compiled the comparison and then broke `orderby x in desc` (`in` is not a
predicate there), join conditions, and the parameter types of parallel clause
lambdas. A static lambda also cannot capture a local of an enclosing closure,
which is the snapshot array: the bytecode loads the outer class and casts it to
`GroovyObject`.
So the query closure is marked `DYNAMIC_RESOLUTION = Boolean.TRUE` and
`StaticTypesClosureWriter` emits it with the dynamic writer. The enclosing
method stays static. Calls inside the closure are still `makeDynamic`, and the
stand-in target is cleared only there, so the dynamic writer does not emit
`invokevirtual` of a method that does not exist. An extension registered before
`GinqRowChecks` still sees the calls the visitor delivers on the enclosing
method (`toList`, the macro call, `defer`). It does not see `where` and
`select` as those names. I would rather those stayed visible. The codegen above
is why the closure is dynamic anyway.
(6) `LazyQueryable` is public because the generated bytecode calls `defer`
and is not in its package. The class is `@Internal`. `defer` is
`@GroovyABI(since = "7.0.0")`. Callers program to `Queryable`. The class is the
thunk that builds the pipeline on first read and then caches it. Fluent
`Ginq.from` does not use it.
(7) Confirmed. A member that is not a string, a class literal, or a list or
array of those was left unchanged and the extension was dropped. That is now a
compile error, and the member is left as written. A class literal is stored as
the class name, because the checker loads strings. A list that itself contains
a call is the error case, not a supported form.
(8) `Ginq.from(list)` was caching the materialized list, so the second
`toList()` did not see `numbers.add(4)`. It re-reads the iterable on every
terminal now. `where` and `select` still only record the work. `Ginq.from` on a
`Stream` reads that stream once and replays the buffer; a stream cannot be read
twice.
`stream()` was returning the same one-shot `Stream` until something
materialized the query, so the second `count()` failed. `stream()` now returns
a new stream each time and does not pull while creating it. Elements already
pulled are replayed from the start; the rest are pulled from the pipeline once.
`count()` on a sized pipeline does not pull, which is ordinary `Stream.count`
and is what keeps an `exists` subquery from running its `select` just because
the groups are built. After the source has been read to the end, later
`stream()` calls read the buffer. None of those streams is the instance a
previous caller is holding.
> GINQ: support static compilation and a fluent query API
> -------------------------------------------------------
>
> Key: GROOVY-12724
> URL: https://issues.apache.org/jira/browse/GROOVY-12724
> Project: Groovy
> Issue Type: Improvement
> Components: GINQ
> Reporter: Daniel Sun
> Assignee: Daniel Sun
> Priority: Major
>
> Under @CompileStatic / @TypeChecked, GQ, GQL, and @GQ currently rely on
> dynamic lowering. This improvement makes those entry points usable from a
> statically checked scope while keeping the SQL-like syntax lowered to the
> in-memory collection runtime.
> h3. Goals
> * Allow GQ / GQL / @GQ from @CompileStatic and @TypeChecked without forcing
> the lowered tree through static checking at the expression site.
> * Defer pipeline construction with LazyQueryable until a terminal read
> (toList, count, etc.), copying locals at the expression site (same binding as
> dynamic GQ).
> * Add Ginq.from(...) as a deferred, generic fluent chain on the same runtime,
> with @ClosureParams so where/select/join/aggregate closures can infer element
> types where inference is sound.
> * Preserve default lazy/deferred evaluation and dynamic GQ behavior for map
> rows and other intentionally dynamic cases.
> h3. Non-goals / known limits
> * Stored untyped Closure variables still need a SAM target type for parameter
> inference (same as Java/C#).
> * Raw Query parameters/fields cannot recover a safe element type.
> * Names bound by from inside GQ / GQL / @GQ stay dynamic by design; callers
> that need checked element types use Ginq.from.
> h3. Related work
> GitHub PR: https://github.com/apache/groovy/pull/3259
> Stacked on GEP-26 SQL window work (PR #3167 / gep26-sql-window).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
