Remove all deprecated methods.#7604
Conversation
…o java17-buildsystem
…o java17-treeutils
…java17-deprecated
…o java17-treeutils
…java17-deprecated
mernst
left a comment
There was a problem hiding this comment.
Should JVMNames.getJVMMethodName() be removed?
The class TreeUtilsAfterJava11 should probably be deprecated. Also add a comment about why the class exists.
The nested classes within it should also be deprecated.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java (1)
820-832:⚠️ Potential issue | 🟡 MinorUpdate stale compatibility Javadoc for
isBindingVariable.Lines 822–823 claim JDK 8/11 compatibility, but line 831 directly references
ElementKind.BINDING_VARIABLE, which does not exist before Java 12. Update the Javadoc to reflect the actual implementation and avoid misleading users.Suggested doc fix
- * <p>This implementation compiles and runs under JDK 8 and 11 as well as versions that contain - * {`@code` ElementKind.BINDING_VARIABLE}. + * <p>Equivalent to checking whether {`@code` element.getKind()} is + * {`@link` ElementKind#BINDING_VARIABLE}.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java` around lines 820 - 832, Update the Javadoc for isBindingVariable to remove the misleading claim about JDK 8/11 compatibility and clearly state that the implementation relies on ElementKind.BINDING_VARIABLE (so it only applies to JDKs that define that enum constant, e.g., Java 12+ or runtimes that include it); reference the method name isBindingVariable and the enum ElementKind.BINDING_VARIABLE in the doc so readers understand the dependency, and keep the existing `@Deprecated` annotation and since/forRemoval metadata unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 2400-2402: The method TreeUtils.isSwitchExpression currently uses
"tree instanceof SwitchExpressionTree" but is not marked deprecated like similar
helpers (isBindingPatternTree, isYield); to be consistent, annotate
isSwitchExpression with `@Deprecated` and update its Javadoc to instruct callers
to use "instanceof SwitchExpressionTree" directly (reference the
TreeUtils.isBindingPatternTree and TreeUtils.isYield deprecations as the model
to follow), keeping the existing method body for backward compatibility.
---
Outside diff comments:
In `@javacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.java`:
- Around line 820-832: Update the Javadoc for isBindingVariable to remove the
misleading claim about JDK 8/11 compatibility and clearly state that the
implementation relies on ElementKind.BINDING_VARIABLE (so it only applies to
JDKs that define that enum constant, e.g., Java 12+ or runtimes that include
it); reference the method name isBindingVariable and the enum
ElementKind.BINDING_VARIABLE in the doc so readers understand the dependency,
and keep the existing `@Deprecated` annotation and since/forRemoval metadata
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46d9c63a-d397-425e-adcd-29c17293a197
📒 Files selected for processing (7)
annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/JVMNames.javachecker-util/src/main/java/org/checkerframework/checker/signedness/util/SignednessUtil.javaframework/src/main/java/org/checkerframework/framework/stub/AnnotationFileUtil.javajavacutil/src/main/java/org/checkerframework/javacutil/ElementUtils.javajavacutil/src/main/java/org/checkerframework/javacutil/Resolver.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.javajavacutil/src/main/java/org/checkerframework/javacutil/TreeUtilsAfterJava11.java
💤 Files with no reviewable changes (1)
- annotation-file-utilities/src/main/java/org/checkerframework/afu/scenelib/util/JVMNames.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java`:
- Around line 2399-2402: The isSwitchExpression(Tree) method is only annotated
with a bare `@Deprecated` while sibling helpers use explicit removal metadata;
update the `@Deprecated` annotation on isSwitchExpression(Tree) to include
forRemoval = true and since = "4.0.0" (matching the pattern used by the other
helpers) so the API shows it is slated for removal in 4.0.0 and aligns with the
removal metadata used by the surrounding methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b6c658c-ca60-41a4-83f9-a0ab83b2a135
📒 Files selected for processing (1)
javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java
| * @deprecated Use {@code tree instanceof SwitchExpressionTree} | ||
| */ | ||
| @Deprecated | ||
| public static boolean isSwitchExpression(Tree tree) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider aligning isSwitchExpression(Tree) with the same removal metadata.
Unlike the sibling helpers on Lines 2342 and 2413, this one is only marked with bare @Deprecated. If it belongs to the same 4.0.0 removal wave, adding forRemoval = true, since = "4.0.0" will keep the API signal consistent.
🧩 Proposed change
- `@Deprecated`
+ `@Deprecated`(forRemoval = true, since = "4.0.0")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @deprecated Use {@code tree instanceof SwitchExpressionTree} | |
| */ | |
| @Deprecated | |
| public static boolean isSwitchExpression(Tree tree) { | |
| * `@deprecated` Use {`@code` tree instanceof SwitchExpressionTree} | |
| */ | |
| `@Deprecated`(forRemoval = true, since = "4.0.0") | |
| public static boolean isSwitchExpression(Tree tree) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@javacutil/src/main/java/org/checkerframework/javacutil/TreeUtils.java` around
lines 2399 - 2402, The isSwitchExpression(Tree) method is only annotated with a
bare `@Deprecated` while sibling helpers use explicit removal metadata; update the
`@Deprecated` annotation on isSwitchExpression(Tree) to include forRemoval = true
and since = "4.0.0" (matching the pattern used by the other helpers) so the API
shows it is slated for removal in 4.0.0 and aligns with the removal metadata
used by the surrounding methods.
merge after #7582