StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

Stephan202 · 2022-12-31T14:38:57Z

This change introduces a SourceVersion utility class, analogous to the
existing RuntimeVersion class. The new class tells whether selected language
features are available for the source version used by the compiler.

The new utility class is used to make sure that
StatementSwitchToExpressionSwitch flags statement switches only if the source
version supports expression switches, irrespective of whether the current
runtime supports such switch types. Concretely this makes the check compatible
with projects that target JDK 11, yet support building with JDK 17.

…versions This change introduces a `TargetVersion` utility class, analogous to the existing `RuntimeVersion` class. The new class tells whether the current compilation process targets a bytecode version that is generally associated with a given (or more recent) JDK version. The new utility class is used to make sure that `StatementSwitchToExpressionSwitch` flags statement switches only if the target version supports expression switches, irrespective of whether the current runtime supports such switch types. Concretely this makes the check compatible with projects that target JDK 11, yet support building with JDK 17.

cushon · 2022-12-31T14:49:39Z

Thanks, SGTM overall.

I appreciate making TargetVersion match the style of the existing RuntimeVersion, but in hindsight I'm not sure all of the isAtLeastN overloads was a great API, and for this example it might be nice to express it in terms of the feature we want to detect instead of the corresponding version number.

javac has a built-in API for detecting if features are available at the current source version, what do you think about using that instead, e.g.? Feature.SWITCH_EXPRESSION.allowedInSource(Source.instance(context))

That's using the source version instead of the target version, which wouldn't enable the check for configurations like -source 8 -target 19, but I think that's fine.

cushon

(#3646 (comment))

Stephan202

Added some notes.

Stephan202 · 2022-12-31T14:41:09Z

check_api/src/main/java/com/google/errorprone/util/TargetVersion.java

@@ -0,0 +1,88 @@
+/*
+ * Copyright 2023 The Error Prone Authors.


Seems safe to assume that this PR won't be merged before the fireworks ;)

Stephan202 · 2022-12-31T14:42:25Z

pom.xml

            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
-            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
-            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
+            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
+            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED</arg>
            <arg>--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>


Summary of changes:

Sorted.

Removed duplicate com.sun.tools.javac.util entry.

Added com.sun.tools.javac.jvm.

Stephan202 · 2022-12-31T14:55:25Z

pom.xml

@@ -213,6 +213,7 @@
            -Xmx1g
            --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
            --add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
+            --add-exports=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED


If this PR is accepted, then the documentation should also be updated to include this flag.

Stephan202 · 2022-12-31T15:00:20Z

what do you think about using that instead, e.g.?

Interesting idea! Let me have a look a this and report back here 👍

Stephan202

Will check whether this can be nicely tested without too much hassle.

Stephan202 · 2022-12-31T15:34:55Z

check_api/src/main/java/com/google/errorprone/util/SourceVersion.java

+  private static final ImmutableMap<String, Feature> KNOWN_FEATURES =
+      Maps.uniqueIndex(Arrays.asList(Feature.values()), Enum::name);


The string-based lookup is necessary because older runtimes won't recognize newer enum values. A bit fragile, but no more fragile than what's usual for Error Prone.

Thanks, I didn't consider that when making the suggestion. This looks like a good way to deal with that.

One more suggestion would be to keep the current API, but avoid relying on the Feature enum, e.g.:

public static boolean supportsSwitchExpressions(Context context) { return Source.instance(state.context).compareTo(Source.lookup("14")) >= 0; }

That means we can't use javac's knowledge about which features were used in which versions, but it also avoids depending on the string names of the Feature constants, and they're an internal API that could change (e.g. constants will probably be removed as javac drops support for old version features).

Works for me; I can see how this reduces the maintenance burden. The Source.lookup call does require a null check; will keep the extra method for that purpose.

Stephan202 · 2022-12-31T15:36:31Z

check_api/src/main/java/com/google/errorprone/util/SourceVersion.java

+ *
+ * @see RuntimeVersion
+ */
+public final class SourceVersion {


Could also name the class something like SourceFeature(s), though that might perhaps also call for slightly different method names. 🤔

Stephan202 · 2022-12-31T15:36:51Z

core/src/main/java/com/google/errorprone/bugpatterns/StatementSwitchToExpressionSwitch.java

-    // Expression switches finalized in Java 14
-    if (!RuntimeVersion.isAtLeast14()) {
+    if (!SourceVersion.supportsSwitchExpressions(state.context)) {


Dropped the comment because the code is now self-documenting.

Stephan202

Added some unit tests.

Stephan202 · 2022-12-31T16:02:45Z

check_api/src/main/java/com/google/errorprone/util/SourceVersion.java

+  /** Returns true if the compiler source version level supports text blocks. */
+  public static boolean supportsTextBlocks(Context context) {
+    return supportsFeature("TEXT_BLOCKS", context);
+  }


The reason for adding this method is twofold:

With two examples it should be clear for future devs how other methods may be added.

I have a use case for this particular feature in Update all tests to use text blocks PicnicSupermarket/error-prone-support#198.

👍

There is at least one other use of Source in Error Prone that can also be cleaned up to use this approach. (That's just an FYI, it doesn't need to happen in this PR.)

https://cs.github.com/google/error-prone/blob/721096fd16b9f9af7ae7dd657bd9d4ae75db5b46/core/src/main/java/com/google/errorprone/bugpatterns/VarChecker.java?q=repo%3Agoogle%2Ferror-prone++Source.instance#L102

I don't mind adding that to the PR, but I'm not 100% sure what the extra method would have to be called. (My best guess is that this relates to EFFECTIVELY_FINAL_IN_INNER_CLASSES, but I'm not at all sure.)

Stephan202

Applied :)

Stephan202 · 2022-12-31T16:58:19Z

check_api/src/main/java/com/google/errorprone/util/SourceVersion.java

+  /** Returns true if the compiler source version level supports text blocks. */
+  public static boolean supportsTextBlocks(Context context) {
+    return supportsFeature("TEXT_BLOCKS", context);
+  }


I don't mind adding that to the PR, but I'm not 100% sure what the extra method would have to be called. (My best guess is that this relates to EFFECTIVELY_FINAL_IN_INNER_CLASSES, but I'm not at all sure.)

Stephan202 · 2022-12-31T16:59:41Z

check_api/src/main/java/com/google/errorprone/util/SourceVersion.java

+  private static final ImmutableMap<String, Feature> KNOWN_FEATURES =
+      Maps.uniqueIndex(Arrays.asList(Feature.values()), Enum::name);


Works for me; I can see how this reduces the maintenance burden. The Source.lookup call does require a null check; will keep the extra method for that purpose.

Stephan202 · 2022-12-31T17:10:07Z

(Updated the PR description to be in line with the latest revision.)

Tnx for the rapid review iterations @cushon!

…versions This change introduces a `SourceVersion` utility class, analogous to the existing `RuntimeVersion` class. The new class tells whether selected language features are available for the source version used by the compiler. The new utility class is used to make sure that `StatementSwitchToExpressionSwitch` flags statement switches only if the source version supports expression switches, irrespective of whether the current runtime supports such switch types. Concretely this makes the check compatible with projects that target JDK 11, yet support building with JDK 17. Fixes #3646 FUTURE_COPYBARA_INTEGRATE_REVIEW=#3646 from PicnicSupermarket:bugfix/dont-suggest-switch-expressions-pre-jdk14 641c9dc PiperOrigin-RevId: 498755371

This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.google.errorprone:error_prone_core](https://errorprone.info) ([source](https://github.com/google/error-prone)) | | minor | `2.16` -> `2.18.0` | | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | compile | minor | `2.16` -> `2.18.0` | | [org.apache.maven.plugins:maven-failsafe-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | | [org.apache.maven.plugins:maven-surefire-plugin](https://maven.apache.org/surefire/) | build | patch | `3.0.0-M7` -> `3.0.0-M8` | --- ### Release Notes <details> <summary>google/error-prone</summary> ### [`v2.18.0`](https://github.com/google/error-prone/releases/tag/v2.18.0): Error Prone 2.18.0 [Compare Source](google/error-prone@v2.17.0...v2.18.0) New Checkers: - [`InjectOnBugCheckers`](https://errorprone.info/bugpattern/InjectOnBugCheckers) - [`LabelledBreakTarget`](https://errorprone.info/bugpattern/LabelledBreakTarget) - [`UnusedLabel`](https://errorprone.info/bugpattern/UnusedLabel) - [`YodaCondition`](https://errorprone.info/bugpattern/YodaCondition) Fixes issues: [#1650](google/error-prone#1650), [#2706](google/error-prone#2706), [#3404](google/error-prone#3404), [#3493](google/error-prone#3493), [#3504](google/error-prone#3504), [#3519](google/error-prone#3519), [#3579](google/error-prone#3579), [#3610](google/error-prone#3610), [#3632](google/error-prone#3632), [#3638](google/error-prone#3638), [#3645](google/error-prone#3645), [#3646](google/error-prone#3646), [#3652](google/error-prone#3652), [#3690](google/error-prone#3690) **Full Changelog**: google/error-prone@v2.17.0...v2.18.0 ### [`v2.17.0`](https://github.com/google/error-prone/releases/tag/v2.17.0): Error Prone 2.17.0 [Compare Source](google/error-prone@v2.16...v2.17.0) New Checkers: - [`AvoidObjectArrays`](https://errorprone.info/bugpattern/AvoidObjectArrays) - [`Finalize`](https://errorprone.info/bugpattern/Finalize) - [`IgnoredPureGetter`](https://errorprone.info/bugpattern/IgnoredPureGetter) - [`ImpossibleNullComparison`](https://errorprone.info/bugpattern/ProtoFieldNullComparison) - [`MathAbsoluteNegative`](https://errorprone.info/bugpattern/MathAbsoluteNegative) - [`NewFileSystem`](https://errorprone.info/bugpattern/NewFileSystem) - [`StatementSwitchToExpressionSwitch`](https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch) - [`UnqualifiedYield`](https://errorprone.info/bugpattern/UnqualifiedYield) Fixed issues: [#2321](google/error-prone#2321), [#3144](google/error-prone#3144), [#3297](google/error-prone#3297), [#3428](google/error-prone#3428), [#3437](google/error-prone#3437), [#3462](google/error-prone#3462), [#3482](google/error-prone#3482), [#3494](google/error-prone#3494) **Full Changelog**: google/error-prone@v2.16...v2.17.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Stephan202 force-pushed the bugfix/dont-suggest-switch-expressions-pre-jdk14 branch from c7a23da to 2a0bb07 Compare December 31, 2022 14:40

cushon requested changes Dec 31, 2022

View reviewed changes

Stephan202 commented Dec 31, 2022

View reviewed changes

Alternative implementation

42a036c

Stephan202 commented Dec 31, 2022

View reviewed changes

Stephan202 added 2 commits December 31, 2022 16:40

Javadoc doesn't like @apiNote

3690ab1

Unit test proposal

7806808

Stephan202 commented Dec 31, 2022

View reviewed changes

Stephan202 mentioned this pull request Dec 31, 2022

Update all tests to use text blocks PicnicSupermarket/error-prone-support#198

Draft

Avoid reliance on Source.Feature

641c9dc

Stephan202 commented Dec 31, 2022

View reviewed changes

cushon approved these changes Dec 31, 2022

View reviewed changes

copybara-service bot mentioned this pull request Jan 2, 2023

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3657

Closed

copybara-service bot closed this in b5cbee9 Jan 2, 2023

Stephan202 deleted the bugfix/dont-suggest-switch-expressions-pre-jdk14 branch January 3, 2023 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

Stephan202 commented Dec 31, 2022 •

edited

cushon commented Dec 31, 2022

cushon left a comment •

edited

Stephan202 left a comment

Stephan202 Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 commented Dec 31, 2022

Stephan202 left a comment

Stephan202 Dec 31, 2022

cushon Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 left a comment

Stephan202 Dec 31, 2022

cushon Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 left a comment

Stephan202 Dec 31, 2022

Stephan202 Dec 31, 2022

Stephan202 commented Dec 31, 2022

		private static final ImmutableMap<String, Feature> KNOWN_FEATURES =
		Maps.uniqueIndex(Arrays.asList(Feature.values()), Enum::name);

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

Conversation

Stephan202 commented Dec 31, 2022 • edited

cushon commented Dec 31, 2022

cushon left a comment • edited

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Dec 31, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Dec 31, 2022

Stephan202 commented Dec 31, 2022 •

edited

cushon left a comment •

edited