Skip to content
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

StatementSwitchToExpressionSwitch: only trigger on compatible target versions #3646

Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,16 @@

package com.google.errorprone.util;

/** JDK version string utilities. */
/**
* JDK runtime version utilities.
*
* <p>These methods are generally used when deciding which method to call reflectively. Bug checkers
* that rely on support for specific source code constructs should consult {@link SourceVersion}
* instead.
*
* @see RuntimeVersion
*/
public final class RuntimeVersion {

private static final int FEATURE = Runtime.version().feature();

/** Returns true if the current runtime is JDK 12 or newer. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright 2023 The Error Prone Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.errorprone.util;

import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.sun.tools.javac.code.Source;
import com.sun.tools.javac.code.Source.Feature;
import com.sun.tools.javac.util.Context;
import java.util.Arrays;

/**
* JDK source version utilities.
*
* @see RuntimeVersion
*/
public final class SourceVersion {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** Returns true if the compiler source version level supports switch expressions. */
public static boolean supportsSwitchExpressions(Context context) {
return supportsFeature("SWITCH_EXPRESSION", context);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for adding this method is twofold:

  1. With two examples it should be clear for future devs how other methods may be added.
  2. I have a use case for this particular feature in Update all tests to use text blocks PicnicSupermarket/error-prone-support#198.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


/**
* Returns true if the compiler source version level supports the {@link Feature} indicated by the
* specified string.
*
* @apiNote For features explicitly recognized by this class, prefer calling the associated method
* instead.
*/
public static boolean supportsFeature(String featureString, Context context) {
Feature feature = KNOWN_FEATURES.get(featureString);
return feature != null && feature.allowedInSource(Source.instance(context));
}

private SourceVersion() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.Reachability;
import com.google.errorprone.util.RuntimeVersion;
import com.google.errorprone.util.SourceVersion;
import com.sun.source.tree.BreakTree;
import com.sun.source.tree.CaseTree;
import com.sun.source.tree.ExpressionTree;
Expand Down Expand Up @@ -85,8 +86,7 @@ public StatementSwitchToExpressionSwitch(ErrorProneFlags flags) {

@Override
public Description matchSwitch(SwitchTree switchTree, VisitorState state) {
// Expression switches finalized in Java 14
if (!RuntimeVersion.isAtLeast14()) {
if (!SourceVersion.supportsSwitchExpressions(state.context)) {
Comment on lines -88 to +89
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return NO_MATCH;
}

Expand Down