Skip to content

Commit

Permalink
Move "DisambiguateProperties.Warnings" into a top-level class "Proper…
Browse files Browse the repository at this point in the history
…tyRenamingDiagnostics" to clip dependencies between "DisambiguateProperties" and other passes.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=246923257
  • Loading branch information
nreid260 authored and brad4d committed May 7, 2019
1 parent a4fadf7 commit 16cf3a9
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 47 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/AmbiguateProperties.java
Expand Up @@ -439,7 +439,7 @@ public void setAnnotation(Annotation data) {
private void reportInvalidRenameFunction(Node n, String functionName, String message) {
compiler.report(
JSError.make(
n, DisambiguateProperties.Warnings.INVALID_RENAME_FUNCTION, functionName, message));
n, PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION, functionName, message));
}
private static final String WRONG_ARGUMENT_COUNT = " Must be called with 1 or 2 arguments.";
private static final String WANT_STRING_LITERAL = " The first argument must be a string literal.";
Expand Down
7 changes: 4 additions & 3 deletions src/com/google/javascript/jscomp/DiagnosticGroups.java
Expand Up @@ -410,9 +410,10 @@ public static DiagnosticGroup forName(String name) {
CheckAccessControls.CONST_PROPERTY_REASSIGNED_VALUE);

public static final DiagnosticGroup TYPE_INVALIDATION =
DiagnosticGroups.registerGroup("typeInvalidation",
DisambiguateProperties.Warnings.INVALIDATION,
DisambiguateProperties.Warnings.INVALIDATION_ON_TYPE);
DiagnosticGroups.registerGroup(
"typeInvalidation",
PropertyRenamingDiagnostics.INVALIDATION,
PropertyRenamingDiagnostics.INVALIDATION_ON_TYPE);

public static final DiagnosticGroup DUPLICATE_VARS =
DiagnosticGroups.registerGroup("duplicate",
Expand Down
54 changes: 23 additions & 31 deletions src/com/google/javascript/jscomp/DisambiguateProperties.java
Expand Up @@ -95,23 +95,6 @@ class DisambiguateProperties implements CompilerPass {
DisambiguateProperties.class.getName());
private static final Pattern NONWORD_PATTERN = Pattern.compile("[^\\w$]");

static class Warnings {
// TODO(user): {1} and {2} are not exactly useful for most people.
static final DiagnosticType INVALIDATION = DiagnosticType.disabled(
"JSC_INVALIDATION",
"Property disambiguator skipping all instances of property {0} "
+ "because of type {1} node {2}. {3}");

static final DiagnosticType INVALIDATION_ON_TYPE = DiagnosticType.disabled(
"JSC_INVALIDATION_TYPE",
"Property disambiguator skipping instances of property {0} on type {1}. {2}");

// TODO(tbreisacher): Check this in a separate pass, so that users get the error even if
// optimizations are not running.
static final DiagnosticType INVALID_RENAME_FUNCTION =
DiagnosticType.error("JSC_INVALID_RENAME_FUNCTION", "{0} call is invalid: {1}");
}

private final AbstractCompiler compiler;

private final InvalidatingTypes invalidatingTypes;
Expand Down Expand Up @@ -519,8 +502,14 @@ private void handleGetProp(Node n) {
suggestion += Joiner.on("\n").join(errors);
}
}
compiler.report(JSError.make(n, propertiesToErrorFor.get(name),
Warnings.INVALIDATION, name, String.valueOf(type), n.toString(),
compiler.report(
JSError.make(
n,
propertiesToErrorFor.get(name),
PropertyRenamingDiagnostics.INVALIDATION,
name,
String.valueOf(type),
n.toString(),
suggestion));
}
}
Expand Down Expand Up @@ -562,7 +551,7 @@ private void handleObjectLit(Node n) {
JSError.make(
child,
propertiesToErrorFor.get(name),
Warnings.INVALIDATION,
PropertyRenamingDiagnostics.INVALIDATION,
name,
String.valueOf(objlitType),
n.toString(),
Expand Down Expand Up @@ -628,7 +617,7 @@ private void handleClass(Node classNode) {
JSError.make(
member,
propertiesToErrorFor.get(name),
Warnings.INVALIDATION,
PropertyRenamingDiagnostics.INVALIDATION,
name,
String.valueOf(ownerType),
member.toString(),
Expand Down Expand Up @@ -678,7 +667,7 @@ private void handleObjectPattern(Node pattern) {
JSError.make(
stringKey,
propertiesToErrorFor.get(name),
Warnings.INVALIDATION,
PropertyRenamingDiagnostics.INVALIDATION,
name,
String.valueOf(objectPatternType),
stringKey.toString(),
Expand All @@ -693,7 +682,7 @@ private void handlePropertyRenameFunctionCall(Node call, String renameFunctionNa
compiler.report(
JSError.make(
call,
Warnings.INVALID_RENAME_FUNCTION,
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION,
renameFunctionName,
" Must be called with 1 or 2 arguments"));
return;
Expand All @@ -703,7 +692,7 @@ private void handlePropertyRenameFunctionCall(Node call, String renameFunctionNa
compiler.report(
JSError.make(
call,
Warnings.INVALID_RENAME_FUNCTION,
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION,
renameFunctionName,
" The first argument must be a string literal."));
return;
Expand All @@ -715,7 +704,7 @@ private void handlePropertyRenameFunctionCall(Node call, String renameFunctionNa
compiler.report(
JSError.make(
call,
Warnings.INVALID_RENAME_FUNCTION,
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION,
renameFunctionName,
" The first argument must not be a property path."));
return;
Expand Down Expand Up @@ -747,7 +736,7 @@ private void handlePropertyRenameFunctionCall(Node call, String renameFunctionNa
JSError.make(
call,
propertiesToErrorFor.get(propName),
Warnings.INVALIDATION,
PropertyRenamingDiagnostics.INVALIDATION,
propName,
String.valueOf(type),
renameFunctionName,
Expand Down Expand Up @@ -831,11 +820,14 @@ void renameProperties() {
&& checkLevelForProp != CheckLevel.OFF
&& !reported.contains(prop.name)) {
reported.add(prop.name);
compiler.report(JSError.make(
node,
checkLevelForProp,
Warnings.INVALIDATION_ON_TYPE, prop.name,
rootType.toString(), ""));
compiler.report(
JSError.make(
node,
checkLevelForProp,
PropertyRenamingDiagnostics.INVALIDATION_ON_TYPE,
prop.name,
rootType.toString(),
""));
}
}
}
Expand Down
38 changes: 38 additions & 0 deletions src/com/google/javascript/jscomp/PropertyRenamingDiagnostics.java
@@ -0,0 +1,38 @@
/*
* Copyright 2008 The Closure Compiler Authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.javascript.jscomp;

/** Shared diagnotic type related to property renaming. */
final class PropertyRenamingDiagnostics {
// TODO(user): {1} and {2} are not exactly useful for most people.
static final DiagnosticType INVALIDATION =
DiagnosticType.disabled(
"JSC_INVALIDATION",
"Property disambiguator skipping all instances of property {0} "
+ "because of type {1} node {2}. {3}");

static final DiagnosticType INVALIDATION_ON_TYPE =
DiagnosticType.disabled(
"JSC_INVALIDATION_TYPE",
"Property disambiguator skipping instances of property {0} on type {1}. {2}");

// TODO(tbreisacher): Check this in a separate pass, so that users get the error even if
// optimizations are not running.
static final DiagnosticType INVALID_RENAME_FUNCTION =
DiagnosticType.error("JSC_INVALID_RENAME_FUNCTION", "{0} call is invalid: {1}");

private PropertyRenamingDiagnostics() {}
}
17 changes: 12 additions & 5 deletions test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java
Expand Up @@ -18,7 +18,6 @@

import static com.google.common.truth.Truth.assertThat;

import com.google.javascript.jscomp.DisambiguateProperties.Warnings;
import com.google.javascript.rhino.Node;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -1233,22 +1232,30 @@ public void testDontRenamePrototypeWithoutExterns() {

@Test
public void testInvalidRenameFunction_withZeroArgs_causesWarning() {
testError("const p = JSCompiler_renameProperty()", Warnings.INVALID_RENAME_FUNCTION);
testError(
"const p = JSCompiler_renameProperty()",
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION);
}

@Test
public void testInvalidRenameFunction_withThreeArgs_causesWarning() {
testError("const p = JSCompiler_renameProperty('foo', 0, 1)", Warnings.INVALID_RENAME_FUNCTION);
testError(
"const p = JSCompiler_renameProperty('foo', 0, 1)",
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION);
}

@Test
public void testInvalidRenameFunction_withNonStringArg_causesWarning() {
testError("const p = JSCompiler_renameProperty(0)", Warnings.INVALID_RENAME_FUNCTION);
testError(
"const p = JSCompiler_renameProperty(0)",
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION);
}

@Test
public void testInvalidRenameFunction_withPropertyRefInFirstArg_causesWarning() {
testError("const p = JSCompiler_renameProperty('a.b')", Warnings.INVALID_RENAME_FUNCTION);
testError(
"const p = JSCompiler_renameProperty('a.b')",
PropertyRenamingDiagnostics.INVALID_RENAME_FUNCTION);
}

@Test
Expand Down
Expand Up @@ -16,12 +16,11 @@
package com.google.javascript.jscomp;

import static com.google.common.truth.Truth.assertThat;
import static com.google.javascript.jscomp.DisambiguateProperties.Warnings.INVALIDATION;
import static com.google.javascript.jscomp.DisambiguateProperties.Warnings.INVALIDATION_ON_TYPE;
import static com.google.javascript.jscomp.PropertyRenamingDiagnostics.INVALIDATION;
import static com.google.javascript.jscomp.PropertyRenamingDiagnostics.INVALIDATION_ON_TYPE;

import com.google.common.collect.Multimap;
import com.google.javascript.jscomp.CompilerOptions.LanguageMode;
import com.google.javascript.jscomp.DisambiguateProperties.Warnings;
import com.google.javascript.rhino.Node;
import java.util.Collection;
import java.util.HashMap;
Expand Down Expand Up @@ -3605,7 +3604,7 @@ public void testObjectPattern_withTypeMismatch_emitsInvalidationError_aboutType(
"Foo.foobar = 'static property!';",
// cause a type mismatch warning on Foo, preventing disambiguation
"const /** !Foo */ foo = {};")),
error(Warnings.INVALIDATION).withMessageContaining("Foo"));
error(INVALIDATION).withMessageContaining("Foo"));
}

@Test
Expand All @@ -3623,7 +3622,7 @@ public void testObjectPattern_withUnknownType_stringKey_emitsInvalidationError_a
"Foo.foobar = 'static property!';",
// because unknownName is of the unknown type '?', we can't disambiguate foobar
"const {foobar: someRandomName} = unknownName;")),
error(Warnings.INVALIDATION).withMessageContaining("someRandomName"));
error(INVALIDATION).withMessageContaining("someRandomName"));
}

private void testSets(String js, String expected, final String fieldTypes) {
Expand Down
5 changes: 3 additions & 2 deletions test/com/google/javascript/jscomp/IntegrationTest.java
Expand Up @@ -2405,7 +2405,8 @@ public void testDisambiguatePropertiesWithPropertyInvalidationError() {
ImmutableMap.of("a", CheckLevel.ERROR));
options.setRemoveDeadCode(true);
options.setRemoveAbstractMethods(true);
test(options,
test(
options,
lines(
"function fn(x){return x.a;}",
"/** @interface */ function I() {}",
Expand All @@ -2422,7 +2423,7 @@ public void testDisambiguatePropertiesWithPropertyInvalidationError() {
"Foo.prototype.a = function(x) {};",
"function Bar(){}",
"Bar.prototype.a=function(x){};"),
DisambiguateProperties.Warnings.INVALIDATION);
PropertyRenamingDiagnostics.INVALIDATION);
}

@Test
Expand Down

0 comments on commit 16cf3a9

Please sign in to comment.