On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Changes look generally good. Personally, I found the changes in Flow the harder to follow - but I that part is just hard (even in the spec), as it has to perform a search in both breadth (as records have more than one component) and in depth (as patterns can be nested). I've added some comments to check my understanding. src/java.base/share/classes/java/lang/MatchException.java line 41: > 39: * constants at runtime than it had at compilation time, or the > type hierarchy has changed > 40: * in incompatible ways between compile time and run time.</li> > 41: * <li>Null targets and sealed types. If an interface or abstract > class {@code C} is sealed Should this be `null targets and nested sealed type test patterns` ? Also, not sure `null` target is the right expression - maybe `null and nested xyz` would work better? src/jdk.compiler/share/classes/com/sun/source/tree/DeconstructionPatternTree.java line 43: > 41: * @return the deconstructed type > 42: */ > 43: Tree getDeconstructor(); Shouldn't this return an ExpressionTree? src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180: > 4178: type = attribTree(tree.var.vartype, env, varInfo); > 4179: } else { > 4180: type = resultInfo.pt; Looks good - infers the binging var type from the record component being processed. If not in a record, then I suspect resultInfo.pt is just the target expression type (e.g. var in toplevel environment). src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 750: > 748: private Set<Symbol> coveredSymbols(DiagnosticPosition pos, Type > targetType, > 749: Iterable<? extends > JCCaseLabel> labels) { > 750: Set<Symbol> constants = new HashSet<>(); Should this be called `constants` ? src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 824: > 822: } > 823: for (Symbol currentType : nestedCovered) { > 824: if (types.isSubtype(types.erasure(currentType.type), Not 100% what this test does src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 836: > 834: for (Entry<Symbol, List<JCDeconstructionPattern>> e : > componentType2Patterns.entrySet()) { > 835: if (coversDeconstructionStartingFromComponent(pos, > targetType, e.getValue(), component + 1)) { > 836: covered.add(e.getKey()); So... it seems to me that what this code is doing is that, for a component index _i_, we get all its recursively covered symbols - then we add them to the set of covered symbols of component _i_, but only if components _w_, where _w_ > _i_ are also covered? That is, if in `R(P1, P2, ... Pn)`, we see that `P1` is covered, but `P2` is not, we also consider `P1` not covered (which in turn makes `R` not covered). src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 788: > 786: } > 787: if (token.kind == LPAREN) { > 788: ListBuffer<JCPattern> nested = new ListBuffer<>(); should we add comment saying "deconstruction pattern" src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 808: > 806: pattern = toP(F.at(pos).DeconstructionPattern(e, > nested.toList(), var)); > 807: } else { > 808: JCVariableDecl var = toP(F.at(token.pos).VarDef(mods, > ident(), e, null)); and, here, a comment saying "type test pattern" ? src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 1004: > 1002: JCExpression type = unannotatedType(false); > 1003: if (token.kind == IDENTIFIER) { > 1004: checkSourceLevel(token.pos, > Feature.PATTERN_MATCHING_IN_INSTANCEOF); Two question/comments: * I believe that `checkSourceLevel` is called here, but not in `analyzePattern` because `analyzePattern` is also called in the `switch` code, in which case we already check for source level which supports pattern in switch, correct? * For type test pattern, this code recurses to parsePattern, which seems correct, but the same doesn't happen for deconstructor patterns. Apart from the initial "peek" the code seems otherwise the same - would it be possible to consolidate? src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line 3186: > 3184: : PatternResult.PATTERN; > 3185: } > 3186: parentDepth++; break; * s/parentDepth/parenDepth * maybe s/depth/typeDepth ------------- PR: https://git.openjdk.java.net/jdk/pull/8516
