-
Notifications
You must be signed in to change notification settings - Fork 721
SONARJAVA-6106 ScopedValue instances should be assigned to a stable reference #5480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
aurelien-coet-sonarsource
wants to merge
7
commits into
master
Choose a base branch
from
ac/SONARJAVA-6106
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+254
−1
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
30fe1fe
Implement rule S8465
aurelien-coet-sonarsource d1d776a
Change the rule's logic to detect more cases
aurelien-coet-sonarsource f709012
Add rule S8465 description and metadata
aurelien-coet-sonarsource 70418ac
Fix the autoscan tests
aurelien-coet-sonarsource a256095
Revert to more conservative logic for the rule
aurelien-coet-sonarsource 4fcb167
Add support for chained where and nested calls to ScopedValue.newInst…
aurelien-coet-sonarsource 3ef64d8
Fix the quality gate
aurelien-coet-sonarsource File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 changes: 6 additions & 0 deletions
6
its/autoscan/src/test/resources/autoscan/diffs/diff_S8465.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8465", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 0, | ||
| "falsePositives": 0 | ||
| } |
42 changes: 42 additions & 0 deletions
42
...ecks-test-sources/default/src/main/java/checks/ScopedValueStableReferenceCheckSample.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package checks; | ||
|
|
||
| import java.util.Objects; | ||
| import org.apache.commons.lang3.tuple.Pair; | ||
|
|
||
| class ScopedValueStableReferenceCheckSample { | ||
|
|
||
| private static final ScopedValue<String> VALUE = ScopedValue.newInstance(); | ||
|
|
||
| public void where() { | ||
| ScopedValue.where(ScopedValue.newInstance(), "inaccessible").run(() -> { // Noncompliant {{Consider using a stable reference for ScopedValue instances.}} | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| // Cannot reference the scoped value here, as it has no name. | ||
| }); | ||
| } | ||
|
|
||
| public void chainedWhere() { | ||
| ScopedValue<String> scopedValue = ScopedValue.newInstance(); | ||
| ScopedValue.where(scopedValue, "accessible").where(ScopedValue.newInstance(), "inaccessible").run(() -> { // Noncompliant {{Consider using a stable reference for ScopedValue instances.}} | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| }); | ||
| } | ||
|
|
||
| public void nestedArgument() { | ||
| ScopedValue.where(Objects.requireNonNull(ScopedValue.newInstance()), "scopedValue").run(() -> {}); // Noncompliant {{Consider using a stable reference for ScopedValue instances.}} | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ScopedValue.where((ScopedValue.newInstance()), "scopedValue").run(() -> {}); // Noncompliant {{Consider using a stable reference for ScopedValue instances.}} | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| ScopedValue.where(Pair.of(ScopedValue.newInstance(), 0).getLeft(), "scopedValue").run(() -> {}); // Noncompliant {{Consider using a stable reference for ScopedValue instances.}} | ||
| // ^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| } | ||
|
|
||
| public String readFieldInWhere() { | ||
| return ScopedValue.where(VALUE, "field value").call(VALUE::get); // Compliant | ||
| } | ||
|
|
||
| public String readLocalVarInWhere() { | ||
| ScopedValue<String> value = ScopedValue.newInstance(); | ||
| return ScopedValue.where(value, "local value").call(value::get); // Compliant | ||
| } | ||
|
|
||
| } |
78 changes: 78 additions & 0 deletions
78
java-checks/src/main/java/org/sonar/java/checks/ScopedValueStableReferenceCheck.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) 2012-2025 SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * This program is free software; you can redistribute it and/or | ||
| * modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.sonar.check.Rule; | ||
| import org.sonar.java.checks.methods.AbstractMethodDetection; | ||
| import org.sonar.plugins.java.api.JavaVersion; | ||
| import org.sonar.plugins.java.api.JavaVersionAwareVisitor; | ||
| import org.sonar.plugins.java.api.semantic.MethodMatchers; | ||
| import org.sonar.plugins.java.api.tree.BaseTreeVisitor; | ||
| import org.sonar.plugins.java.api.tree.ExpressionTree; | ||
| import org.sonar.plugins.java.api.tree.MethodInvocationTree; | ||
|
|
||
| @Rule(key = "S8465") | ||
| public class ScopedValueStableReferenceCheck extends AbstractMethodDetection implements JavaVersionAwareVisitor { | ||
|
|
||
| private static final MethodMatchers WHERE_MATCHER = MethodMatchers.create() | ||
| .ofTypes("java.lang.ScopedValue", "java.lang.ScopedValue$Carrier") | ||
| .names("where") | ||
| .withAnyParameters() | ||
| .build(); | ||
|
|
||
| @Override | ||
| public boolean isCompatibleWithJavaVersion(JavaVersion version) { | ||
| return version.isJava25Compatible(); | ||
| } | ||
|
|
||
| @Override | ||
| protected MethodMatchers getMethodInvocationMatchers() { | ||
| return WHERE_MATCHER; | ||
| } | ||
|
|
||
| @Override | ||
| protected void onMethodInvocationFound(MethodInvocationTree mit) { | ||
| ExpressionTree firstArgument = mit.arguments().get(0); | ||
| var finder = new NewInstanceInvocationFinder(); | ||
| firstArgument.accept(finder); | ||
| if (finder.invocation != null) { | ||
| reportIssue(finder.invocation, "Consider using a stable reference for ScopedValue instances."); | ||
| } | ||
| } | ||
|
|
||
| private static class NewInstanceInvocationFinder extends BaseTreeVisitor { | ||
|
|
||
| private static final MethodMatchers NEW_INSTANCE_MATCHER = MethodMatchers.create() | ||
| .ofTypes("java.lang.ScopedValue") | ||
| .names("newInstance") | ||
| .addWithoutParametersMatcher() | ||
| .build(); | ||
|
|
||
| private MethodInvocationTree invocation = null; | ||
|
|
||
| @Override | ||
| public void visitMethodInvocation(MethodInvocationTree tree) { | ||
| if (NEW_INSTANCE_MATCHER.matches(tree.methodSymbol())) { | ||
| invocation = tree; | ||
| return; | ||
| } | ||
| super.visitMethodInvocation(tree); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
44 changes: 44 additions & 0 deletions
44
java-checks/src/test/java/org/sonar/java/checks/ScopedValueStableReferenceCheckTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) 2012-2025 SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * This program is free software; you can redistribute it and/or | ||
| * modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; | ||
|
|
||
| class ScopedValueStableReferenceCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/ScopedValueStableReferenceCheckSample.java")) | ||
| .withCheck(new ScopedValueStableReferenceCheck()) | ||
| .withJavaVersion(25) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void test_java_24() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(mainCodeSourcesPath("checks/ScopedValueStableReferenceCheckSample.java")) | ||
| .withCheck(new ScopedValueStableReferenceCheck()) | ||
| .withJavaVersion(24) | ||
| .verifyNoIssues(); | ||
| } | ||
|
|
||
| } |
54 changes: 54 additions & 0 deletions
54
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8465.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| <p>A <code>ScopedValue</code> must be accessible within the functional task where it is bound. Creating a new instance directly inside a | ||
| <code>ScopedValue.where()</code> call makes the key unreachable, as there is no variable name to reference when calling <code>.get()</code>.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>The primary purpose of a <code>ScopedValue</code> is to provide a way to share data without passing it as method arguments. To retrieve the value | ||
| using <code>.get()</code>, you must have access to the <code>ScopedValue</code> instance (the key).</p> | ||
| <p>If a <code>ScopedValue</code> is instantiated anonymously within the <code>.where()</code> method, it is "lost" immediately after the binding is | ||
| created. The code inside the <code>Runnable</code> or <code>Callable</code> has no way to reference that specific instance to retrieve the associated | ||
| value, rendering the scoped value useless.</p> | ||
| <h2>How to fix it</h2> | ||
| <p>Assign the <code>ScopedValue</code> instance to a stable reference — typically a <code>static final</code> field — before using it in a binding. | ||
| This allows different parts of the code to refer to the same key to set and retrieve values.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| public class Renderer { | ||
|
|
||
| public void process() { | ||
| // Noncompliant: The ScopedValue instance is anonymous and unreachable inside the run method | ||
| ScopedValue.where(ScopedValue.newInstance(), "DARK").run(() -> { | ||
| render(); | ||
| }); | ||
| } | ||
|
|
||
| void render() { | ||
| // There is no way to call .get() here because the key is unknown | ||
| } | ||
|
|
||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| public class Renderer { | ||
|
|
||
| // Compliant: The ScopedValue is assigned to a constant that can be referenced elsewhere | ||
| private static final ScopedValue<String> THEME = ScopedValue.newInstance(); | ||
|
|
||
| public void process() { | ||
| ScopedValue.where(THEME, "DARK").run(() -> { | ||
| render(); | ||
| }); | ||
| } | ||
|
|
||
| void render() { | ||
| System.out.println("Theme is: " + THEME.get()); | ||
| } | ||
|
|
||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li><a href="https://openjdk.org/jeps/506">JEP 506: Scoped Values</a></li> | ||
| </ul> | ||
|
|
22 changes: 22 additions & 0 deletions
22
sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S8465.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| { | ||
| "title": "ScopedValue instances should be assigned to a stable reference", | ||
| "type": "BUG", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-8465", | ||
| "sqKey": "S8465", | ||
| "scope": "All", | ||
| "quickfix": "unknown", | ||
| "code": { | ||
| "impacts": { | ||
| "MAINTAINABILITY": "HIGH", | ||
| "RELIABILITY": "MEDIUM" | ||
| }, | ||
| "attribute": "CONVENTIONAL" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -524,6 +524,7 @@ | |
| "S8444", | ||
| "S8445", | ||
| "S8447", | ||
| "S8450" | ||
| "S8450", | ||
| "S8465" | ||
| ] | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.