Add suppress warnings & remove redudant cast quick-fix code action - #8646
Add suppress warnings & remove redudant cast quick-fix code action#8646zielinsky wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Java quick fixes for redundant casts and warning suppression. Adds configurable Java lint options, integrates them into Java compiler creation, and updates Java target resolution. Adds unit and LSP coverage for the new behavior. ChangesJava Quick Fixes
Java Lint Configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CodeActionProvider
participant SuppressWarnings
participant JavaTrees
participant Buffers
Client->>CodeActionProvider: request Java code actions
CodeActionProvider->>SuppressWarnings: process diagnostics
SuppressWarnings->>Buffers: read document text
SuppressWarnings->>JavaTrees: resolve enclosing member
JavaTrees-->>SuppressWarnings: return Java element
SuppressWarnings-->>CodeActionProvider: return TextEdit code action
CodeActionProvider-->>Client: return available action
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0d30058 to
3af8b1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala (1)
436-452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming ambiguity: field
javaTargetis typedJvmTarget, then pattern-matched againstJavaTarget.This isn't introduced by this diff, but the changed lines make the ambiguity more visible:
javaTarget: JvmTarget(line 437) is matched withcase j: JavaTarget => j.options(line 450). Consider renaming the field (e.g.target: JvmTarget) to reduce confusion now that the surrounding PR stack explicitly distinguishesJavaTargetfromJvmTarget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala` around lines 436 - 452, The `JavaLazyCompiler` field name is ambiguous because `javaTarget` is typed as `JvmTarget` but later pattern-matched against `JavaTarget` in `newCompiler`. Rename the field to something neutral like `target` (and update its uses such as `buildTargetId` and the `javaTarget match` block) so the `JavaTarget` vs `JvmTarget` distinction is clear and the code is easier to read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala`:
- Around line 91-112: Update warningName in SuppressWarnings so it only returns
a suppressible category for javac warnings, not ERROR diagnostics; gate the
lookup on diagnostic severity before calling warningNameFrom. Also tighten
warningNameFrom to avoid substring matches by using word-boundary/whole-token
matching instead of plain text.contains, and ensure overlapping lint names are
resolved deterministically so cases like unchecked cast map to the correct
category.
---
Nitpick comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala`:
- Around line 436-452: The `JavaLazyCompiler` field name is ambiguous because
`javaTarget` is typed as `JvmTarget` but later pattern-matched against
`JavaTarget` in `newCompiler`. Rename the field to something neutral like
`target` (and update its uses such as `buildTargetId` and the `javaTarget match`
block) so the `JavaTarget` vs `JvmTarget` distinction is clear and the code is
easier to read.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64c263cf-77b1-49f1-a005-7a55602498a2
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalamtags-interfaces/src/main/java/scala/meta/infra/FeatureFlag.javatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
💤 Files with no reviewable changes (1)
- mtags-interfaces/src/main/java/scala/meta/infra/FeatureFlag.java
3af8b1e to
b217fe0
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/src/test/scala/tests/InfraSuite.scala (1)
15-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid leaking
enabledFlagsbetween suites.SuppressWarningsLspSuite.beforeAll()mutates a shared singleton before callingsuper.beforeAll(), so a setup failure can leaveJAVAC_OPTIONSenabled for later suites. Move the toggle into a fixture/try-finallyteardown or reset the set centrally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/InfraSuite.scala` around lines 15 - 22, The shared TestingInfra.enabledFlags set is being mutated by SuppressWarningsLspSuite.beforeAll() without guaranteed cleanup, so a failed setup can leak state into later suites. Update the test setup to either use a fixture or wrap the toggle in a try-finally teardown that always removes the JAVAC_OPTIONS flag, or centralize reset logic for TestingInfra.enabledFlags so each suite starts from a clean state. Use the existing TestingInfra singleton and SuppressWarningsLspSuite.beforeAll() as the main places to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/src/test/scala/tests/InfraSuite.scala`:
- Around line 15-22: The shared TestingInfra.enabledFlags set is being mutated
by SuppressWarningsLspSuite.beforeAll() without guaranteed cleanup, so a failed
setup can leak state into later suites. Update the test setup to either use a
fixture or wrap the toggle in a try-finally teardown that always removes the
JAVAC_OPTIONS flag, or centralize reset logic for TestingInfra.enabledFlags so
each suite starts from a clean state. Use the existing TestingInfra singleton
and SuppressWarningsLspSuite.beforeAll() as the main places to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e08126e6-9956-4f39-94cd-b85abb70e10d
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
5e375ac to
249d4f9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
802-812: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the concurrent diagnostics-wait pattern.
The
diagnosticsPublished = server.awaitNextDiagnostics(...)assignment before_ <- server.didFocus(path)is a deliberate pattern to register the diagnostics handler before the focus event triggers publishing. A brief comment would help future readers understand why the future is created here rather than inline with<-.📝 Suggested comment
_ <- server.didOpen(path) + // Register the diagnostics handler before didFocus so diagnostics + // triggered by the focus event are captured. diagnosticsPublished = server.awaitNextDiagnostics(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 802 - 812, The diagnostics wait is intentionally started before the focus event to avoid missing the publish, so document this concurrent pattern in the test flow around the diagnosticsPublished assignment and server.didFocus call. Add a brief comment near the awaitNextDiagnostics setup in SuppressWarningsLspSuite to explain that the handler must be registered first, then the focus event can trigger diagnostics, and keep the existing sequencing unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 802-812: The diagnostics wait is intentionally started before the
focus event to avoid missing the publish, so document this concurrent pattern in
the test flow around the diagnosticsPublished assignment and server.didFocus
call. Add a brief comment near the awaitNextDiagnostics setup in
SuppressWarningsLspSuite to explain that the handler must be registered first,
then the focus event can trigger diagnostics, and keep the existing sequencing
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67e22879-e85c-4937-a673-59bcb6707da5
📒 Files selected for processing (9)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
249d4f9 to
f79fc10
Compare
c9a3884 to
e6255ce
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
621-719: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a runtime-JDK guard for JDK-only lint cases.
These fixtures require newer compiler diagnostics:
this-escapeneeds JDK 21, value-based class synchronization needs JDK 16, and obsoletestrictfpneeds JDK 17. On older CI runtimes,checkSuppressWarningswaits for diagnostics that do not exist. Guard these cases with a Java-versionassume, or split the suite by JDK versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 621 - 719, Add Java runtime-version assumptions to the JDK-dependent cases in checkSuppressWarnings: require JDK 17+ for strictfp-method, JDK 16+ for synchronization-method, and JDK 21+ for this-escape-constructor. Keep the existing fixtures and assertions unchanged, and leave the text-blocks-method case unguarded unless its diagnostics also require a version check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala`:
- Line 559: Update the Java lint options default formatting near
JavaLintOptions.default so each option value is quoted before joining, producing
a valid JSON array of strings such as ["cast","deprecation",...].
- Around line 50-60: Remove "this-escape" from the allValues used to construct
JavaLintOptions.default, while preserving it in allowed if user configuration
should still accept it. Ensure the default lint flags remain compatible with
Java 11/17.
---
Nitpick comments:
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 621-719: Add Java runtime-version assumptions to the JDK-dependent
cases in checkSuppressWarnings: require JDK 17+ for strictfp-method, JDK 16+ for
synchronization-method, and JDK 21+ for this-escape-constructor. Keep the
existing fixtures and assertions unchanged, and leave the text-blocks-method
case unguarded unless its diagnostics also require a version check.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 42ea884d-32e3-4622-a2f0-2f9c7d541875
📒 Files selected for processing (12)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/RemoveRedundantCast.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/main/scala/tests/BaseLspSuite.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (6)
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/test/scala/tests/UserConfigurationSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scala
- metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala
431f438 to
f437208
Compare
f437208 to
301267b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
lalias forCodeAction.Line 8 imports
org.eclipse.{lsp4j => l}. Line 31 uses the fully qualifiedorg.eclipse.lsp4j.CodeActioninstead. If no other reference tolexists in this file, the alias import becomes unused. The Metals build enables fatal warnings for unused imports in several modules, so this can break compilation.♻️ Proposed change
- private val onlyRemoveRedundantCast: org.eclipse.lsp4j.CodeAction => Boolean = + private val onlyRemoveRedundantCast: l.CodeAction => Boolean = _.getTitle() == RemoveRedundantCast.titleCompile this module with Metals MCP tools to confirm no unused-import warning is raised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala` around lines 31 - 32, Update the onlyRemoveRedundantCast type annotation to use the imported l alias instead of the fully qualified org.eclipse.lsp4j name, ensuring the existing lsp4j alias import remains used and the CodeAction predicate behavior is unchanged.Source: Coding guidelines
tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala (1)
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated
DeprecatedApilayout into a shared value.The same
DeprecatedApi.javalayout appears three times. Define it once as a private val and reuse it.♻️ Proposed refactor
+ private val deprecatedApiLayout = + """|/a/src/main/java/a/DeprecatedApi.java + |package a; + | + |class DeprecatedApi { + | `@Deprecated` + | static void old() {} + |} + |""".stripMarginThen pass
extraLayout = deprecatedApiLayoutin the three cases.Also applies to: 218-225, 281-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala` around lines 82 - 89, Extract the repeated DeprecatedApi.java content into a private shared value in SuppressWarningsLspSuite, then replace each of the three inline layouts with extraLayout = deprecatedApiLayout. Preserve the existing string contents and stripMargin behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scala`:
- Around line 229-260: Update appendWarningEdit to handle empty annotation
arrays and named value forms before constructing the TextEdit: render an empty
{} as {"warning"}, and preserve value = syntax by converting scalar or array
named values into valid forms such as {value = "deprecation", "warning"}. Keep
the existing duplicate-warning check and add coverage for empty, scalar named,
and array named annotations.
---
Nitpick comments:
In
`@tests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scala`:
- Around line 31-32: Update the onlyRemoveRedundantCast type annotation to use
the imported l alias instead of the fully qualified org.eclipse.lsp4j name,
ensuring the existing lsp4j alias import remains used and the CodeAction
predicate behavior is unchanged.
In `@tests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala`:
- Around line 82-89: Extract the repeated DeprecatedApi.java content into a
private shared value in SuppressWarningsLspSuite, then replace each of the three
inline layouts with extraLayout = deprecatedApiLayout. Preserve the existing
string contents and stripMargin behavior.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: ac5d9c3a-21f5-4897-b0bb-9e475fbc3e81
📒 Files selected for processing (13)
metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/Compilers.scalametals/src/main/scala/scala/meta/internal/metals/MetalsEnrichments.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/RemoveRedundantCast.scalametals/src/main/scala/scala/meta/internal/metals/codeactions/SuppressWarnings.scalametals/src/main/scala/scala/meta/internal/parsing/JavaTrees.scalatests/unit/src/main/scala/tests/BaseLspSuite.scalatests/unit/src/test/scala/tests/InfraSuite.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/codeactions/RemoveRedundantCastLspSuite.scalatests/unit/src/test/scala/tests/codeactions/SuppressWarningsLspSuite.scala
🚧 Files skipped from review as they are similar to previous changes (7)
- metals/src/main/scala/scala/meta/internal/metals/codeactions/CodeActionProvider.scala
- tests/unit/src/test/scala/tests/UserConfigurationSuite.scala
- tests/unit/src/test/scala/tests/InfraSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/CompilerConfiguration.scala
- metals/src/main/scala/scala/meta/internal/metals/Compilers.scala
- tests/unit/src/main/scala/tests/BaseLspSuite.scala
- metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala
c17a998 to
03d2b80
Compare
03d2b80 to
7d98a0b
Compare
Part of #8502
Summary by CodeRabbit
New Features
Bug Fixes
Tests