Working "fix all" mode, fixes for false positives

Dan1ve · May 13, 2018 · 064c1b1 · 064c1b1
1 parent 2bcd013
commit 064c1b1
Show file tree

Hide file tree

Showing 13 changed files with 89 additions and 39 deletions.
diff --git a/closure-inspections-plugin.jar b/closure-inspections-plugin.jar
diff --git a/src/de/veihelmann/closureplugin/ClosureDependenciesExtractor.java b/src/de/veihelmann/closureplugin/ClosureDependenciesExtractor.java
@@ -22,7 +22,7 @@ public class ClosureDependenciesExtractor {
     /**
      * Namespaces provided (via goog.provide('x.y.Z') ) in the current file, with their corresponding PSI element.
      */
-    public final SortedMap<String, JSStatement> googRequires = new TreeMap<>(Comparator.naturalOrder());
+    public final SortedMap<String, PsiElement> googRequires = new TreeMap<>(Comparator.naturalOrder());
 
     /**
      * Namespaces required (via goog.require('x.y.Z') ) by the current file, with their corresponding PSI element.

diff --git a/src/de/veihelmann/closureplugin/MissingOrObsoleteGoogRequiresInspection.java b/src/de/veihelmann/closureplugin/MissingOrObsoleteGoogRequiresInspection.java
@@ -3,7 +3,6 @@
 import com.intellij.codeInsight.daemon.GroupNames;
 import com.intellij.codeInspection.LocalInspectionTool;
 import com.intellij.codeInspection.ProblemsHolder;
-import com.intellij.lang.javascript.psi.JSStatement;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiElementVisitor;
 import com.intellij.psi.PsiFile;
@@ -38,7 +37,6 @@ public String getShortName() {
         return "MissingOrObsoleteGoogRequiresInspection";
     }
 
-
     @NotNull
     @Override
     public PsiElementVisitor buildVisitor(@NotNull final ProblemsHolder holder, boolean isOnTheFly) {
@@ -69,7 +67,7 @@ public void visitFile(PsiFile file) {
         }
 
         private void markObsoleteRequires(ClosureDependenciesExtractor extractor) {
-            for (Map.Entry<String, JSStatement> declaredDependency : extractor.googRequires.entrySet()) {
+            for (Map.Entry<String, PsiElement> declaredDependency : extractor.googRequires.entrySet()) {
                 String namespace = declaredDependency.getKey();
                 if (extractor.dependencies.containsKey(namespace)) {
                     continue;

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/CommentDependencyCollector.java b/src/de/veihelmann/closureplugin/dependency_recognizers/CommentDependencyCollector.java
@@ -7,6 +7,7 @@
 
 import java.util.*;
 
+import static de.veihelmann.closureplugin.dependency_recognizers.DependencyRecognizerBase.normalizeNamespace;
 import static java.util.Arrays.asList;
 import static java.util.stream.Collectors.toList;
 
@@ -46,7 +47,10 @@ public void collectTypeDependenciesFromComment(PsiComment psiComment) {
         for (JSDocTag tag : relevantTags) {
 
             Optional<String> typeReferences = extractClosureTypeReference(tag.getValue());
-            typeReferences.ifPresent(rawTypesInComments::add);
+            typeReferences.ifPresent(reference -> {
+                reference = normalizeNamespace(reference);
+                rawTypesInComments.add(reference);
+            });
         }
     }
 

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/ConstructorDependencyRecognizer.java b/src/de/veihelmann/closureplugin/dependency_recognizers/ConstructorDependencyRecognizer.java
@@ -32,7 +32,7 @@ protected boolean doConsumeElement(JSNewExpression newCallElement) {
             if (isInvalidDependency(namespace)) {
                 return false;
             }
-            constructors.put(namespace, child);
+            constructors.put(normalizeNamespace(namespace), child);
             return true;
         }
 

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/DependencyRecognizerBase.java b/src/de/veihelmann/closureplugin/dependency_recognizers/DependencyRecognizerBase.java
@@ -38,7 +38,7 @@ public final boolean consumeElement(PsiElement element) {
     protected abstract boolean doConsumeElement(T element);
 
     protected int childCount(PsiElement element) {
-        return element.getChildren().length + 1;
+        return element.getChildren().length;
     }
 
     protected boolean childType(PsiElement element, int childIndex, Class<?> targetClass) {
@@ -54,12 +54,15 @@ protected boolean childType(PsiElement element, int childIndex, Class<?> targetC
      * contains at least one dot (.).
      */
     protected boolean isInvalidDependency(String namespace) {
-        return !namespace.contains(".") || containsAny(namespace, "(", "[", ".prototype.")
+        return !namespace.contains(".") || namespace.startsWith("location.") || namespace.startsWith("$") || namespace.startsWith("document.") || containsAny(namespace, "(", "[", ".prototype.")
                 || namespace.endsWith(".prototype");
     }
 
+    public static String normalizeNamespace(String namespace) {
+        return namespace.replaceAll("[\n\\s]", "");
+    }
+
     private static boolean containsAny(String input, String... terms) {
         return Arrays.stream(terms).anyMatch(input::contains);
     }
-
 }
diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/ES6BaseClassDependencyRecognizer.java b/src/de/veihelmann/closureplugin/dependency_recognizers/ES6BaseClassDependencyRecognizer.java
@@ -32,7 +32,7 @@ protected boolean doConsumeElement(ES6ClassExpression classExpression) {
             }
             String dependency = matcher.group(1);
             if (!isInvalidDependency(dependency)) {
-                dependencyMap.put(dependency, child);
+                dependencyMap.put(normalizeNamespace(dependency), child);
                 return true;
             }
         }

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/GoogInheritsDependencyRecognizer.java b/src/de/veihelmann/closureplugin/dependency_recognizers/GoogInheritsDependencyRecognizer.java
@@ -22,7 +22,7 @@ protected boolean doConsumeElement(JSCallExpression callElement) {
         // [1] JSArgumentsList
         // [1] JSReferenceExpression : nth-of-type(2) : text
 
-        if (!isStaticMethodCall(callElement) || !callElement.getFirstChild().getText().equals("goog.inherits")) {
+        if (!isStaticMethodCall(callElement) || !normalizeNamespace(callElement.getFirstChild().getText()).equals("goog.inherits")) {
             return false;
         }
 
@@ -36,7 +36,7 @@ protected boolean doConsumeElement(JSCallExpression callElement) {
             }
 
             if (alreadySawOneReference) {
-                dependencies.put(argument.getText(), argument);
+                dependencies.put(normalizeNamespace(argument.getText()), argument);
                 return true;
             }
             alreadySawOneReference = true;

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/GoogRequireOrProvideRecognizer.java b/src/de/veihelmann/closureplugin/dependency_recognizers/GoogRequireOrProvideRecognizer.java
@@ -11,11 +11,11 @@
 
 public class GoogRequireOrProvideRecognizer extends DependencyRecognizerBase<JSCallExpression> {
 
-    private final Map<String, JSStatement> googRequires;
+    private final Map<String, PsiElement> googRequires;
 
     private final Map<String, JSStatement> googProvides;
 
-    public GoogRequireOrProvideRecognizer(Map<String, JSStatement> googRequires, Map<String, JSStatement> googProvides) {
+    public GoogRequireOrProvideRecognizer(Map<String, PsiElement> googRequires, Map<String, JSStatement> googProvides) {
         this.googRequires = googRequires;
         this.googProvides = googProvides;
     }
@@ -39,6 +39,7 @@ protected boolean doConsumeElement(JSCallExpression callElement) {
         String calledMethod = targetMethod.getText();
 
         JSArgumentList argumentList = (JSArgumentList) callElement.getChildren()[1];
+        calledMethod = normalizeNamespace(calledMethod);
         boolean isGoogRequire = calledMethod.equals("goog.require");
         if (argumentList.getArguments().length != 1 || !(isGoogRequire || calledMethod.equals("goog.provide"))) {
             return false;

diff --git a/src/de/veihelmann/closureplugin/dependency_recognizers/MemberDependencyRecognizer.java b/src/de/veihelmann/closureplugin/dependency_recognizers/MemberDependencyRecognizer.java
@@ -43,22 +43,29 @@ protected boolean doConsumeElement(JSReferenceExpression memberCallElement) {
         }
 
         String namespace = memberCallElement.getFirstChild().getText();
+        if (memberCallElement.getText().matches(NAMESPACE_WITH_CONSTANT_AND_OPTIONAL_SUFFIX)) {
+            namespace = memberCallElement.getText();
+        }
 
         if (namespace.equals("goog.provide") || namespace.equals("goog.require")) {
             // Not handled in this class
             return false;
         }
 
+        boolean removedConstant = false;
         while (namespace.matches(NAMESPACE_WITH_CONSTANT_AND_OPTIONAL_SUFFIX)) {
             // Cut of any potential constants appended to the namespace
             namespace = namespace.substring(0, namespace.lastIndexOf("."));
+            removedConstant = true;
         }
 
-        if (namespace.matches(NAMESPACE_WITH_MEMBER_PATTERN)) {
+        if (!removedConstant && namespace.matches(NAMESPACE_WITH_MEMBER_PATTERN)) {
             // e.g. myvar.length (no Type or constant in reference (if following default Closure naming conventions)
             return false;
         }
 
+        namespace = normalizeNamespace(namespace);
+
         if (isInvalidDependency(namespace)) {
             return false;
         }

diff --git a/...mann/closureplugin/dependency_recognizers/StaticMethodOrConstantDependencyRecognizer.java b/...mann/closureplugin/dependency_recognizers/StaticMethodOrConstantDependencyRecognizer.java
@@ -7,10 +7,18 @@
 import com.intellij.psi.PsiElement;
 import de.veihelmann.closureplugin.utils.ListMap;
 
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
 public class StaticMethodOrConstantDependencyRecognizer extends DependencyRecognizerBase<JSCallExpression> {
 
     private static final String NAMESPACE_WITH_CONSTANT_PATTERN = ".+\\.[A-Z_]+$";
 
+    private final static Set<String> WHITELISTED_METHODS = new HashSet<>(Arrays.asList(
+            "substring", "toString", "toLowerCase", "toUpperCase", "split", "slice", "splice", "toLocaleString", "push", "getBBox", "getBrowserEvent", "preventDefault", "concat"
+    ));
+
     protected final ListMap<String, PsiElement> dependencies;
 
     public StaticMethodOrConstantDependencyRecognizer(ListMap<String, PsiElement> constructors) {
@@ -42,11 +50,18 @@ protected boolean doConsumeElement(JSCallExpression callElement) {
             return false;
         }
 
-        String namespace = namespaceWithMethod.substring(0, namespaceWithMethod.lastIndexOf("."));
+        int offsetBeforeMethodOrConstant = namespaceWithMethod.lastIndexOf(".");
+        String namespace = namespaceWithMethod.substring(0, offsetBeforeMethodOrConstant);
 
+        namespace = normalizeNamespace(namespace);
 
         if (namespace.matches(NAMESPACE_WITH_CONSTANT_PATTERN)) {
             namespace = namespace.substring(0, namespace.lastIndexOf("."));
+        } else {
+            String methodName = namespaceWithMethod.substring(offsetBeforeMethodOrConstant + 1);
+            if (WHITELISTED_METHODS.contains(methodName)) {
+                return false;
+            }
         }
 
 
@@ -65,6 +80,8 @@ protected boolean doConsumeElement(JSCallExpression callElement) {
     }
 
     protected boolean isStaticMethodCall(JSCallExpression callElement) {
-        return childCount(callElement) > 1 && childType(callElement, 0, JSReferenceExpression.class) && childType(callElement, 1, JSArgumentList.class) && !callElement.getText().startsWith("this.") && !(callElement.getChildren()[0].getChildren()[0] instanceof JSNewExpression);
+        return childCount(callElement) > 1 && childType(callElement, 0, JSReferenceExpression.class) &&
+                childType(callElement, 1, JSArgumentList.class) && !callElement.getText().startsWith("$") && !callElement.getText().startsWith("this.")
+                && !(callElement.getChildren()[0].getChildren()[0] instanceof JSNewExpression);
     }
 }
diff --git a/src/de/veihelmann/closureplugin/fixes/MissingGoogRequireFix.java b/src/de/veihelmann/closureplugin/fixes/MissingGoogRequireFix.java
@@ -1,16 +1,16 @@
 package de.veihelmann.closureplugin.fixes;
 
 import com.intellij.lang.javascript.psi.JSStatement;
+import com.intellij.openapi.diagnostic.Logger;
+import com.intellij.openapi.editor.Document;
 import com.intellij.openapi.project.Project;
+import com.intellij.psi.PsiDocumentManager;
 import com.intellij.psi.PsiElement;
 import com.intellij.psi.PsiFile;
-import com.intellij.psi.PsiFileFactory;
 import org.jetbrains.annotations.NotNull;
 
-import java.util.Optional;
-import java.util.SortedMap;
+import java.util.*;
 
-import static de.veihelmann.closureplugin.utils.LanguageUtils.JAVASCRIPT;
 import static java.util.Comparator.comparingInt;
 
 /**
@@ -20,11 +20,11 @@ public class MissingGoogRequireFix extends GoogRequireFixBase {
 
     private final String missingNamespace;
 
-    private final SortedMap<String, JSStatement> currentRequires;
+    private final SortedMap<String, PsiElement> currentRequires;
 
     private final SortedMap<String, JSStatement> googProvides;
 
-    public MissingGoogRequireFix(@NotNull PsiElement element, SortedMap<String, JSStatement> currentRequires, String missingNamespace, SortedMap<String, JSStatement> googProvides) {
+    public MissingGoogRequireFix(@NotNull PsiElement element, SortedMap<String, PsiElement> currentRequires, String missingNamespace, SortedMap<String, JSStatement> googProvides) {
         super(element);
         this.missingNamespace = missingNamespace;
         this.currentRequires = currentRequires;
@@ -35,37 +35,59 @@ public MissingGoogRequireFix(@NotNull PsiElement element, SortedMap<String, JSSt
     @Override
     public void invoke(@NotNull Project project, @NotNull PsiFile psiFile, @NotNull PsiElement psiElement, @NotNull PsiElement psiElement1) {
 
-        PsiFileFactory factory = PsiFileFactory.getInstance(project);
-
         if (currentRequires.keySet().contains(missingNamespace)) {
             // Already fixed. This is unexpected here, but it doesn't hurt to check.
             return;
         }
 
-        // Insert new goog. require (a) after existing goog.require, or (b) after goog.provide, or (c) at the top of the file (fallback).
-        Optional<JSStatement> elementToInsertRequireAfter = currentRequires.values().stream().findFirst();
-        if (!elementToInsertRequireAfter.isPresent()) {
-            elementToInsertRequireAfter = googProvides.values().stream().max(comparingInt(PsiElement::getTextOffset));
+        SortedSet<String> allSortedNamespaces = new TreeSet<>(Comparator.naturalOrder());
+        allSortedNamespaces.addAll(currentRequires.keySet());
+        allSortedNamespaces.add(missingNamespace);
+
+        Document document = PsiDocumentManager.getInstance(project).getDocument(psiFile);
+
+        if (document == null) {
+            Logger.getInstance(getClass()).error("Unexpected error: Document is null");
+            return;
         }
+
+        // Remove existing goog.requires
+        document.setText(document.getText().replaceAll("goog\\s*\\.\\s*require\\s*\\([^)]+\\);?[\\n\\r]+", ""));
+
+        // Insert new goog. require (a) after goog.provide, or (b) at the top of the file (fallback).
+        Optional<JSStatement> elementToInsertRequireAfter =
+                googProvides.values().stream().max(comparingInt(PsiElement::getTextOffset));
+
         if (!elementToInsertRequireAfter.isPresent()) {
             elementToInsertRequireAfter = Optional.of((JSStatement) psiFile.getFirstChild());
         }
 
-        String statement = "goog.require('" + missingNamespace + "');";
+        document.insertString(elementToInsertRequireAfter.get().getNextSibling().getTextOffset(), "\n" + buildRequireStatements(allSortedNamespaces));
 
-        // Create a new temp file to let IntelliJ parse the PSI element(s) for our input. Apparently, this is the supposed way to do this.
-        final PsiElement fileElement = factory.createFileFromText("temp.js", JAVASCRIPT, statement);
-        PsiElement newElement = fileElement.getFirstChild();
-        newElement = elementToInsertRequireAfter.get().getParent().addAfter(newElement, elementToInsertRequireAfter.get());
+        currentRequires.put(this.missingNamespace, null);
+    }
 
-        currentRequires.put(missingNamespace, (JSStatement) newElement);
+    private String buildRequireStatements(SortedSet<String> allSortedNamespaces) {
+        StringBuilder builder = new StringBuilder();
+        String lastPrefix = null;
+        for (String namespace : allSortedNamespaces) {
+            if (namespace.contains(".") && !namespace.substring(0, namespace.indexOf(".")).equals(lastPrefix)) {
+                // add empty line between namespaces of different origins.
+                builder.append("\n");
+            }
+            if (namespace.contains(".")) {
+                lastPrefix = namespace.substring(0, namespace.indexOf("."));
+            }
+            builder.append("goog.require('")
+                    .append(namespace)
+                    .append("');\n");
+        }
+        return builder.toString();
     }
 
     @NotNull
     @Override
     public String getText() {
         return "Add goog.require('" + missingNamespace + "')";
     }
-
-
 }
diff --git a/src/de/veihelmann/closureplugin/fixes/ObsoleteRequireFix.java b/src/de/veihelmann/closureplugin/fixes/ObsoleteRequireFix.java
@@ -29,6 +29,4 @@ public void invoke(@NotNull Project project, @NotNull PsiFile psiFile, @NotNull
     public String getText() {
         return "Remove goog.require";
     }
-
-
 }