blackdrag commented on PR #2933: URL: https://github.com/apache/groovy/pull/2933#issuecomment-6417467003
Another review by AI, this time more highlevel and after some discussion: ## Architectural concerns I have a few architectural concerns about how pattern matching is represented and processed. My main concern is not the amount of code involved, but how much of the implementation depends on implicit conventions between compiler phases. These conventions could make the feature difficult to evolve and harder to support consistently across the compiler. ### 1. Pattern semantics are encoded in metadata rather than represented explicitly The parser currently does much more than construct an AST for the source construct. It lowers patterns into existing expressions and statements, generates tests and bindings, and attaches metadata that later compiler phases use to recover the original pattern semantics. `SwitchPatternUtils` centralizes many of these metadata keys, but the underlying representation remains implicit. The static type checker and bytecode generation code must recognize particular combinations of ordinary AST nodes and metadata to distinguish pattern matching from regular Groovy expressions and switch labels. This creates several problems: - **The semantics are fragile across compiler phases.** A transformation that modifies, copies, or reconstructs an AST node can lose metadata or create an expression whose apparent structure no longer agrees with its intended semantics. - **The representation is difficult to evolve.** Adding new pattern forms or changing their semantics requires maintaining conventions across parsing, type checking, variable-scope handling, and code generation. The compiler cannot rely on an explicit representation that defines which pattern construct it is processing. - **The semantics are difficult to preserve outside the parser.** Code that constructs or transforms an AST programmatically must reproduce implementation details that are currently established as a side effect of parsing source code. I think this is the most significant design issue in the current implementation. **Suggested direction:** Introduce an explicit internal representation of patterns that preserves their semantics through the compiler phases that need them. It does not necessarily need to be a public AST API or a new family of public AST node classes. An internal pattern model or intermediate representation would already provide a clear contract between parsing, semantic analysis, and code generation. The compiler could then lower patterns into ordinary expressions and statements at a deliberate phase boundary, once the phases that require pattern-specific information have completed their work. Metadata can still be useful for auxiliary information, but should not be the primary representation of the language construct. ### 2. Programmatic AST construction and Groovy macros The representation problem also affects code that generates Groovy code. Consider a transformation or macro that constructs a method containing a pattern-matching switch. If it constructs the AST directly, it cannot simply create the conceptual pattern and let the compiler handle it. It must instead reproduce the parser's lowering strategy and attach the metadata expected by the type checker and code generators. This is an unnecessarily fragile interface for compiler extensions. It also means that changes to the implementation can break AST-generating code even when the source-level language construct remains unchanged. Source-level macro expansion that produces Groovy source and reparses it can avoid some of this problem, but it does not solve it for macros or transformations that work directly with the AST. An explicit pattern representation would give these facilities a semantic construct to work with, rather than requiring them to manufacture the compiler's current implementation details. It would also provide a foundation for future macro support without requiring the pattern implementation to be designed specifically around macros. I would therefore treat support for programmatic construction as a concrete reason to address the representation issue, rather than as a separate feature request. ### 3. Static and dynamic compilation should share pattern semantics The implementation has pattern-specific handling in several compiler phases, including the static type checker and bytecode generation. Given that patterns are lowered into ordinary AST constructs and metadata, there is a risk that different phases infer different semantics from the same construct. This matters for more than type inference. The two compilation modes need to agree on such things as: - which pattern matches a value, including null handling; - when a binding is introduced and what its scope is; - the types assigned to bindings, including nested and generic record components; - the order in which patterns, guards, and bindings are evaluated; - how failed matches proceed to subsequent arms. Static compilation can provide additional diagnostics and optimizations, but it should not silently define different matching behavior from dynamic compilation. **Suggested direction:** Keep pattern-specific semantic analysis separate from the mechanics of lowering and code generation, and make both compilation modes consume the same pattern semantics. Add paired tests for dynamic compilation, `@TypeChecked`, and `@CompileStatic`, especially for nested patterns, generic record components, guards, nulls, binding scope, and failed matches. This would also make future additions to pattern matching less likely to require duplicating semantic decisions across several compiler phases. ### 4. Runtime deconstruction and exception handling There is a concrete concern in `RecordPatternSupport.components`: the implementation invokes `toList()` and catches `MissingMethodException` to handle the case where deconstruction is unavailable. A user-defined `toList()` method can itself throw `MissingMethodException` from inside its implementation. Catching that exception at the call site can therefore mistake an exception raised by user code for the absence of a deconstruction method, potentially replacing the original failure with a misleading result. The implementation should distinguish failure to resolve or invoke the intended method from an exception thrown by the method body. In particular, an exception originating inside user code should not be silently reinterpreted as a failed deconstruction lookup. The broader contract for deconstruction also deserves to be explicit. Supporting arbitrary objects through `toList()` extends the mechanism beyond native records and introduces dynamic behavior that may differ from what users expect from a record pattern. The implementation should define what happens when the method is absent, throws, or returns an unexpected value, and test those cases. ### 5. Repeated materialization of iterable values `ListPatternSupport.elementsOrNull` materializes non-`List` iterables into an `ArrayList`. A switch with multiple list-pattern arms can consequently traverse and materialize the same selector repeatedly. Besides the allocation cost, this has observable implications for iterables whose traversal has side effects, is expensive, or cannot be repeated safely. An infinite iterable can also prevent the match from completing. The documentation's requirement that an iterable be traversable non-destructively helps establish a constraint on callers, but repeated materialization still makes the behavior and cost of matching less predictable. **Suggested direction:** Define the supported iterable contract clearly and add tests for repeated traversal and failure cases. If the intended semantics permit it, consider caching the materialized elements for a selector during a single switch evaluation so that subsequent list-pattern arms can reuse them. That optimization should preserve the intended matching and evaluation semantics rather than being applied unconditionally. ### 6. Parser disambiguation and compatibility with existing Groovy syntax Pattern syntax is being introduced into a language that already permits flexible expressions and case labels. The parser therefore has to distinguish new pattern constructs from existing Groovy syntax in cases involving type names, method calls, capitalization, parentheses, and different forms of case labels. These distinctions are not merely parser implementation details: they determine whether existing code continues to mean what it meant before pattern matching was introduced. The implementation includes disambiguation logic, but this logic will become harder to maintain as the pattern grammar grows unless the rules are explicit and tested as a coherent compatibility contract. **Suggested direction:** Document the disambiguation rules and add regression tests for ambiguous forms, including legacy `isCase` behavior and mixed legacy and pattern labels. Where a new construct changes the interpretation of previously valid code, the compatibility implications and any available escape route should be clear. ### 7. Public API and binary compatibility The implementation introduces public runtime support classes, including `RecordPatternSupport`, `ListPatternSupport`, and `MapPatternSupport`, as well as pattern-related utilities under `org.apache.groovy.ast.tools`. Some of these methods appear to expose compiler implementation machinery rather than a deliberate user-facing API. Once public, such methods can become compatibility obligations even if the implementation needs to change as pattern matching evolves. **Suggested direction:** Review which classes and methods genuinely need to be public and keep implementation details internal where possible. Any intentionally public surface should have a clear API purpose and an appropriate compatibility policy. I could not establish from the review evidence whether the relevant binary-compatibility report has been generated and reviewed for this PR. Has that check been performed, and were the new public types and methods considered explicitly? ## Overall recommendation I would prioritize the pattern representation issue. The current approach can work, but it makes the semantics of a language construct depend heavily on metadata -- 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]
