blackdrag commented on PR #3055: URL: https://github.com/apache/groovy/pull/3055#issuecomment-6548036813
I think this is a substantial improvement over the original implementation. Introducing `MatchPattern` and `PatternExpression`, and moving the lowering into a dedicated `PatternLowering` pass, addresses my main concern with the previous approach: Relying too much on metadata attached AST nodes. ### 1. Lifecycle of `PatternExpression` and lowered code It introduces two representations of the same construct that must remain consistent. For example, a transformation that modifies the pattern after lowering could leave the existing lowered expression implementing the old pattern. The same applies to changes to guards or other pattern-related information. The current API requires transformations running after the lowering pass to invoke `PatternLowering.lower(...)`. This makes the lifecycle of the AST node an important part of the extension contract. If the implementation became immutable after the lowering we would not have the problem, but that is not how our AST work in general. And since we have really no control over if a change violates the implicit contract this depends on... This is especially relevant to future macro support and AST transformations that operate at different compiler phases. ### 2. Tooling and AST visitors need access to the semantic representation The default visitPatternExpression implementation visits the lowered code when available. This is useful for compatibility with existing visitors, but it also means that generic AST tooling can observe the generated implementation instead of the original pattern construct. I see here potential problems for tooling. ### 3. Runtime deconstruction still has an exception-handling problem RecordPatternSupport.components() still invokes toList() and catches MissingMethodException to handle cases where deconstruction is unavailable. This can also catch a MissingMethodException thrown from inside a user-defined toList() implementation. In that case, an exception originating in user code can be mistaken for the absence of a deconstruction method, potentially replacing the original failure with a misleading result. ### 4. Iterable materialization can be repeated across pattern arms ListPatternSupport.elementsOrNull() materializes non-List iterables. Since the lowered code invokes this helper for individual list-pattern arms, a switch with multiple list-pattern arms can traverse and materialize the same selector more than once. Repeated traversal should be covered by tests. And maybe materializing the selector once per switch evaluation and reusing the result could be done. ### 5. GroovyAbi sill unused -- 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]
