diff --git a/src/com/google/javascript/jscomp/BasicErrorManager.java b/src/com/google/javascript/jscomp/BasicErrorManager.java index e2d86f00487..5795463aea0 100644 --- a/src/com/google/javascript/jscomp/BasicErrorManager.java +++ b/src/com/google/javascript/jscomp/BasicErrorManager.java @@ -17,8 +17,6 @@ package com.google.javascript.jscomp; import com.google.common.collect.ImmutableSet; -import java.util.ArrayList; -import java.util.List; /** * An error manager that generates a sorted report when the {@link #generateReport()} method is @@ -37,13 +35,9 @@ public BasicErrorManager() { @Override public void generateReport() { - List list = new ArrayList<>(); - for (ErrorWithLevel message = messages.poll(); message != null; message = messages.poll()) { + for (ErrorWithLevel message : super.getSortedDiagnostics()) { println(message.level, message.error); - list.add(message); } - // Restore the messages since some tests assert the values after generating the report. - messages.addAll(list); printSummary(); } diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index 69b83ea7d10..4e5174e73de 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -27,6 +27,7 @@ import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; import com.google.debugging.sourcemap.SourceMapConsumerV3; @@ -336,10 +337,10 @@ public void initOptions(CompilerOptions options) { setErrorManager( new LoggerErrorManager(createMessageFormatter(), logger)); } else { - PrintStreamErrorManager printer = - new PrintStreamErrorManager(createMessageFormatter(), this.outStream); - printer.setSummaryDetailLevel(options.summaryDetailLevel); - setErrorManager(printer); + PrintStreamErrorReportGenerator printer = + new PrintStreamErrorReportGenerator( + createMessageFormatter(), this.outStream, options.summaryDetailLevel); + setErrorManager(new SortingErrorManager(ImmutableSet.of(printer))); } } diff --git a/src/com/google/javascript/jscomp/PrintStreamErrorManager.java b/src/com/google/javascript/jscomp/PrintStreamErrorManager.java index 48f41aee658..071feac689f 100644 --- a/src/com/google/javascript/jscomp/PrintStreamErrorManager.java +++ b/src/com/google/javascript/jscomp/PrintStreamErrorManager.java @@ -17,18 +17,19 @@ package com.google.javascript.jscomp; import com.google.javascript.jscomp.parsing.parser.util.format.SimpleFormat; - import java.io.PrintStream; /** - *

An error manager that prints errors and warnings to the print stream - * provided in addition to the functionality of the - * {@link BasicErrorManager}.

+ * An error manager that prints errors and warnings to the print stream provided in addition to the + * functionality of the {@link BasicErrorManager}. * - *

It collaborates with a {@link SourceExcerptProvider} via a - * {@link MessageFormatter} to display error messages with source context.

+ *

It collaborates with a {@link SourceExcerptProvider} via a {@link MessageFormatter} to display + * error messages with source context. * + * @deprecated, Please use the {#SortingErrorManger} with a {#PrintStreamErrorReportGenerator} + * instead. */ +@Deprecated public class PrintStreamErrorManager extends BasicErrorManager { private final MessageFormatter formatter; private final PrintStream stream; diff --git a/src/com/google/javascript/jscomp/PrintStreamErrorReportGenerator.java b/src/com/google/javascript/jscomp/PrintStreamErrorReportGenerator.java new file mode 100644 index 00000000000..e73381daa86 --- /dev/null +++ b/src/com/google/javascript/jscomp/PrintStreamErrorReportGenerator.java @@ -0,0 +1,76 @@ +/* + * Copyright 2007 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.javascript.jscomp.parsing.parser.util.format.SimpleFormat; +import java.io.PrintStream; + +/** + * An error report generator that prints errors and warnings to the print stream provided. + * + *

It collaborates with a {@link SourceExcerptProvider} via a {@link MessageFormatter} to display + * error messages with source context. + */ +public class PrintStreamErrorReportGenerator implements SortingErrorManager.ErrorReportGenerator { + private final MessageFormatter formatter; + private final PrintStream stream; + private final int summaryDetailLevel; + + /** + * Creates an error report generator. + * + * @param formatter the message formatter used to format the messages + * @param stream the stream on which the errors and warnings should be printed. This class does + * not close the stream + */ + public PrintStreamErrorReportGenerator( + MessageFormatter formatter, PrintStream stream, int summaryDetailLevel) { + this.formatter = formatter; + this.stream = stream; + this.summaryDetailLevel = summaryDetailLevel; + } + + @Override + public void generateReport(SortingErrorManager manager) { + for (SortingErrorManager.ErrorWithLevel e : manager.getSortedDiagnostics()) { + println(e.level, e.error); + } + printSummary(manager); + } + + private void println(CheckLevel level, JSError error) { + stream.println(error.format(level, formatter)); + } + + private void printSummary(ErrorManager manager) { + if (summaryDetailLevel >= 3 + || (summaryDetailLevel >= 1 && manager.getErrorCount() + manager.getWarningCount() > 0) + || (summaryDetailLevel >= 2 && manager.getTypedPercent() > 0.0)) { + if (manager.getTypedPercent() > 0.0) { + stream.print( + SimpleFormat.format( + "%d error(s), %d warning(s), %.1f%% typed%n", + manager.getErrorCount(), manager.getWarningCount(), manager.getTypedPercent())); + } else { + stream.print( + SimpleFormat.format( + "%d error(s), %d warning(s)%n", + manager.getErrorCount(), manager.getWarningCount())); + } + } + } +} diff --git a/src/com/google/javascript/jscomp/SortingErrorManager.java b/src/com/google/javascript/jscomp/SortingErrorManager.java index e4cfec9b5f2..fcc3ae91b35 100644 --- a/src/com/google/javascript/jscomp/SortingErrorManager.java +++ b/src/com/google/javascript/jscomp/SortingErrorManager.java @@ -16,15 +16,14 @@ package com.google.javascript.jscomp; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; -import java.util.HashSet; import java.util.List; import java.util.Objects; -import java.util.PriorityQueue; import java.util.Set; +import java.util.TreeSet; /** * A customizable error manager that sorts all errors and warnings reported to it, and has @@ -32,9 +31,7 @@ */ public class SortingErrorManager implements ErrorManager { - final PriorityQueue messages = - new PriorityQueue<>(1, new LeveledJSErrorComparator()); - private final Set alreadyAdded = new HashSet<>(); + private final TreeSet messages = new TreeSet<>(new LeveledJSErrorComparator()); private int originalErrorCount = 0; private int promotedErrorCount = 0; private int warningCount = 0; @@ -50,8 +47,7 @@ public SortingErrorManager(Set errorReportGenerators) { @Override public void report(CheckLevel level, JSError error) { ErrorWithLevel e = new ErrorWithLevel(error, level); - if (alreadyAdded.add(e)) { - messages.add(e); + if (messages.add(e)) { if (level == CheckLevel.ERROR) { if (error.getType().level == CheckLevel.ERROR) { originalErrorCount++; @@ -90,7 +86,9 @@ public JSError[] getWarnings() { } Iterable getSortedDiagnostics() { - return Collections.unmodifiableCollection(messages); + // TODO(b/114762232): It should be possible to remove the copying here and switch to an + // unmodifiable collection once we get rid of usages that add warnings during generateReport. + return ImmutableList.copyOf(messages); } @Override @@ -113,6 +111,8 @@ private JSError[] toArray(CheckLevel level) { return errors.toArray(new JSError[0]); } + // TODO(b/114762232): It should be invalid to report errors during the execution of this method; + // doing so will become impossible once all subclases have migrated to an ErrorReportGenerator. @Override public void generateReport() { for (ErrorReportGenerator generator : this.errorReportGenerators) { diff --git a/test/com/google/javascript/jscomp/SortingErrorManagerTest.java b/test/com/google/javascript/jscomp/SortingErrorManagerTest.java index eb666ee24fd..1d98794234d 100644 --- a/test/com/google/javascript/jscomp/SortingErrorManagerTest.java +++ b/test/com/google/javascript/jscomp/SortingErrorManagerTest.java @@ -123,11 +123,7 @@ protected void printSummary() { } assertThat(printedErrors).hasSize(1); } - // Ensure that more warnings can be added from generating the report. - // One case in which this happened is when the report tries to use the source maps to map back to - // the original source, yet a warning was produced because of a corrupted source map. This test - // ensures that there is no java.util.ConcurrentModificationException while adding a new warning - // when iterating over the existing warnings to print them out. + // This test is testing a "feature" that seems bogus and should likely be forbidden. public void testGenerateReportCausesMoreWarnings() { BasicErrorManager manager = new BasicErrorManager() { @@ -136,6 +132,8 @@ public void testGenerateReportCausesMoreWarnings() { @Override public void println(CheckLevel level, JSError error) { if (error.getType().equals(FOO_TYPE)) { + // TODO(b/114762232) This behavior should not be supported, and will become malformed + // when migrating to a SortingErrorManager with an ErrorReportGenerator. this.report(CheckLevel.ERROR, JSError.make(NULL_SOURCE, -1, -1, JOO_TYPE)); } printed++; @@ -143,7 +141,7 @@ public void println(CheckLevel level, JSError error) { @Override protected void printSummary() { - assertEquals(2, printed); + assertEquals(1, printed); } }; manager.report(CheckLevel.ERROR, JSError.make(NULL_SOURCE, -1, -1, FOO_TYPE));