From 7a655affe61b7cced5f1fff99e7c0754b04355a3 Mon Sep 17 00:00:00 2001 From: Tagir Valeev Date: Fri, 17 Jul 2020 18:08:11 +0700 Subject: [PATCH] Report unannotated strings passed to @Nls Part of IDEA-246014 Enhance hardcoded strings inspection GitOrigin-RevId: b6922daf5e67b5bd30cd938b3fbf5cd296f01347 --- .../messages/JavaI18nBundle.properties | 2 + .../codeInspection/i18n/I18nInspection.java | 87 ++++++++++++++++--- .../intellij/codeInspection/i18n/NlsInfo.java | 42 ++++++--- .../inspections/i18n/UseConstant.java | 9 ++ .../inspections/i18n/UseConstantNls.java | 36 ++++++++ .../i18n/I18NInspectionTest.java | 15 ++++ 6 files changed, 169 insertions(+), 22 deletions(-) create mode 100644 plugins/java-i18n/testData/inspections/i18n/UseConstant.java create mode 100644 plugins/java-i18n/testData/inspections/i18n/UseConstantNls.java diff --git a/plugins/java-i18n/resources/messages/JavaI18nBundle.properties b/plugins/java-i18n/resources/messages/JavaI18nBundle.properties index c3b57c695390..ce591f8ae8a5 100644 --- a/plugins/java-i18n/resources/messages/JavaI18nBundle.properties +++ b/plugins/java-i18n/resources/messages/JavaI18nBundle.properties @@ -45,6 +45,7 @@ inspection.error.dialog.title=Error inspection.i18n.display.name=Hard coded strings inspection.i18n.expression.is.invalid.error.message=The I18nized Expression template is not a valid expression inspection.i18n.message.general.with.value=Hardcoded string literal: {0} +inspection.i18n.message.non.localized.passed.to.localized=Reference to non-localized string is used where localized string is expected inspection.i18n.option.ignore.assert=Ignore for assert statement arguments inspection.i18n.option.ignore.assigned.to.constants=Ignore literals assigned to constants inspection.i18n.option.ignore.comment.pattern=Ignore lines containing this comment (pattern in java.util.Pattern format): @@ -54,6 +55,7 @@ inspection.i18n.option.ignore.for.exception.constructor.arguments=Ignore for exc inspection.i18n.option.ignore.for.junit.assert.arguments=Ignore for JUnit assert arguments inspection.i18n.option.ignore.for.specified.exception.constructor.arguments=Ignore for Specified Exception Constructor Arguments inspection.i18n.option.ignore.nls=Ignore if target is not annotated with @Nls +inspection.i18n.option.report.unannotated.refs=Report unannotated references inspection.i18n.option.ignore.nonalphanumerics=Ignore literals which do not contain alphabetic characters inspection.i18n.option.ignore.property.keys=Ignore literals which have value equal to existing property key inspection.i18n.option.ignore.qualified.class.names=Ignore literals which have value equal to existing qualified class name diff --git a/plugins/java-i18n/src/com/intellij/codeInspection/i18n/I18nInspection.java b/plugins/java-i18n/src/com/intellij/codeInspection/i18n/I18nInspection.java index c910ca876ced..5861ecd5d21b 100644 --- a/plugins/java-i18n/src/com/intellij/codeInspection/i18n/I18nInspection.java +++ b/plugins/java-i18n/src/com/intellij/codeInspection/i18n/I18nInspection.java @@ -4,6 +4,7 @@ package com.intellij.codeInspection.i18n; import com.intellij.codeInsight.AnnotationUtil; import com.intellij.codeInsight.externalAnnotation.NonNlsAnnotationProvider; +import com.intellij.codeInsight.intention.AddAnnotationFix; import com.intellij.codeInspection.*; import com.intellij.ide.util.TreeClassChooser; import com.intellij.ide.util.TreeClassChooserFactory; @@ -35,8 +36,10 @@ import com.intellij.util.ObjectUtils; import com.intellij.util.ThreeState; import com.intellij.util.containers.ContainerUtil; import com.siyeh.HardcodedMethodConstants; +import com.siyeh.ig.callMatcher.CallMatcher; import com.siyeh.ig.psiutils.ExpressionUtils; import com.siyeh.ig.psiutils.MethodUtils; +import com.siyeh.ig.psiutils.TypeUtils; import gnu.trove.THashSet; import org.jdom.Element; import org.jetbrains.annotations.NonNls; @@ -63,6 +66,9 @@ import java.util.regex.Pattern; import static com.intellij.codeInsight.AnnotationUtil.CHECK_EXTERNAL; public class I18nInspection extends AbstractBaseUastLocalInspectionTool implements CustomSuppressableInspectionTool { + private static final Set IGNORED = ContainerUtil.immutableSet("", "", "", ""); + private static final CallMatcher IGNORED_METHODS = CallMatcher.exactInstanceCall(CommonClassNames.JAVA_LANG_STRING, "substring", "trim"); + public boolean ignoreForAssertStatements = true; public boolean ignoreForExceptionConstructors = true; @NonNls @@ -72,6 +78,7 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen public boolean ignoreForPropertyKeyReferences = true; public boolean ignoreForNonAlpha = true; private boolean ignoreForAllButNls = false; + public boolean reportUnannotatedReferences = false; public boolean ignoreAssignedToConstants; public boolean ignoreToString; @NonNls public String nonNlsCommentPattern = "NON-NLS"; @@ -250,6 +257,13 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen ignoreForAllButNls = ignoreAllButNls.isSelected(); } }); + final JCheckBox reportRefs = new JCheckBox(JavaI18nBundle.message("inspection.i18n.option.report.unannotated.refs"), reportUnannotatedReferences); + reportRefs.addChangeListener(new ChangeListener() { + @Override + public void stateChanged(@NotNull ChangeEvent e) { + reportUnannotatedReferences = reportRefs.isSelected(); + } + }); final GridBagConstraints gc = new GridBagConstraints(); gc.fill = GridBagConstraints.HORIZONTAL; @@ -261,6 +275,9 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen gc.weighty = 0; panel.add(ignoreAllButNls, gc); + gc.gridy ++; + panel.add(reportRefs, gc); + gc.gridy ++; panel.add(assertStatementsCheckbox, gc); @@ -425,7 +442,7 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen List result = new ArrayList<>(); for (UMethod method : aClass.getMethods()) { - if (method.getSourcePsi() == aClass.getSourcePsi()) { // primary constructor that will not be proccsed other way + if (method.getSourcePsi() == aClass.getSourcePsi()) { // primary constructor that will not be processed other way checkMethodBody(method, manager, isOnTheFly, result); } } @@ -532,13 +549,24 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen return; } + Class[] wantedClasses = ignoreForAllButNls && reportUnannotatedReferences ? + new Class[]{UInjectionHost.class, UAnnotation.class, UCallExpression.class, UReferenceExpression.class} : + new Class[]{UInjectionHost.class, UAnnotation.class}; UElement uElement = - UastContextKt.toUElementOfExpectedTypes(element, UInjectionHost.class, UAnnotation.class); + UastContextKt.toUElementOfExpectedTypes(element, wantedClasses); if (uElement instanceof UInjectionHost) { visitLiteralExpression(element, (UInjectionHost)uElement); return; } + + if (uElement instanceof UCallExpression) { + visitCallExpression(element, (UCallExpression)uElement); + } + + if (uElement instanceof UReferenceExpression) { + visitReferenceExpression(element, (UReferenceExpression)uElement); + } if (uElement instanceof UAnnotation) { //prevent from @SuppressWarnings @@ -550,8 +578,42 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen element.acceptChildren(this); } - private void visitLiteralExpression(@NotNull PsiElement sourcePsi, - @NotNull UInjectionHost expression) { + private void visitCallExpression(@NotNull PsiElement sourcePsi, @NotNull UCallExpression ref) { + PsiMethod target = ref.resolve(); + if (target == null) return; + if (IGNORED_METHODS.methodMatches(target)) return; + UExpression expr = ref; + if (ref.getUastParent() instanceof UQualifiedReferenceExpression) { + expr = (UQualifiedReferenceExpression)ref.getUastParent(); + } + processReferenceToNonLocalized(sourcePsi, expr, target); + } + + private void visitReferenceExpression(@NotNull PsiElement sourcePsi, @NotNull UReferenceExpression ref) { + PsiVariable target = ObjectUtils.tryCast(ref.resolve(), PsiVariable.class); + if (target == null || target instanceof PsiLocalVariable) return; + processReferenceToNonLocalized(sourcePsi, ref, target); + } + + private void processReferenceToNonLocalized(@NotNull PsiElement sourcePsi, @NotNull UExpression ref, PsiModifierListOwner target) { + PsiType type = ref.getExpressionType(); + if (!TypeUtils.isJavaLangString(type)) return; + if (NlsInfo.forModifierListOwner(target) instanceof NlsInfo.Localized) return; + if (NlsInfo.forType(type) instanceof NlsInfo.Localized) return; + + String value = target instanceof PsiVariable ? ObjectUtils.tryCast(((PsiVariable)target).computeConstantValue(), String.class) : null; + + NlsInfo targetInfo = getExpectedNlsInfo(myManager.getProject(), ref, value, new THashSet<>()); + if (targetInfo instanceof NlsInfo.Localized) { + AddAnnotationFix fix = new AddAnnotationFix(((NlsInfo.Localized)targetInfo).suggestAnnotation(target), target, AnnotationUtil.NON_NLS); + String description = JavaI18nBundle.message("inspection.i18n.message.non.localized.passed.to.localized"); + final ProblemDescriptor problem = myManager.createProblemDescriptor( + sourcePsi, description, myOnTheFly, new LocalQuickFix[] {fix}, ProblemHighlightType.GENERIC_ERROR_OR_WARNING); + myProblems.add(problem); + } + } + + private void visitLiteralExpression(@NotNull PsiElement sourcePsi, @NotNull UInjectionHost expression) { String stringValue = getStringValueOfKnownPart(expression); if (StringUtil.isEmptyOrSpaces(stringValue)) { return; @@ -667,10 +729,13 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen } private NlsInfo getExpectedNlsInfo(@NotNull Project project, - @NotNull UInjectionHost expression, - @NotNull String value, + @NotNull UExpression expression, + @Nullable String value, @NotNull Set nonNlsTargets) { - if (ignoreForNonAlpha && !StringUtil.containsAlphaCharacters(value)) { + if (ignoreForNonAlpha && value != null && !StringUtil.containsAlphaCharacters(value)) { + return NlsInfo.nonLocalized(); + } + if (value != null && IGNORED.contains(value.toLowerCase(Locale.ROOT))) { return NlsInfo.nonLocalized(); } @@ -708,7 +773,7 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen return NlsInfo.nonLocalized(); } - private boolean isSuppressedByComment(@NotNull Project project, @NotNull UInjectionHost expression) { + private boolean isSuppressedByComment(@NotNull Project project, @NotNull UExpression expression) { Pattern pattern = myCachedNonNlsPattern; if (pattern != null) { PsiElement sourcePsi = expression.getSourcePsi(); @@ -734,7 +799,7 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen } private boolean shouldIgnoreUsage(@NotNull Project project, - @NotNull String value, + @Nullable String value, @NotNull Set nonNlsTargets, @NotNull UExpression usage) { if (isInNonNlsCall(usage, nonNlsTargets)) { @@ -764,10 +829,10 @@ public class I18nInspection extends AbstractBaseUastLocalInspectionTool implemen if (ignoreForJUnitAsserts && isArgOfJUnitAssertion(usage)) { return true; } - if (ignoreForClassReferences && isClassRef(usage, value)) { + if (ignoreForClassReferences && value != null && isClassRef(usage, value)) { return true; } - if (ignoreForPropertyKeyReferences && !PropertiesImplUtil.findPropertiesByKey(project, value).isEmpty()) { + if (ignoreForPropertyKeyReferences && value != null && !PropertiesImplUtil.findPropertiesByKey(project, value).isEmpty()) { return true; } if (ignoreToString && isToString(usage)) { diff --git a/plugins/java-i18n/src/com/intellij/codeInspection/i18n/NlsInfo.java b/plugins/java-i18n/src/com/intellij/codeInspection/i18n/NlsInfo.java index 43711092819b..0e6a6fdb0214 100644 --- a/plugins/java-i18n/src/com/intellij/codeInspection/i18n/NlsInfo.java +++ b/plugins/java-i18n/src/com/intellij/codeInspection/i18n/NlsInfo.java @@ -21,10 +21,7 @@ import org.jetbrains.annotations.Nullable; import org.jetbrains.uast.*; import org.jetbrains.uast.util.UastExpressionUtils; -import java.util.Collection; -import java.util.List; -import java.util.OptionalInt; -import java.util.Set; +import java.util.*; import java.util.stream.IntStream; /** @@ -40,20 +37,23 @@ public abstract class NlsInfo { * Describes a string that should be localized */ public static final class Localized extends NlsInfo { - private static final Localized NLS = new Localized(Capitalization.NotSpecified, "", ""); - private static final Localized NLS_TITLE = new Localized(Capitalization.Title, "", ""); - private static final Localized NLS_SENTENCE = new Localized(Capitalization.Sentence, "", ""); + private static final Localized NLS = new Localized(Capitalization.NotSpecified, "", "", null); + private static final Localized NLS_TITLE = new Localized(Capitalization.Title, "", "", null); + private static final Localized NLS_SENTENCE = new Localized(Capitalization.Sentence, "", "", null); private final @NotNull Capitalization myCapitalization; private final @NotNull @NonNls String myPrefix; private final @NotNull @NonNls String mySuffix; + private final String myAnnotationName; private Localized(@NotNull Capitalization capitalization, @NotNull @NonNls String prefix, - @NotNull @NonNls String suffix) { + @NotNull @NonNls String suffix, + @Nullable @NonNls String annotationName) { super(ThreeState.YES); myCapitalization = capitalization; myPrefix = prefix; mySuffix = suffix; + myAnnotationName = annotationName; } /** @@ -63,6 +63,14 @@ public abstract class NlsInfo { public @NotNull Capitalization getCapitalization() { return myCapitalization; } + + public @NotNull String suggestAnnotation(PsiElement context) { + if (myAnnotationName != null && + JavaPsiFacade.getInstance(context.getProject()).findClass(myAnnotationName, context.getResolveScope()) != null) { + return myAnnotationName; + } + return AnnotationUtil.NLS; + } /** * @return desired prefix for new property keys @@ -80,11 +88,19 @@ public abstract class NlsInfo { return mySuffix; } - private @NotNull NlsInfo withPrefixAndSuffix(@NotNull String prefix, @NotNull String suffix) { + private @NotNull Localized withPrefixAndSuffix(@NotNull String prefix, @NotNull String suffix) { if (prefix.equals(myPrefix) && suffix.equals(mySuffix)) { return this; } - return new Localized(myCapitalization, prefix, suffix); + return new Localized(myCapitalization, prefix, suffix, myAnnotationName); + } + + private @NotNull Localized withAnnotation(@NotNull PsiAnnotation annotation) { + String qualifiedName = annotation.getQualifiedName(); + if (Objects.equals(qualifiedName, myAnnotationName)) { + return this; + } + return new Localized(myCapitalization, myPrefix, mySuffix, qualifiedName); } } @@ -161,6 +177,10 @@ public abstract class NlsInfo { return fromArgument(expression); } + public static @NotNull NlsInfo forType(@NotNull PsiType type) { + return fromAnnotationOwner(type); + } + public static @NotNull NlsInfo forModifierListOwner(@NotNull PsiModifierListOwner owner) { if (owner instanceof PsiParameter) { PsiElement scope = ((PsiParameter)owner).getDeclarationScope(); @@ -399,7 +419,7 @@ public abstract class NlsInfo { } } if (baseInfo instanceof Localized) { - return ((Localized)baseInfo).withPrefixAndSuffix(prefix, suffix); + return ((Localized)baseInfo).withPrefixAndSuffix(prefix, suffix).withAnnotation(annotation); } return baseInfo; } diff --git a/plugins/java-i18n/testData/inspections/i18n/UseConstant.java b/plugins/java-i18n/testData/inspections/i18n/UseConstant.java new file mode 100644 index 000000000000..bf1d2b709b36 --- /dev/null +++ b/plugins/java-i18n/testData/inspections/i18n/UseConstant.java @@ -0,0 +1,9 @@ +class X { + static final String CONSTANT = "Value"; + + void test() { + use(CONSTANT); + } + + void use(String c) {} +} \ No newline at end of file diff --git a/plugins/java-i18n/testData/inspections/i18n/UseConstantNls.java b/plugins/java-i18n/testData/inspections/i18n/UseConstantNls.java new file mode 100644 index 000000000000..c2aea73bb922 --- /dev/null +++ b/plugins/java-i18n/testData/inspections/i18n/UseConstantNls.java @@ -0,0 +1,36 @@ +import org.jetbrains.annotations.Nls; + +class X { + static final String CONSTANT = "Value"; + static final String EMPTY = " "; + + void test() { + use(CONSTANT); + use(EMPTY); + } + + void testParameter(String s) { + use(s); + } + + void testCall() { + use(getNonAnnotated()); + use(this.getNonAnnotated()); + } + + void testNested(String s) { + use(nlsResult(s.trim())); + } + + @Nls String getNested(String s) { + return X.nlsResultStatic(s.trim()); + } + + native String getNonAnnotated(); + + void use(@Nls String c) {} + + native static @Nls String nlsResultStatic(String nonNlsParam); + + native @Nls String nlsResult(String nonNlsParam); +} \ No newline at end of file diff --git a/plugins/java-i18n/testSrc/com/intellij/codeInspection/i18n/I18NInspectionTest.java b/plugins/java-i18n/testSrc/com/intellij/codeInspection/i18n/I18NInspectionTest.java index a4eb17760a3f..39f4430076bf 100644 --- a/plugins/java-i18n/testSrc/com/intellij/codeInspection/i18n/I18NInspectionTest.java +++ b/plugins/java-i18n/testSrc/com/intellij/codeInspection/i18n/I18NInspectionTest.java @@ -14,6 +14,7 @@ public class I18NInspectionTest extends LightJavaCodeInsightFixtureTestCase { I18nInspection myTool = new I18nInspection(); private void doTest() { + myTool.reportUnannotatedReferences = true; myFixture.enableInspections(myTool); myFixture.testHighlighting("i18n/" + getTestName(false) + ".java"); } @@ -145,6 +146,20 @@ public class I18NInspectionTest extends LightJavaCodeInsightFixtureTestCase { myTool.setIgnoreForAllButNls(old); } } + + public void testUseConstant() { + doTest(); + } + + public void testUseConstantNls() { + boolean old = myTool.setIgnoreForAllButNls(true); + try { + doTest(); + } + finally { + myTool.setIgnoreForAllButNls(old); + } + } @Override protected String getTestDataPath() {