diff --git a/src/com/google/javascript/jscomp/CheckConformance.java b/src/com/google/javascript/jscomp/CheckConformance.java index 0842b90fd14..8591753596a 100644 --- a/src/com/google/javascript/jscomp/CheckConformance.java +++ b/src/com/google/javascript/jscomp/CheckConformance.java @@ -30,11 +30,13 @@ import java.util.Set; /** - * Provides a framework for checking code against a set of user configured - * conformance rules. The rules are specified by the ConformanceConfig - * proto, which allows for both standard checks (forbidden properties, - * variables, or dependencies) and allow for more complex checks using - * custom rules than specify + * Provides a framework for checking code against a set of user configured conformance rules. The + * rules are specified by the ConformanceConfig proto, which allows for both standard checks + * (forbidden properties, variables, or dependencies) and allow for more complex checks using custom + * rules than specify + * + *

Conformance violations are both reported as compiler errors, and are also reported separately + * to the {cI gue@link ErrorManager} * */ @GwtIncompatible("com.google.protobuf") diff --git a/src/com/google/javascript/jscomp/ConformanceRules.java b/src/com/google/javascript/jscomp/ConformanceRules.java index 82fd9c9f11b..a9e37289558 100644 --- a/src/com/google/javascript/jscomp/ConformanceRules.java +++ b/src/com/google/javascript/jscomp/ConformanceRules.java @@ -128,6 +128,7 @@ public abstract static class AbstractRule implements Rule { @Nullable final Pattern onlyApplyToRegexp; final boolean reportLooseTypeViolations; final TypeMatchingStrategy typeMatchingStrategy; + final Requirement requirement; public AbstractRule(AbstractCompiler compiler, Requirement requirement) throws InvalidRequirementSpec { @@ -149,6 +150,7 @@ public AbstractRule(AbstractCompiler compiler, Requirement requirement) requirement.getOnlyApplyToRegexpList()); reportLooseTypeViolations = requirement.getReportLooseTypeViolations(); typeMatchingStrategy = getTypeMatchingStrategy(requirement); + this.requirement = requirement; } private static TypeMatchingStrategy getTypeMatchingStrategy(Requirement requirement) { @@ -200,10 +202,10 @@ protected abstract ConformanceResult checkConformance( NodeTraversal t, Node n); /** - * @return Whether the specified Node should be checked for conformance, - * according to this rule's whitelist configuration. + * @return Whether the specified Node should be checked for conformance, according to this + * rule's whitelist configuration. */ - protected final boolean shouldCheckConformance(Node n) { + protected final boolean isWhitelistedByRequirement(Node n) { String srcfile = NodeUtil.getSourceName(n); if (srcfile == null) { return true; @@ -229,19 +231,18 @@ private static boolean pathIsInListOrRegexp( @Override public final void check(NodeTraversal t, Node n) { ConformanceResult result = checkConformance(t, n); - if (result.level != ConformanceLevel.CONFORMANCE - && shouldCheckConformance(n)) { - report(t, n, result); + if (result.level != ConformanceLevel.CONFORMANCE) { + report(n, result); } } /** * Report a conformance warning for the given node. + * * @param n The node representing the violating code. * @param result The result representing the confidence of the violation. */ - protected void report( - NodeTraversal t, Node n, ConformanceResult result) { + protected void report(Node n, ConformanceResult result) { DiagnosticType msg; if (severity == Severity.ERROR) { // Always report findings that are errors, even if the types are too loose to be certain. @@ -258,7 +259,14 @@ protected void report( String separator = (result.note.isEmpty()) ? "" : "\n"; - t.report(n, msg, message, separator, result.note); + JSError err = JSError.make(n, msg, message, separator, result.note); + + // Note that shouldReportConformanceViolation has to be called first; this allows + // logging violations that would otherwise be whitelisted. + if (compiler.getErrorManager().shouldReportConformanceViolation(requirement, err) + && isWhitelistedByRequirement(n)) { + compiler.report(err); + } } } diff --git a/src/com/google/javascript/jscomp/ConformanceWhitelister.java b/src/com/google/javascript/jscomp/ConformanceWhitelister.java index f2e96aa5884..a02ca0f109d 100644 --- a/src/com/google/javascript/jscomp/ConformanceWhitelister.java +++ b/src/com/google/javascript/jscomp/ConformanceWhitelister.java @@ -27,7 +27,7 @@ public class ConformanceWhitelister { private ConformanceWhitelister() {} public static ImmutableSet getViolatingPaths( - AbstractCompiler compiler, Node externs, Node ast, Requirement requirement) { + Compiler compiler, Node externs, Node ast, Requirement requirement) { return getConformanceErrors(compiler, externs, ast, requirement) .stream() .map(e -> e.sourceName) @@ -35,7 +35,7 @@ public static ImmutableSet getViolatingPaths( } public static ImmutableSet getViolatingNodes( - AbstractCompiler compiler, Node externs, Node ast, Requirement requirement) { + Compiler compiler, Node externs, Node ast, Requirement requirement) { return getConformanceErrors(compiler, externs, ast, requirement) .stream() .map(e -> e.node) @@ -43,49 +43,35 @@ public static ImmutableSet getViolatingNodes( } public static ImmutableList getConformanceErrors( - AbstractCompiler compiler, Node externs, Node ast, Requirement requirement) { - ConformanceViolationRecordingCompiler recordingCompiler = - new ConformanceViolationRecordingCompiler(compiler); - // Remove existing prefix whitelist entries, but keep regexps (which we don't re-add either). - // TODO(bangert): Check that each regex matches one entry? + Compiler compiler, Node externs, Node ast, Requirement requirement) { Requirement cleanedRequirement = requirement .toBuilder() .clearWhitelist() + .clearWhitelistRegexp() .setSeverity(Severity.ERROR) .build(); // So we only have one type of error. - ConformanceConfig cleanedConfig = ConformanceConfig.newBuilder().addRequirement(cleanedRequirement).build(); - CheckConformance check = - new CheckConformance(recordingCompiler, ImmutableList.of(cleanedConfig)); - check.process(externs, ast); - - return recordingCompiler.getConformanceErrors(); - } - private static class ConformanceViolationRecordingCompiler extends ForwardingCompiler { - private final ImmutableList.Builder conformanceErrors; - - private ConformanceViolationRecordingCompiler(AbstractCompiler abstractCompiler) { - super(abstractCompiler); - conformanceErrors = ImmutableList.builder(); + ErrorManager oldErrorManager = compiler.getErrorManager(); + final ImmutableList.Builder errors = ImmutableList.builder(); + try { + // TODO(bangert): handle invalid conformance requirements + compiler.setErrorManager( + new ThreadSafeDelegatingErrorManager(oldErrorManager) { + @Override + public synchronized boolean shouldReportConformanceViolation( + Requirement requirement, JSError diagnostic) { + errors.add(diagnostic); + return false; + } + }); + CheckConformance check = new CheckConformance(compiler, ImmutableList.of(cleanedConfig)); + check.process(externs, ast); + } finally { + compiler.setErrorManager(oldErrorManager); } - - ImmutableList getConformanceErrors() { - return conformanceErrors.build(); - } - - @Override - public void report(JSError error) { - if (error.getType().equals(CheckConformance.CONFORMANCE_ERROR)) { - conformanceErrors.add(error); - } else if (error.getType().equals(CheckConformance.INVALID_REQUIREMENT_SPEC)) { - throw new IllegalArgumentException("Invalid conformance requirement" + error.description); - } else { - super.report(error); - } - } - + return errors.build(); } } diff --git a/src/com/google/javascript/jscomp/ErrorManager.java b/src/com/google/javascript/jscomp/ErrorManager.java index fbe155a6cc9..54fc16d0262 100644 --- a/src/com/google/javascript/jscomp/ErrorManager.java +++ b/src/com/google/javascript/jscomp/ErrorManager.java @@ -83,4 +83,12 @@ default boolean hasHaltingErrors() { } return false; } + + /** + * Return true if the conformance violation should be reported. This is called before checking the + * whitelist inside {@code requirement}, so it can be used to log whitelisted violations. + */ + default boolean shouldReportConformanceViolation(Requirement requirement, JSError diagnostic) { + return true; + } } diff --git a/src/com/google/javascript/jscomp/ForwardingCompiler.java b/src/com/google/javascript/jscomp/ForwardingCompiler.java deleted file mode 100644 index f1016ee9eb3..00000000000 --- a/src/com/google/javascript/jscomp/ForwardingCompiler.java +++ /dev/null @@ -1,567 +0,0 @@ -/* - * Copyright 2018 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; - -import com.google.common.annotations.VisibleForTesting; -import com.google.common.base.Supplier; -import com.google.common.collect.ImmutableMap; -import com.google.debugging.sourcemap.proto.Mapping.OriginalMapping; -import com.google.javascript.jscomp.CompilerInput.ModuleType; -import com.google.javascript.jscomp.deps.ModuleLoader; -import com.google.javascript.jscomp.parsing.Config; -import com.google.javascript.jscomp.parsing.parser.FeatureSet; -import com.google.javascript.jscomp.parsing.parser.trees.Comment; -import com.google.javascript.jscomp.type.ReverseAbstractInterpreter; -import com.google.javascript.rhino.ErrorReporter; -import com.google.javascript.rhino.InputId; -import com.google.javascript.rhino.Node; -import com.google.javascript.rhino.jstype.JSTypeRegistry; -import java.util.List; -import java.util.Map; -import java.util.Set; -import javax.annotation.Nullable; - -/** - * Forwards calls to another compiler. Subclass this to intercept methods. - */ -class ForwardingCompiler extends AbstractCompiler { - private final AbstractCompiler abstractCompiler; - - ForwardingCompiler(AbstractCompiler abstractCompiler) { - this.abstractCompiler = abstractCompiler; - } - - // Below here everything is an auto-generated delegate. - // TODO(bangert): Do we have an @Auto equivalent of that? - - @Override - public void report(JSError error) { - abstractCompiler.report(error); - } - - @Override - public void beforePass(String passName) { - abstractCompiler.beforePass(passName); - } - - @Override - public void afterPass(String passName) { - abstractCompiler.afterPass(passName); - } - - @Override - public CompilerInput getInput(InputId inputId) { - return abstractCompiler.getInput(inputId); - } - - @Nullable - @Override - public SourceFile getSourceFileByName(String sourceName) { - return abstractCompiler.getSourceFileByName(sourceName); - } - - @Nullable - @Override - public Node getScriptNode(String filename) { - return abstractCompiler.getScriptNode(filename); - } - - @Nullable - @Override - public JSModuleGraph getModuleGraph() { - return abstractCompiler.getModuleGraph(); - } - - @Override - public Iterable getInputsInOrder() { - return abstractCompiler.getInputsInOrder(); - } - - @Override - public int getNumberOfInputs() { - return abstractCompiler.getNumberOfInputs(); - } - - @Override - public void addExportedNames(Set exportedVariableNames) { - abstractCompiler.addExportedNames(exportedVariableNames); - } - - @Override - public Set getExportedNames() { - return abstractCompiler.getExportedNames(); - } - - @Override - public void setVariableMap(VariableMap variableMap) { - abstractCompiler.setVariableMap(variableMap); - } - - @Override - public void setPropertyMap(VariableMap propertyMap) { - abstractCompiler.setPropertyMap(propertyMap); - } - - @Override - public void setStringMap(VariableMap stringMap) { - abstractCompiler.setStringMap(stringMap); - } - - @Override - public FunctionNames getFunctionNames() { - return abstractCompiler.getFunctionNames(); - } - - @Override - public void setFunctionNames(FunctionNames functionNames) { - abstractCompiler.setFunctionNames(functionNames); - } - - @Override - public void setCssNames(Map newCssNames) { - abstractCompiler.setCssNames(newCssNames); - } - - @Override - public void setIdGeneratorMap(String serializedIdMappings) { - abstractCompiler.setIdGeneratorMap(serializedIdMappings); - } - - @Override - public IdGenerator getCrossModuleIdGenerator() { - return abstractCompiler.getCrossModuleIdGenerator(); - } - - @Override - public void setAnonymousFunctionNameMap(VariableMap functionMap) { - abstractCompiler.setAnonymousFunctionNameMap(functionMap); - } - - @Override - public boolean hasTypeCheckingRun() { - return abstractCompiler.hasTypeCheckingRun(); - } - - @Override - public void setTypeCheckingHasRun(boolean hasRun) { - abstractCompiler.setTypeCheckingHasRun(hasRun); - } - - @Override - public JSTypeRegistry getTypeRegistry() { - return abstractCompiler.getTypeRegistry(); - } - - @Override - public void clearJSTypeRegistry() { - abstractCompiler.clearJSTypeRegistry(); - } - - @Override - public void forwardDeclareType(String typeName) { - abstractCompiler.forwardDeclareType(typeName); - } - - @Override - public ScopeCreator getTypedScopeCreator() { - return abstractCompiler.getTypedScopeCreator(); - } - - @Override - public TypedScope getTopScope() { - return abstractCompiler.getTopScope(); - } - - @Override - public IncrementalScopeCreator getScopeCreator() { - return abstractCompiler.getScopeCreator(); - } - - @Override - public void putScopeCreator(IncrementalScopeCreator creator) { - abstractCompiler.putScopeCreator(creator); - } - - @Override - public void throwInternalError(String msg, Throwable cause) { - abstractCompiler.throwInternalError(msg, cause); - } - - @Override - public CodingConvention getCodingConvention() { - return abstractCompiler.getCodingConvention(); - } - - @Override - public void reportChangeToEnclosingScope(Node n) { - abstractCompiler.reportChangeToEnclosingScope(n); - } - - @Override - public void reportChangeToChangeScope(Node changeScopeRoot) { - abstractCompiler.reportChangeToChangeScope(changeScopeRoot); - } - - @Override - public void reportFunctionDeleted(Node node) { - abstractCompiler.reportFunctionDeleted(node); - } - - @Override - public CssRenamingMap getCssRenamingMap() { - return abstractCompiler.getCssRenamingMap(); - } - - @Override - public void setCssRenamingMap(CssRenamingMap map) { - abstractCompiler.setCssRenamingMap(map); - } - - @Override - public Node getNodeForCodeInsertion(@Nullable JSModule module) { - return abstractCompiler.getNodeForCodeInsertion(module); - } - - @Override - public TypeValidator getTypeValidator() { - return abstractCompiler.getTypeValidator(); - } - - @Override - public Iterable getTypeMismatches() { - return abstractCompiler.getTypeMismatches(); - } - - @Override - public Iterable getImplicitInterfaceUses() { - return abstractCompiler.getImplicitInterfaceUses(); - } - - @Override - public void setExternExports(String externExports) { - abstractCompiler.setExternExports(externExports); - } - - @Override - public Node parseSyntheticCode(String code) { - return abstractCompiler.parseSyntheticCode(code); - } - - @Override - public Node parseSyntheticCode(String filename, String code) { - return abstractCompiler.parseSyntheticCode(filename, code); - } - - @VisibleForTesting - @Override - public Node parseTestCode(String code) { - return abstractCompiler.parseTestCode(code); - } - - @Override - public String toSource() { - return abstractCompiler.toSource(); - } - - @Override - public String toSource(Node root) { - return abstractCompiler.toSource(root); - } - - @Override - public ErrorReporter getDefaultErrorReporter() { - return abstractCompiler.getDefaultErrorReporter(); - } - - @Override - public ReverseAbstractInterpreter getReverseAbstractInterpreter() { - return abstractCompiler.getReverseAbstractInterpreter(); - } - - @Override - public LifeCycleStage getLifeCycleStage() { - return abstractCompiler.getLifeCycleStage(); - } - - @Override - public void setLifeCycleStage(LifeCycleStage stage) { - abstractCompiler.setLifeCycleStage(stage); - } - - @Override - public Supplier getUniqueNameIdSupplier() { - return abstractCompiler.getUniqueNameIdSupplier(); - } - - @Override - public boolean hasHaltingErrors() { - return abstractCompiler.hasHaltingErrors(); - } - - @Override - public void addChangeHandler(CodeChangeHandler handler) { - abstractCompiler.addChangeHandler(handler); - } - - @Override - public void removeChangeHandler(CodeChangeHandler handler) { - abstractCompiler.removeChangeHandler(handler); - } - - @Override - public void addIndexProvider(IndexProvider indexProvider) { - abstractCompiler.addIndexProvider(indexProvider); - } - - @Override - public T getIndex(Class type) { - return abstractCompiler.getIndex(type); - } - - @Override - public int getChangeStamp() { - return abstractCompiler.getChangeStamp(); - } - - @Override - public List getChangedScopeNodesForPass(String passName) { - return abstractCompiler.getChangedScopeNodesForPass(passName); - } - - @Override - public List getDeletedScopeNodesForPass(String passName) { - return abstractCompiler.getDeletedScopeNodesForPass(passName); - } - - @Override - public void incrementChangeStamp() { - abstractCompiler.incrementChangeStamp(); - } - - @Override - public Node getJsRoot() { - return abstractCompiler.getJsRoot(); - } - - @Override - public boolean hasScopeChanged(Node n) { - return abstractCompiler.hasScopeChanged(n); - } - - @Override - public Config getParserConfig(ConfigContext context) { - return abstractCompiler.getParserConfig(context); - } - - @Override - public void prepareAst(Node root) { - abstractCompiler.prepareAst(root); - } - - @Override - public ErrorManager getErrorManager() { - return abstractCompiler.getErrorManager(); - } - - @Override - public boolean areNodesEqualForInlining(Node n1, Node n2) { - return abstractCompiler.areNodesEqualForInlining(n1, n2); - } - - @Override - public void setHasRegExpGlobalReferences(boolean references) { - abstractCompiler.setHasRegExpGlobalReferences(references); - } - - @Override - public boolean hasRegExpGlobalReferences() { - return abstractCompiler.hasRegExpGlobalReferences(); - } - - @Override - public CheckLevel getErrorLevel(JSError error) { - return abstractCompiler.getErrorLevel(error); - } - - @Override - public void process(CompilerPass pass) { - abstractCompiler.process(pass); - } - - @Override - public Node getRoot() { - return abstractCompiler.getRoot(); - } - - @Override - public CompilerOptions getOptions() { - return abstractCompiler.getOptions(); - } - - @Override - public FeatureSet getFeatureSet() { - return abstractCompiler.getFeatureSet(); - } - - @Override - public void setFeatureSet(FeatureSet fs) { - abstractCompiler.setFeatureSet(fs); - } - - @Override - public void updateGlobalVarReferences( - Map refMapPatch, Node collectionRoot) { - abstractCompiler.updateGlobalVarReferences(refMapPatch, collectionRoot); - } - - @Override - public GlobalVarReferenceMap getGlobalVarReferences() { - return abstractCompiler.getGlobalVarReferences(); - } - - @Override - public CompilerInput getSynthesizedExternsInput() { - return abstractCompiler.getSynthesizedExternsInput(); - } - - @Override - public CompilerInput getSynthesizedExternsInputAtEnd() { - return abstractCompiler.getSynthesizedExternsInputAtEnd(); - } - - @Override - public double getProgress() { - return abstractCompiler.getProgress(); - } - - @Override - public String getLastPassName() { - return abstractCompiler.getLastPassName(); - } - - @Override - public void setProgress(double progress, @Nullable String lastPassName) { - abstractCompiler.setProgress(progress, lastPassName); - } - - @Override - public Node ensureLibraryInjected(String resourceName, boolean force) { - return abstractCompiler.ensureLibraryInjected(resourceName, force); - } - - @Override - public Set getExternProperties() { - return abstractCompiler.getExternProperties(); - } - - @Override - public void setExternProperties(Set externProperties) { - abstractCompiler.setExternProperties(externProperties); - } - - @Override - public void addInputSourceMap(String name, SourceMapInput sourceMap) { - abstractCompiler.addInputSourceMap(name, sourceMap); - } - - @Override - public void addComments(String filename, List comments) { - abstractCompiler.addComments(filename, comments); - } - - @Override - ImmutableMap getExternGetterAndSetterProperties() { - return null; - } - - @Override - void setExternGetterAndSetterProperties( - ImmutableMap externGetterAndSetterProperties) { - - } - - @Override - ImmutableMap getSourceGetterAndSetterProperties() { - return null; - } - - @Override - void setSourceGetterAndSetterProperties( - ImmutableMap externGetterAndSetterProperties) { - - } - - @Override - public List getComments(String filename) { - return abstractCompiler.getComments(filename); - } - - @Override - public ImmutableMap getDefaultDefineValues() { - return abstractCompiler.getDefaultDefineValues(); - } - - @Override - public void setDefaultDefineValues(ImmutableMap values) { - abstractCompiler.setDefaultDefineValues(values); - } - - @Override - public ModuleLoader getModuleLoader() { - return abstractCompiler.getModuleLoader(); - } - - @Override - public ModuleType getModuleTypeByName(String moduleName) { - return abstractCompiler.getModuleTypeByName(moduleName); - } - - @Override - public void setAnnotation(String key, Object object) { - abstractCompiler.setAnnotation(key, object); - } - - @Nullable - @Override - public Object getAnnotation(String key) { - return abstractCompiler.getAnnotation(key); - } - - @Override - public ModuleMetadataMap getModuleMetadataMap() { - return abstractCompiler.getModuleMetadataMap(); - } - - @Override - public void setModuleMetadataMap(ModuleMetadataMap moduleMetadataMap) { - abstractCompiler.setModuleMetadataMap(moduleMetadataMap); - } - - @Override - public String getSourceLine(String sourceName, int lineNumber) { - return abstractCompiler.getSourceLine(sourceName, lineNumber); - } - - @Override - public Region getSourceRegion(String sourceName, int lineNumber) { - return abstractCompiler.getSourceRegion(sourceName, lineNumber); - } - - @Override - public OriginalMapping getSourceMapping(String sourceName, int lineNumber, int columnNumber) { - return abstractCompiler.getSourceMapping(sourceName, lineNumber, columnNumber); - } -} diff --git a/src/com/google/javascript/jscomp/ThreadSafeDelegatingErrorManager.java b/src/com/google/javascript/jscomp/ThreadSafeDelegatingErrorManager.java index aec51edffb2..446521a42b7 100644 --- a/src/com/google/javascript/jscomp/ThreadSafeDelegatingErrorManager.java +++ b/src/com/google/javascript/jscomp/ThreadSafeDelegatingErrorManager.java @@ -72,4 +72,9 @@ public synchronized double getTypedPercent() { return delegated.getTypedPercent(); } + @Override + public synchronized boolean shouldReportConformanceViolation( + Requirement requirement, JSError diagnostic) { + return delegated.shouldReportConformanceViolation(requirement, diagnostic); + } }