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. -- 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]
