From 9b70c91e95f99eb0e34429339380c62497bf902f Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Thu, 20 Oct 2016 15:10:47 -0700 Subject: [PATCH] Avoid conflicting fixes when there are both missing/extra requires and the requires are not sorted correctly. Also changes the linter to not emit any output when --fix is enabled. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=136766272 --- src/com/google/javascript/jscomp/Linter.java | 25 +++---- .../refactoring/ApplySuggestedFixes.java | 60 ++++++++++++--- .../refactoring/FixingErrorManager.java | 13 +++- .../refactoring/ErrorToFixMapperTest.java | 75 ++++++++++++++++++- 4 files changed, 147 insertions(+), 26 deletions(-) diff --git a/src/com/google/javascript/jscomp/Linter.java b/src/com/google/javascript/jscomp/Linter.java index 477bd324f8a..ba7c2462289 100644 --- a/src/com/google/javascript/jscomp/Linter.java +++ b/src/com/google/javascript/jscomp/Linter.java @@ -15,14 +15,12 @@ */ package com.google.javascript.jscomp; -import static com.google.common.collect.ObjectArrays.concat; import static com.google.javascript.jscomp.parsing.Config.JsDocParsing.INCLUDE_DESCRIPTIONS_WITH_WHITESPACE; import com.google.common.collect.ImmutableList; import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.refactoring.ApplySuggestedFixes; -import com.google.javascript.refactoring.ErrorToFixMapper; -import com.google.javascript.refactoring.SuggestedFix; +import com.google.javascript.refactoring.FixingErrorManager; import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; @@ -72,6 +70,14 @@ static void fix(String filename) throws IOException { private static void lint(Path path, boolean fix) throws IOException { SourceFile file = SourceFile.fromFile(path.toString()); Compiler compiler = new Compiler(System.out); + + FixingErrorManager errorManager = null; + if (fix) { + errorManager = new FixingErrorManager(); + compiler.setErrorManager(errorManager); + errorManager.setCompiler(compiler); + } + CompilerOptions options = new CompilerOptions(); options.setLanguage(LanguageMode.ECMASCRIPT8); @@ -87,8 +93,8 @@ private static void lint(Path path, boolean fix) throws IOException { options.setWarningLevel(DiagnosticGroups.CHECK_TYPES, CheckLevel.WARNING); options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, CheckLevel.WARNING); - options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.WARNING); - options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.WARNING); + options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.ERROR); + options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, CheckLevel.ERROR); options.setWarningLevel(DiagnosticGroups.USE_OF_GOOG_BASE, CheckLevel.WARNING); options.setSummaryDetailLevel(0); compiler.setPassConfig(new LintPassConfig(options)); @@ -96,14 +102,7 @@ private static void lint(Path path, boolean fix) throws IOException { SourceFile externs = SourceFile.fromCode("", ""); compiler.compile(ImmutableList.of(externs), ImmutableList.of(file), options); if (fix) { - List fixes = new ArrayList<>(); - for (JSError warning : concat(compiler.getErrors(), compiler.getWarnings(), JSError.class)) { - SuggestedFix suggestedFix = ErrorToFixMapper.getFixForJsError(warning, compiler); - if (suggestedFix != null) { - fixes.add(suggestedFix); - } - } - ApplySuggestedFixes.applySuggestedFixesToFiles(fixes); + ApplySuggestedFixes.applySuggestedFixesToFiles(errorManager.getAllFixes()); } } } diff --git a/src/com/google/javascript/refactoring/ApplySuggestedFixes.java b/src/com/google/javascript/refactoring/ApplySuggestedFixes.java index f09926aa2aa..da125dd42f4 100644 --- a/src/com/google/javascript/refactoring/ApplySuggestedFixes.java +++ b/src/com/google/javascript/refactoring/ApplySuggestedFixes.java @@ -20,19 +20,21 @@ import com.google.common.base.Function; import com.google.common.base.Joiner; +import com.google.common.base.Preconditions; +import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSetMultimap; import com.google.common.collect.Multimaps; import com.google.common.collect.Ordering; import com.google.common.collect.SetMultimap; import com.google.common.io.Files; - import java.io.File; import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.Set; /** @@ -90,14 +92,12 @@ public static void applySuggestedFixesToFiles(Iterable fixes) */ public static Map applySuggestedFixesToCode( Iterable fixes, Map filenameToCodeMap) { - ImmutableSetMultimap.Builder builder = ImmutableSetMultimap.builder(); + ReplacementMap map = new ReplacementMap(); for (SuggestedFix fix : fixes) { - builder.putAll(fix.getReplacements()); + map.putIfNoOverlap(fix); } - SetMultimap replacementsMap = builder.build(); ImmutableMap.Builder newCodeMap = ImmutableMap.builder(); - for (Map.Entry> entry - : Multimaps.asMap(replacementsMap).entrySet()) { + for (Map.Entry> entry : map.entrySet()) { String filename = entry.getKey(); if (!filenameToCodeMap.containsKey(filename)) { throw new IllegalArgumentException("filenameToCodeMap missing code for file: " + filename); @@ -137,15 +137,57 @@ public static String applyCodeReplacements(Iterable replacement * by ORDER_CODE_REPLACEMENTS. */ private static void validateNoOverlaps(List replacements) { + Preconditions.checkState(ORDER_CODE_REPLACEMENTS.isOrdered(replacements)); + if (containsOverlaps(replacements)) { + throw new IllegalArgumentException( + "Found overlap between code replacements!\n" + Joiner.on("\n\n").join(replacements)); + } + } + + /** + * Checks whether the CodeReplacements have any overlap. The replacements must be provided in + * order sorted by start position, as sorted by ORDER_CODE_REPLACEMENTS. + */ + private static boolean containsOverlaps(List replacements) { + Preconditions.checkState(ORDER_CODE_REPLACEMENTS.isOrdered(replacements)); int start = -1; for (CodeReplacement replacement : replacements) { if (replacement.getStartPosition() < start) { - throw new IllegalArgumentException( - "Found overlap between code replacements!\n" + Joiner.on("\n\n").join(replacements)); + return true; } start = Math.max(start, replacement.getStartPosition() + replacement.getLength()); } + return false; } + private static class ReplacementMap { + private final SetMultimap map; + + ReplacementMap() { + this.map = HashMultimap.create(); + } + + void putIfNoOverlap(SuggestedFix fix) { + if (canPut(fix)) { + map.putAll(fix.getReplacements()); + } + } + + private boolean canPut(SuggestedFix fix) { + for (String filename : fix.getReplacements().keys()) { + List replacements = new ArrayList<>(map.get(filename)); + replacements.addAll(fix.getReplacements().get(filename)); + replacements = ORDER_CODE_REPLACEMENTS.sortedCopy(replacements); + if (containsOverlaps(replacements)) { + return false; + } + } + return true; + } + + Set>> entrySet() { + return Multimaps.asMap(map).entrySet(); + } + } private ApplySuggestedFixes() {} } diff --git a/src/com/google/javascript/refactoring/FixingErrorManager.java b/src/com/google/javascript/refactoring/FixingErrorManager.java index 2c9cfa1914d..f7b481ee048 100644 --- a/src/com/google/javascript/refactoring/FixingErrorManager.java +++ b/src/com/google/javascript/refactoring/FixingErrorManager.java @@ -16,13 +16,13 @@ package com.google.javascript.refactoring; -import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ListMultimap; import com.google.javascript.jscomp.AbstractCompiler; import com.google.javascript.jscomp.BasicErrorManager; import com.google.javascript.jscomp.CheckLevel; import com.google.javascript.jscomp.JSError; +import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -49,9 +49,16 @@ public List getFixesForJsError(JSError error) { return fixes.get(error); } - @VisibleForTesting + /** Returns fixes for errors first, then fixes for warnings. */ public Collection getAllFixes() { - return fixes.values(); + Collection fixes = new ArrayList<>(); + for (JSError error : getErrors()) { + fixes.addAll(getFixesForJsError(error)); + } + for (JSError warning : getWarnings()) { + fixes.addAll(getFixesForJsError(warning)); + } + return fixes; } @Override diff --git a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java index e282e95b739..6e273e799f3 100644 --- a/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java +++ b/test/com/google/javascript/refactoring/ErrorToFixMapperTest.java @@ -18,6 +18,7 @@ import static com.google.common.collect.ObjectArrays.concat; import static com.google.common.truth.Truth.assertThat; import static com.google.javascript.jscomp.CheckLevel.ERROR; +import static com.google.javascript.jscomp.CheckLevel.WARNING; import com.google.common.base.Joiner; import com.google.common.collect.ImmutableList; @@ -56,7 +57,7 @@ public void setUp() { options = RefactoringDriver.getCompilerOptions(); options.setWarningLevel(DiagnosticGroups.CHECK_VARIABLES, ERROR); options.setWarningLevel(DiagnosticGroups.DEBUGGER_STATEMENT_PRESENT, ERROR); - options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, ERROR); + options.setWarningLevel(DiagnosticGroups.LINT_CHECKS, WARNING); options.setWarningLevel(DiagnosticGroups.STRICT_MISSING_REQUIRE, ERROR); options.setWarningLevel(DiagnosticGroups.EXTRA_REQUIRE, ERROR); } @@ -463,6 +464,58 @@ public void testMissingRequireInGoogProvideFile() { "alert(new a.b.C());")); } + @Test + public void testMissingRequire_unsorted1() { + // Both the fix for requires being unsorted, and the fix for the missing require, are applied. + // However, the end result is still out of order. + assertChanges( + LINE_JOINER.join( + "goog.module('module');", + "", + "var Xray = goog.require('goog.Xray');", + "var Anteater = goog.require('goog.Anteater');", + "", + "alert(new Anteater());", + "alert(new Xray());", + "alert(new goog.dom.DomHelper());"), + LINE_JOINER.join( + "goog.module('module');", + "", + "var DomHelper = goog.require('goog.dom.DomHelper');", + "var Anteater = goog.require('goog.Anteater');", + "var Xray = goog.require('goog.Xray');", + "", + "alert(new Anteater());", + "alert(new Xray());", + "alert(new DomHelper());")); + } + + @Test + public void testMissingRequire_unsorted2() { + // Both the fix for requires being unsorted, and the fix for the missing require, are applied. + // However, the end result is still out of order. + assertChanges( + LINE_JOINER.join( + "goog.module('module');", + "", + "var DomHelper = goog.require('goog.dom.DomHelper');", + "var Anteater = goog.require('goog.Anteater');", + "", + "alert(new Anteater());", + "alert(new goog.rays.Xray());", + "alert(new DomHelper());"), + LINE_JOINER.join( + "goog.module('module');", + "var Xray = goog.require('goog.rays.Xray');", + "", + "var Anteater = goog.require('goog.Anteater');", + "var DomHelper = goog.require('goog.dom.DomHelper');", + "", + "alert(new Anteater());", + "alert(new Xray());", + "alert(new DomHelper());")); + } + @Test public void testMissingRequireInGoogModule() { assertChanges( @@ -676,6 +729,26 @@ public void testExtraRequire() { "alert(goog.string.parseInt('7'));")); } + @Test + public void testExtraRequire_unsorted() { + // There is also a warning because requires are not sorted. That one is not fixed because + // the fix would conflict with the extra-require fix. + assertChanges( + LINE_JOINER.join( + "goog.require('goog.string');", + "goog.require('goog.object');", + "goog.require('goog.dom');", + "", + "alert(goog.string.parseInt('7'));", + "alert(goog.dom.createElement('div'));"), + LINE_JOINER.join( + "goog.require('goog.string');", + "goog.require('goog.dom');", + "", + "alert(goog.string.parseInt('7'));", + "alert(goog.dom.createElement('div'));")); + } + @Test public void testDuplicateRequire() { assertChanges(