Skip to content

Commit

Permalink
Avoid conflicting fixes when there are both missing/extra requires an…
Browse files Browse the repository at this point in the history 
…d 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
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Oct 20, 2016
1 parent 943f641 commit 9b70c91
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 26 deletions.
25 changes: 12 additions & 13 deletions src/com/google/javascript/jscomp/Linter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

Expand All @@ -87,23 +93,16 @@ 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));
compiler.disableThreads();
SourceFile externs = SourceFile.fromCode("<Linter externs>", "");
compiler.compile(ImmutableList.<SourceFile>of(externs), ImmutableList.of(file), options);
if (fix) {
List<SuggestedFix> 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());
}
}
}
60 changes: 51 additions & 9 deletions src/com/google/javascript/refactoring/ApplySuggestedFixes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -90,14 +92,12 @@ public static void applySuggestedFixesToFiles(Iterable<SuggestedFix> fixes)
*/
public static Map<String, String> applySuggestedFixesToCode(
Iterable<SuggestedFix> fixes, Map<String, String> filenameToCodeMap) {
ImmutableSetMultimap.Builder<String, CodeReplacement> builder = ImmutableSetMultimap.builder();
ReplacementMap map = new ReplacementMap();
for (SuggestedFix fix : fixes) {
builder.putAll(fix.getReplacements());
map.putIfNoOverlap(fix);
}
SetMultimap<String, CodeReplacement> replacementsMap = builder.build();
ImmutableMap.Builder<String, String> newCodeMap = ImmutableMap.builder();
for (Map.Entry<String, Set<CodeReplacement>> entry
: Multimaps.asMap(replacementsMap).entrySet()) {
for (Map.Entry<String, Set<CodeReplacement>> entry : map.entrySet()) {
String filename = entry.getKey();
if (!filenameToCodeMap.containsKey(filename)) {
throw new IllegalArgumentException("filenameToCodeMap missing code for file: " + filename);
Expand Down Expand Up @@ -137,15 +137,57 @@ public static String applyCodeReplacements(Iterable<CodeReplacement> replacement
* by ORDER_CODE_REPLACEMENTS.
*/
private static void validateNoOverlaps(List<CodeReplacement> 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<CodeReplacement> 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<String, CodeReplacement> 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<CodeReplacement> 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<Entry<String, Set<CodeReplacement>>> entrySet() {
return Multimaps.asMap(map).entrySet();
}
}
private ApplySuggestedFixes() {}
}
13 changes: 10 additions & 3 deletions src/com/google/javascript/refactoring/FixingErrorManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -49,9 +49,16 @@ public List<SuggestedFix> getFixesForJsError(JSError error) {
return fixes.get(error);
}

@VisibleForTesting
/** Returns fixes for errors first, then fixes for warnings. */
public Collection<SuggestedFix> getAllFixes() {
return fixes.values();
Collection<SuggestedFix> fixes = new ArrayList<>();
for (JSError error : getErrors()) {
fixes.addAll(getFixesForJsError(error));
}
for (JSError warning : getWarnings()) {
fixes.addAll(getFixesForJsError(warning));
}
return fixes;
}

@Override
Expand Down
75 changes: 74 additions & 1 deletion test/com/google/javascript/refactoring/ErrorToFixMapperTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 9b70c91

Please sign in to comment.