Make the exit code for dartanalyzer more deterministic.

BUG=
R=brianwilkerson@google.com

Review-Url: https://codereview.chromium.org/2857203002 .

pkg/analyzer_cli/lib/src/analyzer_impl.dart

@@ -65,15 +65,14 @@ class AnalyzerImpl {
       this.librarySource, this.options, this.stats, this.startTime);
 
   /// Returns the maximal [ErrorSeverity] of the recorded errors.
-  ErrorSeverity get maxErrorSeverity {
+  ErrorSeverity computeMaxErrorSeverity() {
     ErrorSeverity status = ErrorSeverity.NONE;
     for (AnalysisErrorInfo errorInfo in errorInfos) {
       for (AnalysisError error in errorInfo.errors) {
         if (_defaultSeverityProcessor(error) == null) {
           continue;
         }
-        ErrorSeverity severity = computeSeverity(error, options);
-        status = status.max(severity);
+        status = status.max(computeSeverity(error, options, analysisOptions));
       }
     }
     return status;
@@ -185,12 +184,8 @@ class AnalyzerImpl {
       _printColdPerf();
     }
 
-    // Compute max severity and set exitCode.
-    ErrorSeverity status = maxErrorSeverity;
-    if (status == ErrorSeverity.WARNING && options.warningsAreFatal) {
-      status = ErrorSeverity.ERROR;
-    }
-    return status;
+    // Compute and return max severity.
+    return computeMaxErrorSeverity();
   }
 
   /// Returns true if we want to report diagnostics for this library.
@@ -242,7 +237,7 @@ class AnalyzerImpl {
     outSink.writeln("total-cold:$totalTime");
   }
 
-  ProcessedSeverity _defaultSeverityProcessor(AnalysisError error) =>
+  ErrorSeverity _defaultSeverityProcessor(AnalysisError error) =>
       determineProcessedSeverity(error, options, analysisOptions);
 
   Future<LibraryElement> _resolveLibrary() async {

pkg/analyzer_cli/lib/src/build_mode.dart

@@ -249,10 +249,10 @@ class BuildMode {
       for (Source source in explicitSources) {
         AnalysisErrorInfo errorInfo = context.getErrors(source);
         for (AnalysisError error in errorInfo.errors) {
-          ProcessedSeverity processedSeverity = determineProcessedSeverity(
+          ErrorSeverity processedSeverity = determineProcessedSeverity(
               error, options, context.analysisOptions);
           if (processedSeverity != null) {
-            maxSeverity = maxSeverity.max(processedSeverity.severity);
+            maxSeverity = maxSeverity.max(processedSeverity);
           }
         }
       }

pkg/analyzer_cli/lib/src/driver.dart

@@ -130,8 +130,8 @@ class Driver implements CommandLineStarter {
     // Do analysis.
     if (options.buildMode) {
       ErrorSeverity severity = _buildModeAnalyze(options);
-      // In case of error propagate exit code.
-      if (severity == ErrorSeverity.ERROR) {
+      // Propagate issues to the exit code.
+      if (_shouldBeFatal(severity, options)) {
         io.exitCode = severity.ordinal;
       }
     } else if (options.shouldBatch) {
@@ -142,8 +142,8 @@ class Driver implements CommandLineStarter {
       });
     } else {
       ErrorSeverity severity = await _analyzeAll(options);
-      // In case of error propagate exit code.
-      if (severity == ErrorSeverity.ERROR) {
+      // Propagate issues to the exit code.
+      if (_shouldBeFatal(severity, options)) {
         io.exitCode = severity.ordinal;
       }
     }
@@ -713,19 +713,12 @@ class Driver implements CommandLineStarter {
   }
 
   /// Analyze a single source.
-  Future<ErrorSeverity> _runAnalyzer(Source source, CommandLineOptions options,
-      ErrorFormatter formatter) async {
+  Future<ErrorSeverity> _runAnalyzer(
+      Source source, CommandLineOptions options, ErrorFormatter formatter) {
     int startTime = currentTimeMillis;
     AnalyzerImpl analyzer = new AnalyzerImpl(_context.analysisOptions, _context,
         analysisDriver, source, options, stats, startTime);
-    ErrorSeverity errorSeverity = await analyzer.analyze(formatter);
-    if (errorSeverity == ErrorSeverity.ERROR) {
-      io.exitCode = errorSeverity.ordinal;
-    }
-    if (options.warningsAreFatal && errorSeverity == ErrorSeverity.WARNING) {
-      io.exitCode = errorSeverity.ordinal;
-    }
-    return errorSeverity;
+    return analyzer.analyze(formatter);
   }
 
   void _setupSdk(CommandLineOptions options, bool useSummaries,
@@ -851,6 +844,19 @@ class Driver implements CommandLineStarter {
   /// Convert [sourcePath] into an absolute path.
   static String _normalizeSourcePath(String sourcePath) =>
       path.normalize(new io.File(sourcePath).absolute.path);
+
+  bool _shouldBeFatal(ErrorSeverity severity, CommandLineOptions options) {
+    if (severity == ErrorSeverity.ERROR) {
+      return true;
+    } else if (severity == ErrorSeverity.WARNING &&
+        (options.warningsAreFatal || options.hintsAreFatal)) {
+      return true;
+    } else if (severity == ErrorSeverity.INFO && options.hintsAreFatal) {
+      return true;
+    } else {
+      return false;
+    }
+  }
 }
 
 class _DriverError implements Exception {

pkg/analyzer_cli/lib/src/error_formatter.dart

@@ -12,14 +12,14 @@ import 'package:analyzer_cli/src/options.dart';
 import 'package:path/path.dart' as path;
 
 /// Returns the given error's severity.
-ProcessedSeverity _severityIdentity(AnalysisError error) =>
-    new ProcessedSeverity(error.errorCode.errorSeverity);
+ErrorSeverity _severityIdentity(AnalysisError error) =>
+    error.errorCode.errorSeverity;
 
 String _pluralize(String word, int count) => count == 1 ? word : word + "s";
 
 /// Returns desired severity for the given [error] (or `null` if it's to be
 /// suppressed).
-typedef ProcessedSeverity SeverityProcessor(AnalysisError error);
+typedef ErrorSeverity SeverityProcessor(AnalysisError error);
 
 /// Analysis statistics counter.
 class AnalysisStats {
@@ -107,7 +107,7 @@ abstract class ErrorFormatter {
   /// Compute the severity for this [error] or `null` if this error should be
   /// filtered.
   ErrorSeverity _computeSeverity(AnalysisError error) =>
-      _severityProcessor(error)?.severity;
+      _severityProcessor(error);
 
   void formatErrors(List<AnalysisErrorInfo> errorInfos) {
     stats.unfilteredCount += errorInfos.length;
@@ -153,24 +153,12 @@ class MachineErrorFormatter extends ErrorFormatter {
     LineInfo_Location location = errorToLine[error].getLocation(error.offset);
     int length = error.length;
 
-    ProcessedSeverity processedSeverity = _severityProcessor(error);
-    ErrorSeverity severity = processedSeverity.severity;
-
-    if (!processedSeverity.overridden) {
-      if (severity == ErrorSeverity.WARNING && options.warningsAreFatal) {
-        severity = ErrorSeverity.ERROR;
-      }
-    }
+    ErrorSeverity severity = _severityProcessor(error);
 
     if (severity == ErrorSeverity.ERROR) {
       stats.errorCount++;
     } else if (severity == ErrorSeverity.WARNING) {
-      // Only treat a warning as an error if it's not been set by a processor.
-      if (!processedSeverity.overridden && options.warningsAreFatal) {
-        stats.errorCount++;
-      } else {
-        stats.warnCount++;
-      }
+      stats.warnCount++;
     } else if (error.errorCode.type == ErrorType.HINT) {
       stats.hintCount++;
     } else if (error.errorCode.type == ErrorType.LINT) {
@@ -233,8 +221,7 @@ class HumanErrorFormatter extends ErrorFormatter {
     Source source = error.source;
     LineInfo_Location location = errorToLine[error].getLocation(error.offset);
 
-    ProcessedSeverity processedSeverity = _severityProcessor(error);
-    ErrorSeverity severity = processedSeverity.severity;
+    ErrorSeverity severity = _severityProcessor(error);
 
     // Get display name; translate INFOs into LINTS and HINTS.
     String errorType = severity.displayName;
@@ -382,13 +369,6 @@ class CLIError implements Comparable<CLIError> {
   }
 }
 
-/// A severity with awareness of whether it was overridden by a processor.
-class ProcessedSeverity {
-  final ErrorSeverity severity;
-  final bool overridden;
-  ProcessedSeverity(this.severity, [this.overridden = false]);
-}
-
 /// Given an absolute path, return a relative path if the file is contained in
 /// the current directory; return the original path otherwise.
 String _relative(String file) {

pkg/analyzer_cli/lib/src/error_severity.dart

@@ -6,51 +6,39 @@ import 'package:analyzer/error/error.dart';
 import 'package:analyzer/source/error_processor.dart';
 import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/generated/engine.dart' hide AnalysisResult;
-import 'package:analyzer_cli/src/error_formatter.dart';
 import 'package:analyzer_cli/src/options.dart';
 
 /// Check various configuration options to get a desired severity for this
 /// [error] (or `null` if it's to be suppressed).
-ProcessedSeverity determineProcessedSeverity(AnalysisError error,
+ErrorSeverity determineProcessedSeverity(AnalysisError error,
     CommandLineOptions commandLineOptions, AnalysisOptions analysisOptions) {
-  ErrorSeverity severity = computeSeverity(error, commandLineOptions,
-      analysisOptions: analysisOptions);
-  bool isOverridden = false;
-
-  // Skip TODOs categorically (unless escalated to ERROR or HINT.)
-  // https://github.com/dart-lang/sdk/issues/26215
+  ErrorSeverity severity =
+      computeSeverity(error, commandLineOptions, analysisOptions);
+  // Skip TODOs categorically unless escalated to ERROR or HINT (#26215).
   if (error.errorCode.type == ErrorType.TODO &&
       severity == ErrorSeverity.INFO) {
     return null;
   }
 
-  // First check for a filter.
-  if (severity == null) {
-    // Null severity means the error has been explicitly ignored.
-    return null;
-  } else {
-    isOverridden = true;
-  }
-
+  // TODO(devoncarew): We should not filter hints here.
   // If not overridden, some "natural" severities get globally filtered.
-  if (!isOverridden) {
-    // Check for global hint filtering.
-    if (severity == ErrorSeverity.INFO && commandLineOptions.disableHints) {
-      return null;
-    }
+  // Check for global hint filtering.
+  if (severity == ErrorSeverity.INFO && commandLineOptions.disableHints) {
+    return null;
   }
 
-  return new ProcessedSeverity(severity, isOverridden);
+  return severity;
 }
 
 /// Compute the severity of the error; however:
 /// - if [options.enableTypeChecks] is false, then de-escalate checked-mode
 ///   compile time errors to a severity of [ErrorSeverity.INFO].
-/// - if [options.hintsAreFatal] is true, escalate hints to errors.
 /// - if [options.lintsAreFatal] is true, escalate lints to errors.
 ErrorSeverity computeSeverity(
-    AnalysisError error, CommandLineOptions commandLineOptions,
-    {AnalysisOptions analysisOptions}) {
+  AnalysisError error,
+  CommandLineOptions commandLineOptions,
+  AnalysisOptions analysisOptions,
+) {
   if (analysisOptions != null) {
     ErrorProcessor processor =
         ErrorProcessor.getProcessor(analysisOptions, error);
@@ -63,10 +51,9 @@ ErrorSeverity computeSeverity(
   if (!commandLineOptions.enableTypeChecks &&
       error.errorCode.type == ErrorType.CHECKED_MODE_COMPILE_TIME_ERROR) {
     return ErrorSeverity.INFO;
-  } else if (commandLineOptions.hintsAreFatal && error.errorCode is HintCode) {
-    return ErrorSeverity.ERROR;
   } else if (commandLineOptions.lintsAreFatal && error.errorCode is LintCode) {
     return ErrorSeverity.ERROR;
   }
+
   return error.errorCode.errorSeverity;
 }

pkg/analyzer_cli/lib/src/options.dart

@@ -144,6 +144,8 @@ class CommandLineOptions {
   /// Whether implicit dynamic is enabled (mainly for strong mode users)
   final bool implicitDynamic;
 
+  // TODO(devoncarew): Do we need this flag? Shouldn't we go by the severity of
+  // the lint?
   /// Whether to treat lints as fatal
   final bool lintsAreFatal;
 

pkg/analyzer_cli/test/all.dart

@@ -8,6 +8,7 @@ import 'driver_test.dart' as driver_test;
 import 'embedder_test.dart' as embedder_test;
 import 'error_test.dart' as error_test;
 import 'errors_reported_once_test.dart' as errors_reported_once_test;
+import 'errors_upgrade_fails_cli_test.dart' as errors_upgrade_fails_cli_test;
 import 'options_test.dart' as options_test;
 import 'package_prefix_test.dart' as package_prefix_test;
 import 'perf_report_test.dart' as perf_report_test;
@@ -23,6 +24,7 @@ main() {
   embedder_test.main();
   error_test.main();
   errors_reported_once_test.main();
+  errors_upgrade_fails_cli_test.main();
   options_test.main();
   package_prefix_test.main();
   perf_report_test.main();

pkg/analyzer_cli/test/data/error_upgrade_fails_cli/analysis_options.yaml

@@ -0,0 +1,3 @@
+analyzer:
+  errors:
+    missing_return: error

pkg/analyzer_cli/test/data/error_upgrade_fails_cli/foo.dart

@@ -0,0 +1 @@
+int foo() {}

pkg/analyzer_cli/test/driver_test.dart

@@ -66,7 +66,7 @@ main() {
     group('exit codes', () {
       test('fatal hints', () async {
         await drive('data/file_with_hint.dart', args: ['--fatal-hints']);
-        expect(exitCode, 3);
+        expect(exitCode, 1);
       });
 
       test('not fatal hints', () async {
@@ -86,7 +86,7 @@ main() {
 
       test('fatal warnings', () async {
         await drive('data/file_with_warning.dart', args: ['--fatal-warnings']);
-        expect(exitCode, 3);
+        expect(exitCode, 2);
       });
 
       test('not parse enableAssertInitializer', () async {

pkg/analyzer_cli/test/errors_upgrade_fails_cli_test.dart

@@ -0,0 +1,50 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'dart:io';
+
+import 'package:analyzer_cli/src/driver.dart';
+import 'package:analyzer_cli/src/options.dart';
+import 'package:path/path.dart' as path;
+import 'package:test/test.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import 'utils.dart';
+
+main() {
+  defineReflectiveTests(ErrorUpgradeFailsCli);
+}
+
+@reflectiveTest
+class ErrorUpgradeFailsCli {
+  StringSink savedOutSink, savedErrorSink;
+  int savedExitCode;
+  ExitHandler savedExitHandler;
+
+  void setUp() {
+    savedOutSink = outSink;
+    savedErrorSink = errorSink;
+    savedExitHandler = exitHandler;
+    savedExitCode = exitCode;
+    exitHandler = (code) => exitCode = code;
+    outSink = new StringBuffer();
+    errorSink = new StringBuffer();
+  }
+
+  void tearDown() {
+    outSink = savedOutSink;
+    errorSink = savedErrorSink;
+    exitCode = savedExitCode;
+    exitHandler = savedExitHandler;
+  }
+
+  test_once() async {
+    String testDir =
+        path.join(testDirectory, 'data', 'error_upgrade_fails_cli');
+    Driver driver = new Driver();
+    await driver.start([path.join(testDir, 'foo.dart')]);
+
+    expect(exitCode, 3);
+  }
+}