From 7047bed839af81f2f0b436b206283b09810b28cf Mon Sep 17 00:00:00 2001 From: bangert Date: Wed, 20 Jun 2018 16:16:17 -0700 Subject: [PATCH] Refactoring and conformance improvements -Expose non-conformant nodes in ConformanceWhitelister -Provide an API to in c.g.js.refactoring.RefactoringUtils to determine if a file uses the closure library or not. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=201435745 --- .../jscomp/ConformanceWhitelister.java | 24 ++++++++-- .../refactoring/RefactoringUtils.java | 47 +++++++++++++++++++ .../javascript/refactoring/SuggestedFix.java | 25 +--------- .../jscomp/ConformanceWhitelisterTest.java | 26 ++++++---- 4 files changed, 86 insertions(+), 36 deletions(-) create mode 100644 src/com/google/javascript/refactoring/RefactoringUtils.java diff --git a/src/com/google/javascript/jscomp/ConformanceWhitelister.java b/src/com/google/javascript/jscomp/ConformanceWhitelister.java index 5364cce0fd7..f2e96aa5884 100644 --- a/src/com/google/javascript/jscomp/ConformanceWhitelister.java +++ b/src/com/google/javascript/jscomp/ConformanceWhitelister.java @@ -28,6 +28,22 @@ private ConformanceWhitelister() {} public static ImmutableSet getViolatingPaths( AbstractCompiler compiler, Node externs, Node ast, Requirement requirement) { + return getConformanceErrors(compiler, externs, ast, requirement) + .stream() + .map(e -> e.sourceName) + .collect(ImmutableSet.toImmutableSet()); + } + + public static ImmutableSet getViolatingNodes( + AbstractCompiler compiler, Node externs, Node ast, Requirement requirement) { + return getConformanceErrors(compiler, externs, ast, requirement) + .stream() + .map(e -> e.node) + .collect(ImmutableSet.toImmutableSet()); + } + + 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). @@ -45,11 +61,7 @@ public static ImmutableSet getViolatingPaths( new CheckConformance(recordingCompiler, ImmutableList.of(cleanedConfig)); check.process(externs, ast); - ImmutableSet.Builder result = ImmutableSet.builder(); - for (JSError e : recordingCompiler.getConformanceErrors()) { - result.add(e.sourceName); - } - return result.build(); + return recordingCompiler.getConformanceErrors(); } private static class ConformanceViolationRecordingCompiler extends ForwardingCompiler { @@ -68,6 +80,8 @@ ImmutableList getConformanceErrors() { 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); } diff --git a/src/com/google/javascript/refactoring/RefactoringUtils.java b/src/com/google/javascript/refactoring/RefactoringUtils.java new file mode 100644 index 00000000000..2cd981ff95c --- /dev/null +++ b/src/com/google/javascript/refactoring/RefactoringUtils.java @@ -0,0 +1,47 @@ +/* + * 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.refactoring; + +import com.google.javascript.jscomp.NodeUtil; +import com.google.javascript.rhino.Node; + +/** Utility methods for refactoring Java code. */ +public class RefactoringUtils { + private RefactoringUtils() {} + + /** Looks for a goog.require(), goog.provide() or goog.module() call in the node's file. */ + public static boolean isInClosurizedFile(Node node, NodeMetadata metadata) { + Node script = NodeUtil.getEnclosingScript(node); + + if (script == null) { + return false; + } + + Node child = script.getFirstChild(); + while (child != null) { + if (NodeUtil.isExprCall(child)) { + if (Matchers.googRequire().matches(child.getFirstChild(), metadata)) { + return true; + } + // goog.require or goog.module. + } else if (child.isVar() && child.getBooleanProp(Node.IS_NAMESPACE)) { + return true; + } + child = child.getNext(); + } + return false; + } +} diff --git a/src/com/google/javascript/refactoring/SuggestedFix.java b/src/com/google/javascript/refactoring/SuggestedFix.java index 998695cf8e2..c61dd16a500 100644 --- a/src/com/google/javascript/refactoring/SuggestedFix.java +++ b/src/com/google/javascript/refactoring/SuggestedFix.java @@ -177,7 +177,8 @@ public static final class Builder { */ public Builder attachMatchedNodeInfo(Node node, AbstractCompiler compiler) { matchedNodeInfo = - MatchedNodeInfo.create(node, isInClosurizedFile(node, new NodeMetadata(compiler))); + MatchedNodeInfo.create( + node, RefactoringUtils.isInClosurizedFile(node, new NodeMetadata(compiler))); return this; } @@ -917,28 +918,6 @@ public SuggestedFix build() { matchedNodeInfo, replacements.build(), description, alternatives.build()); } - /** Looks for a goog.require(), goog.provide() or goog.module() call in the fix's file. */ - private static boolean isInClosurizedFile(Node node, NodeMetadata metadata) { - Node script = NodeUtil.getEnclosingScript(node); - - if (script == null) { - return false; - } - - Node child = script.getFirstChild(); - while (child != null) { - if (NodeUtil.isExprCall(child)) { - if (Matchers.googRequire().matches(child.getFirstChild(), metadata)) { - return true; - } - // goog.require or goog.module. - } else if (child.isVar() && child.getBooleanProp(Node.IS_NAMESPACE)) { - return true; - } - child = child.getNext(); - } - return false; - } } /** diff --git a/test/com/google/javascript/jscomp/ConformanceWhitelisterTest.java b/test/com/google/javascript/jscomp/ConformanceWhitelisterTest.java index 5b3db6979dc..9e78f0a6913 100644 --- a/test/com/google/javascript/jscomp/ConformanceWhitelisterTest.java +++ b/test/com/google/javascript/jscomp/ConformanceWhitelisterTest.java @@ -20,9 +20,10 @@ import com.google.common.annotations.GwtIncompatible; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableMultimap; import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.Requirement.Type; +import com.google.javascript.rhino.Node; import java.io.IOException; import java.util.List; import junit.framework.TestCase; @@ -44,7 +45,7 @@ public void testConformanceWhitelistAddNew() throws IOException { .addValue("Object.prototype.innerHTML"); assertThat(testConformanceWhitelister(sources.build(), requirement.build())) - .containsExactly("/entry.js"); + .containsExactly("/entry.js", 2); } public void testConformanceWhitelistRemove() throws IOException { @@ -81,12 +82,12 @@ public void testConformanceWhitelistPreserve() throws IOException { .addWhitelist("/entry.js"); assertThat(testConformanceWhitelister(sources.build(), requirement.build())) - .containsExactly("/entry.js"); + .containsExactly("/entry.js", 2); } // TODO(bangert): Evaluate if this is always the best behaviour. // The current behaviour pushes the behaviour of how to cluster the whitelist to the program - // driving ConformanceWhitelister. + // driving ConformanceWhitelister. public void testConformanceWhitelistBreaksDownFolder() throws IOException { ImmutableList.Builder sources = ImmutableList.builder(); @@ -103,10 +104,10 @@ public void testConformanceWhitelistBreaksDownFolder() throws IOException { .addWhitelist("/test/"); assertThat(testConformanceWhitelister(sources.build(), requirement.build())) - .containsExactly("/test/entry.js"); + .containsExactly("/test/entry.js", 2); } - private ImmutableSet testConformanceWhitelister( + private ImmutableMultimap testConformanceWhitelister( ImmutableList sources, Requirement config) throws IOException { CompilerOptions options = new CompilerOptions(); @@ -121,7 +122,16 @@ private ImmutableSet testConformanceWhitelister( Result result = compiler.compile(externs, sources, options); assertTrue(result.success); - return ConformanceWhitelister.getViolatingPaths( - compiler, compiler.getExternsRoot(), compiler.getJsRoot(), config); + ImmutableMultimap.Builder errors = ImmutableMultimap.builder(); + for (Node node : + ConformanceWhitelister.getViolatingNodes( + compiler, compiler.getExternsRoot(), compiler.getJsRoot(), config)) { + errors.put(node.getSourceFileName(), node.getLineno()); + } + assertThat(errors.build().keySet()) + .containsExactlyElementsIn( + ConformanceWhitelister.getViolatingPaths( + compiler, compiler.getExternsRoot(), compiler.getJsRoot(), config)); + return errors.build(); } }