On Tue, 13 May 2025 19:36:11 GMT, Vicente Romero <vrom...@openjdk.org> wrote:
>> This PR is defining a new internal annotation, >> `@jdk.internal.RequiresIdentity`, with target types PARAMETER and >> TYPE_PARAMETER. The @RequiresIdentity annotation expresses the expectation >> that an argument to a given method or constructor parameter will be an >> object with a unique identity, not an instance of a value-based class; or >> that the type argument to a given type parameter will not be a value-based >> class type. >> >> For more details please refer to the complete description in the >> corresponding JIRA entry [1] >> >> TIA >> >> [1] https://bugs.openjdk.org/browse/JDK-8354556 > > Vicente Romero has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 34 commits: > > - Merge branch 'master' into JDK-8354556 > - Update src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java > > Co-authored-by: Chen Liang <li...@openjdk.org> > - additional changes from Archie > - removing dead code > - integrating code from Archie > - fixing bugs, removing dead code > - additional documentation changes and bug fixes > - documentation and adding alias to lint categories > - Merge branch 'master' into JDK-8354556 > - addressing review comment > - ... and 24 more: https://git.openjdk.org/jdk/compare/e7ce661a...22acaf29 javac changes look overall good to me. One question and some comments for consideration inline. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 2673: > 2671: result = check(tree, capturedRes, KindSelector.VAL, > resultInfo); > 2672: } > 2673: if (env.info.lint.isEnabled(LintCategory.IDENTITY)) { This is the only place where there's a check whether the lint is enabled before the call to `checkRequiresIdentity`. Is there a reason for that? src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 5671: > 5669: } > 5670: > 5671: void checkRequiresIdentity(JCTree tree, Lint lint) { For consideration: as far as I can see, we have a sharp(er) type when we call `checkRequiresIdentity`, but we give up the type, and re-instante it here using the pattern matching switch. I wonder if it would be more elegant (and hopefully not really too much longer) if the `checkRequiresIdentity` method would have multiple overloads, with the sharp(er) types, like `JCClassDecl`/`JCVariableDecl`, etc. Or is there a reason to given up the sharp(er) type and re-create it using the switch? src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 5823: > 5821: SymbolMetadata sm = t.tsym.getMetadata(); > 5822: if (sm != null && !t.getTypeArguments().isEmpty()) { > 5823: for (Attribute.TypeCompound ta: > sm.getTypeAttributes().stream() The code here, and the code in `checkIfTypeParamsRequiresIdentity` look similar a lot (although they manipulate `List<Type>` and `List<JCExpression>`, of course. I wonder if there's a chance to share the code, at least partially. ------------- PR Review: https://git.openjdk.org/jdk/pull/24746#pullrequestreview-2841121286 PR Review Comment: https://git.openjdk.org/jdk/pull/24746#discussion_r2089504249 PR Review Comment: https://git.openjdk.org/jdk/pull/24746#discussion_r2089507277 PR Review Comment: https://git.openjdk.org/jdk/pull/24746#discussion_r2089509893
