Skip to content

Commit

Permalink
Add a suggested fix that rewrites
Browse files Browse the repository at this point in the history
  goog.module('some.module');

  goog.require('x.y.Z');

  use(x.y.Z);

to

  goog.module('some.module');

  const Z = goog.require('x.y.Z');

  use(Z);

This fix is not fool-proof (e.g. if there's already an existing variable called Z in the module, this will produce a conflict) but I suspect it will be useful in auto-fixing a lot of existing violations to unblock []

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140658229
  • Loading branch information
tbreisacher authored and blickly committed Dec 1, 2016
1 parent 14c7eab commit 8ce8195
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 9 deletions.
10 changes: 10 additions & 0 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
Expand Up @@ -598,6 +598,16 @@ public DiagnosticGroup forName(String name) {
RhinoErrorReporter.TOO_MANY_TEMPLATE_PARAMS, RhinoErrorReporter.TOO_MANY_TEMPLATE_PARAMS,
VariableReferenceCheck.UNUSED_LOCAL_ASSIGNMENT)); VariableReferenceCheck.UNUSED_LOCAL_ASSIGNMENT));


static final DiagnosticGroup STRICT_MODULE_CHECKS =
DiagnosticGroups.registerGroup(
"strictModuleChecks",
ClosureCheckModule.AT_EXPORT_IN_NON_LEGACY_GOOG_MODULE,
ClosureCheckModule.LET_GOOG_REQUIRE,
ClosureCheckModule.JSDOC_REFERENCE_TO_FULLY_QUALIFIED_IMPORT_NAME,
ClosureCheckModule.JSDOC_REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME,
ClosureCheckModule.REFERENCE_TO_FULLY_QUALIFIED_IMPORT_NAME,
ClosureCheckModule.REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME);

// A diagnostic group appears to be enabled if any of the DiagnosticTypes it // A diagnostic group appears to be enabled if any of the DiagnosticTypes it
// contains are enabled. We need this group so we can distinguish whether // contains are enabled. We need this group so we can distinguish whether
// ANALYZER_CHECKS was directly enabled or only appears to be, because // ANALYZER_CHECKS was directly enabled or only appears to be, because
Expand Down
28 changes: 21 additions & 7 deletions src/com/google/javascript/refactoring/ErrorToFixMapper.java
Expand Up @@ -49,6 +49,8 @@ private ErrorToFixMapper() {} // All static
Pattern.compile("missing require: '([^']+)'"); Pattern.compile("missing require: '([^']+)'");
private static final Pattern DUPLICATE_REQUIRE = private static final Pattern DUPLICATE_REQUIRE =
Pattern.compile("'([^']+)' required more than once\\."); Pattern.compile("'([^']+)' required more than once\\.");
private static final Pattern FULLY_QUALIFIED_NAME =
Pattern.compile("Reference to fully qualified import name '([^']+)'.*");
private static final Pattern USE_SHORT_NAME = private static final Pattern USE_SHORT_NAME =
Pattern.compile(".*Please use the short name '(.*)' instead."); Pattern.compile(".*Please use the short name '(.*)' instead.");


Expand Down Expand Up @@ -99,6 +101,8 @@ public static SuggestedFix getFixForJsError(JSError error, AbstractCompiler comp
return getFixForExtraRequire(error, compiler); return getFixForExtraRequire(error, compiler);
case "JSC_REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME": case "JSC_REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME":
case "JSC_JSDOC_REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME": case "JSC_JSDOC_REFERENCE_TO_SHORT_IMPORT_BY_LONG_NAME_INCLUDING_SHORT_NAME":
case "JSC_REFERENCE_TO_FULLY_QUALIFIED_IMPORT_NAME":
// TODO(tbreisacher): Apply this fix for JSC_JSDOC_REFERENCE_TO_FULLY_QUALIFIED_IMPORT_NAME.
return getFixForReferenceToShortImportByLongName(error, compiler); return getFixForReferenceToShortImportByLongName(error, compiler);
default: default:
return null; return null;
Expand Down Expand Up @@ -189,14 +193,24 @@ private static SuggestedFix getFixForEarlyReference(JSError error, AbstractCompi


private static SuggestedFix getFixForReferenceToShortImportByLongName( private static SuggestedFix getFixForReferenceToShortImportByLongName(
JSError error, AbstractCompiler compiler) { JSError error, AbstractCompiler compiler) {
Matcher m = USE_SHORT_NAME.matcher(error.description); SuggestedFix.Builder fix =
if (m.matches()) { new SuggestedFix.Builder().attachMatchedNodeInfo(error.node, compiler);
return new SuggestedFix.Builder() NodeMetadata metadata = new NodeMetadata(compiler);
.attachMatchedNodeInfo(error.node, compiler) Match match = new Match(error.node, metadata);
.replace(error.node, NodeUtil.newQName(compiler, m.group(1)), compiler)
.build(); Matcher shortNameMatcher = USE_SHORT_NAME.matcher(error.description);
String shortName;
if (shortNameMatcher.matches()) {
shortName = shortNameMatcher.group(1);
} else {
Matcher fullNameMatcher = FULLY_QUALIFIED_NAME.matcher(error.description);
Preconditions.checkState(fullNameMatcher.matches(), error.description);
String namespace = fullNameMatcher.group(1);
shortName = namespace.substring(namespace.lastIndexOf('.') + 1);
fix.addLhsToGoogRequire(match, namespace);
} }
return null;
return fix.replace(error.node, NodeUtil.newQName(compiler, shortName), compiler).build();
} }


private static List<SuggestedFix> getFixesForImplicitlyNullableJsDoc( private static List<SuggestedFix> getFixesForImplicitlyNullableJsDoc(
Expand Down
9 changes: 7 additions & 2 deletions src/com/google/javascript/refactoring/SuggestedFix.java
Expand Up @@ -31,12 +31,10 @@
import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token; import com.google.javascript.rhino.Token;
import com.google.javascript.rhino.jstype.JSType; import com.google.javascript.rhino.jstype.JSType;

import java.util.Collection; import java.util.Collection;
import java.util.Map; import java.util.Map;
import java.util.regex.Matcher; import java.util.regex.Matcher;
import java.util.regex.Pattern; import java.util.regex.Pattern;

import javax.annotation.Nullable; import javax.annotation.Nullable;




Expand Down Expand Up @@ -547,6 +545,13 @@ public Builder deleteArgument(Node n, int position) {
return this; return this;
} }


public Builder addLhsToGoogRequire(Match m, String namespace) {
Node existingNode = findGoogRequireNode(m.getNode(), m.getMetadata(), namespace);
String shortName = namespace.substring(namespace.lastIndexOf('.') + 1);
insertBefore(existingNode, "const " + shortName + " = ");
return this;
}

/** /**
* Adds a goog.require for the given namespace to the file if it does not * Adds a goog.require for the given namespace to the file if it does not
* already exist. * already exist.
Expand Down
50 changes: 50 additions & 0 deletions test/com/google/javascript/refactoring/ErrorToFixMapperTest.java
Expand Up @@ -345,6 +345,7 @@ public void testSortRequiresInGoogModule_const() {
public void testSortRequiresInGoogModule_standalone() { public void testSortRequiresInGoogModule_standalone() {
assertChanges( assertChanges(
LINE_JOINER.join( LINE_JOINER.join(
"/** @fileoverview @suppress {strictModuleChecks} */",
"goog.module('m');", "goog.module('m');",
"", "",
"goog.require('a.c');", "goog.require('a.c');",
Expand All @@ -355,6 +356,7 @@ public void testSortRequiresInGoogModule_standalone() {
"alert(a.b.d());", "alert(a.b.d());",
"alert(a.b.c());"), "alert(a.b.c());"),
LINE_JOINER.join( LINE_JOINER.join(
"/** @fileoverview @suppress {strictModuleChecks} */",
"goog.module('m');", "goog.module('m');",
"", "",
"goog.require('a.b.c');", "goog.require('a.b.c');",
Expand Down Expand Up @@ -612,6 +614,54 @@ public void testStandaloneVarDoesntCrashMissingRequire() {
"class Cat extends Animal {}")); "class Cat extends Animal {}"));
} }


@Test
public void testAddLhsToGoogRequire() {
assertChanges(
LINE_JOINER.join(
"goog.module('m');",
"",
"goog.require('world.util.Animal');",
"",
"class Cat extends world.util.Animal {}"),
LINE_JOINER.join(
"goog.module('m');",
"",
"const Animal = goog.require('world.util.Animal');",
"",
"class Cat extends Animal {}"));
}

@Test
public void testAddLhsToGoogRequire_new() {
assertChanges(
LINE_JOINER.join(
"goog.module('m');",
"",
"goog.require('world.util.Animal');",
"",
"let cat = new world.util.Animal();"),
LINE_JOINER.join(
"goog.module('m');",
"",
"const Animal = goog.require('world.util.Animal');",
"",
"let cat = new Animal();"));
}

@Test
public void testAddLhsToGoogRequire_jsdoc() {
// TODO(tbreisacher): Add "const Animal = " before the goog.require and change
// world.util.Animal to Animal
assertNoChanges(
LINE_JOINER.join(
"goog.module('m');",
"",
"goog.require('world.util.Animal');",
"",
"/** @type {world.util.Animal} */",
"var cat;"));
}

@Test @Test
public void testSwitchToShorthand_JSDoc1() { public void testSwitchToShorthand_JSDoc1() {
assertChanges( assertChanges(
Expand Down

0 comments on commit 8ce8195

Please sign in to comment.