From 2404a9c990b5ec14c6cc599eaa5dc07236e615a5 Mon Sep 17 00:00:00 2001 From: Yaroslav Pankratyev Date: Wed, 27 Jun 2018 16:52:57 +0700 Subject: [PATCH] Partially implemented StringToUpperWithoutLocale2Inspection (w/o registration) --- .../codeInspection/NonNlsUastUtil.java | 69 ++++++++++++--- .../jvm/analysis/JvmAnalysisBundle.properties | 5 +- .../StringToUpperWithoutLocale2.html | 11 +++ ...StringToUpperWithoutLocale2Inspection.java | 84 +++++++++++++++++++ .../toUpperWithoutLocale/NonNlsCases.java | 79 +++++++++++++++++ .../toUpperWithoutLocale/SimpleCases.java | 19 +++++ ...ingToUpperWithoutLocaleInspectionTest.java | 26 ++++++ .../codeInspection/KtNonNlsUastUtilTest.java | 8 ++ ...oUpperWithoutLocaleInspectionTestBase.java | 26 ++++++ 9 files changed, 313 insertions(+), 14 deletions(-) create mode 100644 jvm/jvm-analysis-impl/resources/inspectionDescriptions/StringToUpperWithoutLocale2.html create mode 100644 jvm/jvm-analysis-impl/src/com/intellij/codeInspection/StringToUpperWithoutLocale2Inspection.java create mode 100644 jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/NonNlsCases.java create mode 100644 jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/SimpleCases.java create mode 100644 jvm/jvm-analysis-java-tests/testSrc/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTest.java create mode 100644 jvm/jvm-analysis-tests-api/src/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTestBase.java diff --git a/jvm/jvm-analysis-api/src/com/intellij/codeInspection/NonNlsUastUtil.java b/jvm/jvm-analysis-api/src/com/intellij/codeInspection/NonNlsUastUtil.java index 9ed093cf81f1..83f4ec12b602 100644 --- a/jvm/jvm-analysis-api/src/com/intellij/codeInspection/NonNlsUastUtil.java +++ b/jvm/jvm-analysis-api/src/com/intellij/codeInspection/NonNlsUastUtil.java @@ -4,6 +4,7 @@ package com.intellij.codeInspection; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.psi.PsiElement; import com.intellij.psi.PsiMethod; +import com.intellij.psi.PsiModifierListOwner; import com.intellij.psi.PsiParameter; import com.siyeh.HardcodedMethodConstants; import com.siyeh.ig.psiutils.MethodUtils; @@ -26,6 +27,15 @@ public final class NonNlsUastUtil { return element instanceof UAnnotated && ((UAnnotated)element).findAnnotation(AnnotationUtil.NON_NLS) != null; } + /** + * @return true if expression qualifier (part before ::) should be considered non-localizable; + * false otherwise. + */ + public static boolean isCallableReferenceExpressionWithNonNlsQualifier(@Nullable UCallableReferenceExpression expression) { + if (expression == null) return false; + return false; //TODO implement + } + /** * @return true if expression receiver should be considered non-localizable; false otherwise. */ @@ -40,6 +50,10 @@ public final class NonNlsUastUtil { if (receiver instanceof USimpleNameReferenceExpression) { // reference to variable/field/parameter return isReferenceToNonNlsElement((USimpleNameReferenceExpression)receiver); } + if (receiver instanceof UCallExpression || receiver instanceof UQualifiedReferenceExpression) { // call chain + PsiElement resolved = ((UResolvable)receiver).resolve(); + return resolved != null && isNonNlsAnnotatedPsi(resolved); + } UElement expressionParent = expression.getUastParent(); if (expressionParent instanceof UVariable && isNonNlsAnnotated(expressionParent)) { // initialized field/variable @@ -57,27 +71,43 @@ public final class NonNlsUastUtil { if (!UastLiteralUtils.isStringLiteral(expression)) return false; if (isPlacedInNonNlsClass(expression) || isPlacedInNonNlsPackage(expression)) return true; - UElement parent = expression.getUastParent(); - if (parent instanceof UPolyadicExpression && !(parent instanceof UBinaryExpression)) { // multiline string literals - parent = parent.getUastParent(); - } + UElement parent = UastUtils.getParentOfType(expression, + true, + UExpressionList.class, + UVariable.class, + UReturnExpression.class, + UCallExpression.class, + UBinaryExpression.class); if (parent instanceof UField || parent instanceof ULocalVariable || parent instanceof UParameter) { return isNonNlsAnnotated(parent); } - else if (parent instanceof UBinaryExpression) { // e.g. assigning value to parameter - UExpression leftOperand = ((UBinaryExpression)parent).getLeftOperand(); - if (leftOperand instanceof USimpleNameReferenceExpression) { - return isReferenceToNonNlsElement((USimpleNameReferenceExpression)leftOperand); - } - } - else if (parent instanceof UReturnExpression) { + if (parent instanceof UReturnExpression) { return isReturnExpressionInNonNlsMethod((UReturnExpression)parent); } - else if (parent instanceof UCallExpression) { + if (parent instanceof UCallExpression) { return isNonNlsArgument(expression, (UCallExpression)parent) || isCallExpressionWithNonNlsReceiver((UCallExpression)parent); } + + while (parent instanceof UBinaryExpression) { + if (!isAssignmentExpression((UBinaryExpression)parent)) { + // go upper to find assignment expression (or field/variable declaration) + parent = UastUtils.getParentOfType(parent, true, UBinaryExpression.class, UVariable.class); + if (parent instanceof UVariable) { + return isNonNlsAnnotated(parent); + } + continue; + } + + UExpression leftOperand = ((UBinaryExpression)parent).getLeftOperand(); + //TODO probably this doesn't cover all cases + return leftOperand instanceof USimpleNameReferenceExpression && + isReferenceToNonNlsElement((USimpleNameReferenceExpression)leftOperand); + } + + //TODO UExpressionList? + return false; } @@ -116,7 +146,7 @@ public final class NonNlsUastUtil { private static boolean isNonNlsArgument(@NotNull ULiteralExpression argument, @NotNull UCallExpression callExpression) { PsiParameter parameter = UastUtils.getParameterForArgument(callExpression, argument); if (parameter == null) return false; - if (AnnotationUtil.findAnnotation(parameter, AnnotationUtil.NON_NLS) != null) return true; + if (isNonNlsAnnotatedPsi(parameter)) return true; // special handling for equals() if (!HardcodedMethodConstants.EQUALS.equals(callExpression.getMethodName())) return false; @@ -124,4 +154,17 @@ public final class NonNlsUastUtil { if (!MethodUtils.isEquals(method)) return false; return isCallExpressionWithNonNlsReceiver(callExpression); } + + //TODO looks dirty, is there a better way to check this? <-- see UElementAsPsiInspection + private static boolean isAssignmentExpression(@NotNull UBinaryExpression expression) { + UIdentifier operatorIdentifier = expression.getOperatorIdentifier(); + if (operatorIdentifier == null) return false; + String operatorName = operatorIdentifier.getName(); + return operatorName.equals("=") || operatorName.equals("+="); + } + + private static boolean isNonNlsAnnotatedPsi(@NotNull PsiElement element) { + return element instanceof PsiModifierListOwner && + AnnotationUtil.findAnnotation((PsiModifierListOwner)element, AnnotationUtil.NON_NLS) != null; + } } diff --git a/jvm/jvm-analysis-impl/resources/com/intellij/jvm/analysis/JvmAnalysisBundle.properties b/jvm/jvm-analysis-impl/resources/com/intellij/jvm/analysis/JvmAnalysisBundle.properties index f8e3c0ec18f8..43d06f9c4345 100644 --- a/jvm/jvm-analysis-impl/resources/com/intellij/jvm/analysis/JvmAnalysisBundle.properties +++ b/jvm/jvm-analysis-impl/resources/com/intellij/jvm/analysis/JvmAnalysisBundle.properties @@ -2,4 +2,7 @@ jvm.inspections.group.name=JVM languages jvm.inspections.unstable.api.usage.display.name=Unstable API Usage jvm.inspections.unstable.api.usage.annotations.list=Unstable API annotations -jvm.inspections.unstable.api.usage.description=''{0}'' is marked unstable \ No newline at end of file +jvm.inspections.unstable.api.usage.description=''{0}'' is marked unstable + +jvm.inspections.string.touppercase.tolowercase.without.locale.display.name=Call to 'String.toUpperCase()' or 'toLowerCase()' without a Locale +jvm.inspections.string.touppercase.tolowercase.without.locale.description=String.{0}() called without specifying a Locale using internationalized strings #loc diff --git a/jvm/jvm-analysis-impl/resources/inspectionDescriptions/StringToUpperWithoutLocale2.html b/jvm/jvm-analysis-impl/resources/inspectionDescriptions/StringToUpperWithoutLocale2.html new file mode 100644 index 000000000000..9b9cac6a0829 --- /dev/null +++ b/jvm/jvm-analysis-impl/resources/inspectionDescriptions/StringToUpperWithoutLocale2.html @@ -0,0 +1,11 @@ + + +Reports any call of toUpperCase() or +toLowerCase() on String objects which +do not specify a java.util.Locale. +Such calls are usually incorrect in an internationalized environment. + +

+ + + \ No newline at end of file diff --git a/jvm/jvm-analysis-impl/src/com/intellij/codeInspection/StringToUpperWithoutLocale2Inspection.java b/jvm/jvm-analysis-impl/src/com/intellij/codeInspection/StringToUpperWithoutLocale2Inspection.java new file mode 100644 index 000000000000..8acebc77bef0 --- /dev/null +++ b/jvm/jvm-analysis-impl/src/com/intellij/codeInspection/StringToUpperWithoutLocale2Inspection.java @@ -0,0 +1,84 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.codeInspection; + +import com.intellij.analysis.JvmAnalysisBundle; +import com.intellij.psi.CommonClassNames; +import com.intellij.psi.PsiElement; +import com.intellij.psi.PsiElementVisitor; +import com.intellij.psi.PsiIdentifier; +import com.siyeh.HardcodedMethodConstants; +import org.jetbrains.annotations.Nls; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.uast.UCallExpression; +import org.jetbrains.uast.UCallableReferenceExpression; +import org.jetbrains.uast.UElement; +import org.jetbrains.uast.UastContextKt; + + +public class StringToUpperWithoutLocale2Inspection extends AbstractBaseUastLocalInspectionTool { + @Nls + @NotNull + @Override + public String getDisplayName() { //TODO remove once inspection is registered in JvmAnalysisPlugin.xml + return "StringToUpperWithoutLocale2Inspection"; + } + + private static final UastCallMatcher MATCHER = UastCallMatcher.anyOf( + UastCallMatcher.builder() + .withMethodName(HardcodedMethodConstants.TO_UPPER_CASE) + .withClassFqn(CommonClassNames.JAVA_LANG_STRING) + .withArgumentsCount(0).build(), + UastCallMatcher.builder() + .withMethodName(HardcodedMethodConstants.TO_LOWER_CASE) + .withClassFqn(CommonClassNames.JAVA_LANG_STRING) + .withArgumentsCount(0).build() + ); + + @NotNull + @Override + public PsiElementVisitor buildVisitor(@NotNull ProblemsHolder holder, boolean isOnTheFly) { + return new PsiElementVisitor() { + @Override + public void visitElement(PsiElement element) { + UCallExpression callExpression = AnalysisUastUtil.getUCallExpression(element); + if (callExpression != null) { + handleCallExpression(callExpression, holder); + return; + } + + if (!(element instanceof PsiIdentifier)) return; + PsiElement parent = element.getParent(); + UElement parentUElement = UastContextKt.toUElement(parent); + if (parentUElement instanceof UCallableReferenceExpression) { + handleCallableReferenceExpression((UCallableReferenceExpression)parentUElement, element, holder); + } + } + }; + } + + private static void handleCallExpression(@NotNull UCallExpression callExpression, @NotNull ProblemsHolder holder) { + if (!MATCHER.testCallExpression(callExpression)) return; + if (NonNlsUastUtil.isCallExpressionWithNonNlsReceiver(callExpression)) return; + + PsiElement methodIdentifierPsi = AnalysisUastUtil.getMethodIdentifierSourcePsi(callExpression); + if (methodIdentifierPsi == null) return; + + String methodName = callExpression.getMethodName(); + if (methodName == null) return; // shouldn't happen + holder.registerProblem(methodIdentifierPsi, getErrorDescription(methodName)); + } + + private static void handleCallableReferenceExpression(@NotNull UCallableReferenceExpression expression, + @NotNull PsiElement identifier, + @NotNull ProblemsHolder holder) { + if (!MATCHER.testCallableReferenceExpression(expression)) return; + if (NonNlsUastUtil.isCallableReferenceExpressionWithNonNlsQualifier(expression)) return; + + holder.registerProblem(identifier, getErrorDescription(expression.getCallableName())); + } + + @NotNull + private static String getErrorDescription(@NotNull String methodName) { + return JvmAnalysisBundle.message("jvm.inspections.string.touppercase.tolowercase.without.locale.description", methodName); + } +} diff --git a/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/NonNlsCases.java b/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/NonNlsCases.java new file mode 100644 index 000000000000..8a5376ee0c55 --- /dev/null +++ b/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/NonNlsCases.java @@ -0,0 +1,79 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.codeInspection.aaaNonNlsTest; + +import org.jetbrains.annotations.NonNls; + +@SuppressWarnings({"ResultOfMethodCallIgnored", "MethodMayBeStatic"}) +public class NonNlsCases { + String plainField = "3"; + @NonNls String nonNlsField = "4"; + + String initializedPlainField = "1".toUpperCase() + "2".toLowerCase(); + @NonNls String initializedNonNlsField = "1".toUpperCase() + "2".toLowerCase(); + @NonNls String initializedNonNlsField2 = initializedNonNlsField.toUpperCase(); + + + public void checkFields() { + plainField.toUpperCase(); + String s = plainField.toUpperCase(); + nonNlsField.toUpperCase(); + String ss = nonNlsField.toUpperCase(); + + plainField.concat("1".toUpperCase()); + nonNlsField.concat("1".toUpperCase()); + } + + + public void checkVariables() { + String plain = "123"; + @NonNls String nonNls = "123"; + + plain.toUpperCase(); + nonNls.toUpperCase(); + + plain.concat("1".toUpperCase()); + nonNls.concat("1".toUpperCase()); + + @NonNls String s = "123".toUpperCase(); + s.toLowerCase(); + String ss = "123".toUpperCase(); + ss.toLowerCase(); + } + + + public void checkParameters(@NonNls String nonNlsParam, String plainParam) { + nonNlsParam.toUpperCase(); + plainParam.toUpperCase(); + + plainParam.concat("1".toUpperCase()); + nonNlsParam.concat("1".toUpperCase()); + } + + + public void checkMethods() { + nonNlsMethod().toUpperCase(); + plainMethod().toUpperCase(); + + Clazz clazzInstance = new Clazz(); + clazzInstance.nestedNonNlsMethod().toUpperCase(); + clazzInstance.nestedPlainMethod().toUpperCase(); + } + + + + @NonNls + String nonNlsMethod() { return ""; } + String plainMethod() { return ""; } + + static class Clazz { + @NonNls + String nestedNonNlsMethod() { return ""; } + String nestedPlainMethod() { return ""; } + } + + + @NonNls + static class NonNlsClass { + String s = "123".toUpperCase(); + } +} diff --git a/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/SimpleCases.java b/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/SimpleCases.java new file mode 100644 index 000000000000..b3ca1725f55b --- /dev/null +++ b/jvm/jvm-analysis-java-tests/testData/codeInspection/toUpperWithoutLocale/SimpleCases.java @@ -0,0 +1,19 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.codeInspection; + +import java.util.List; +import java.util.Locale; + +public class SimpleCases { + String s1 = "123".toUpperCase(); + String s2 = "123".toUpperCase(Locale.ENGLISH); + + public void foo() { + String foo = "foo".toUpperCase(); + String bar = "bar".toLowerCase(Locale.US); + } + + public void methodRef(List list) { + list.stream().map(String::toUpperCase); + } +} diff --git a/jvm/jvm-analysis-java-tests/testSrc/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTest.java b/jvm/jvm-analysis-java-tests/testSrc/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTest.java new file mode 100644 index 000000000000..af1f06c461fe --- /dev/null +++ b/jvm/jvm-analysis-java-tests/testSrc/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTest.java @@ -0,0 +1,26 @@ +package com.intellij.codeInspection; + +import com.intellij.jvm.analysis.JvmAnalysisTestsUtil; +import com.intellij.testFramework.TestDataPath; + +@TestDataPath("$CONTENT_ROOT/testData/codeInspection/toUpperWithoutLocale") +public class StringToUpperWithoutLocaleInspectionTest extends StringToUpperWithoutLocaleInspectionTestBase { + @Override + protected String getBasePath() { + return JvmAnalysisTestsUtil.TEST_DATA_PROJECT_RELATIVE_BASE_PATH + "/codeInspection/toUpperWithoutLocale"; + } + + public void testSimpleCases() { + myFixture.testHighlighting("SimpleCases.java"); + //TODO test after quickfix once it's implemented + } + + public void testNonNlsCases() { + //TODO test #4 (about equals()) + //TODO test #5 (about @NonNls in the left side of assignment expression) + //TODO test #6 with assigning method return value to variable? If this is not super hard to support. + myFixture.testHighlighting("NonNlsCases.java"); + } + + //TODO test pkg/subPkg +} diff --git a/jvm/jvm-analysis-kotlin-tests/testSrc/com/intellij/codeInspection/KtNonNlsUastUtilTest.java b/jvm/jvm-analysis-kotlin-tests/testSrc/com/intellij/codeInspection/KtNonNlsUastUtilTest.java index e521b1c60d55..085ac9483de5 100644 --- a/jvm/jvm-analysis-kotlin-tests/testSrc/com/intellij/codeInspection/KtNonNlsUastUtilTest.java +++ b/jvm/jvm-analysis-kotlin-tests/testSrc/com/intellij/codeInspection/KtNonNlsUastUtilTest.java @@ -6,8 +6,10 @@ import com.intellij.testFramework.TestDataPath; import com.intellij.testFramework.builders.JavaModuleFixtureBuilder; import com.intellij.testFramework.fixtures.JavaCodeInsightFixtureTestCase; import com.intellij.util.PathUtil; +import kotlin.KotlinVersion; import org.jetbrains.annotations.NonNls; import org.jetbrains.uast.ULiteralExpression; +import org.junit.Assume; import java.util.Set; @@ -16,6 +18,12 @@ import static com.intellij.codeInspection.NonNlsUastUtil.isNonNlsStringLiteral; @TestDataPath("$CONTENT_ROOT/testData/codeInspection/nonNls") public class KtNonNlsUastUtilTest extends JavaCodeInsightFixtureTestCase { + @Override + protected void setUp() throws Exception { + Assume.assumeTrue(KotlinVersion.CURRENT.isAtLeast(1, 2, 60)); + super.setUp(); + } + @Override protected String getBasePath() { return JvmAnalysisKtTestsUtil.TEST_DATA_PROJECT_RELATIVE_BASE_PATH + "/codeInspection/nonNls"; diff --git a/jvm/jvm-analysis-tests-api/src/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTestBase.java b/jvm/jvm-analysis-tests-api/src/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTestBase.java new file mode 100644 index 000000000000..8d4f5453cfe1 --- /dev/null +++ b/jvm/jvm-analysis-tests-api/src/com/intellij/codeInspection/StringToUpperWithoutLocaleInspectionTestBase.java @@ -0,0 +1,26 @@ +package com.intellij.codeInspection; + +import com.intellij.pom.java.LanguageLevel; +import com.intellij.testFramework.IdeaTestUtil; +import com.intellij.testFramework.builders.JavaModuleFixtureBuilder; +import com.intellij.testFramework.fixtures.JavaCodeInsightFixtureTestCase; +import com.intellij.util.PathUtil; +import org.jetbrains.annotations.NonNls; + +import java.util.stream.Stream; + +public abstract class StringToUpperWithoutLocaleInspectionTestBase extends JavaCodeInsightFixtureTestCase { + @Override + protected void setUp() throws Exception { + super.setUp(); + myFixture.enableInspections(new StringToUpperWithoutLocale2Inspection()); + } + + @Override + protected void tuneFixture(JavaModuleFixtureBuilder moduleBuilder) { + moduleBuilder.setLanguageLevel(LanguageLevel.JDK_1_8); + moduleBuilder.addJdk(IdeaTestUtil.getMockJdk18Path().getPath()); + moduleBuilder.addLibrary("annotations", PathUtil.getJarPathForClass(NonNls.class)); + moduleBuilder.addLibrary("javaUtil", PathUtil.getJarPathForClass(Stream.class)); + } +}