Rollup merge of rust-lang#62055 - matthewjasper:fix-error-counting, r=pnkfelix

Fix error counting

Count duplicate errors for `track_errors` and other error counting checks.
Add FIXMEs to make it clear that we should be moving away from this kind of logic.

Closes rust-lang#61663

Author: Centril

src/librustc/infer/mod.rs

@@ -171,6 +171,7 @@ pub struct InferCtxt<'a, 'tcx> {
     /// Track how many errors were reported when this infcx is created.
     /// If the number of errors increases, that's also a sign (line
     /// `tained_by_errors`) to avoid reporting certain kinds of errors.
+    // FIXME(matthewjasper) Merge into `tainted_by_errors_flag`
     err_count_on_creation: usize,
 
     /// This flag is true while there is an active snapshot.

src/librustc/session/mod.rs

@@ -320,8 +320,13 @@ impl Session {
         self.diagnostic().abort_if_errors();
     }
     pub fn compile_status(&self) -> Result<(), ErrorReported> {
-        compile_result_from_err_count(self.err_count())
+        if self.has_errors() {
+            Err(ErrorReported)
+        } else {
+            Ok(())
+        }
     }
+    // FIXME(matthewjasper) Remove this method, it should never be needed.
     pub fn track_errors<F, T>(&self, f: F) -> Result<T, ErrorReported>
     where
         F: FnOnce() -> T,
@@ -1388,11 +1393,3 @@ pub fn early_warn(output: config::ErrorOutputType, msg: &str) {
 }
 
 pub type CompileResult = Result<(), ErrorReported>;
-
-pub fn compile_result_from_err_count(err_count: usize) -> CompileResult {
-    if err_count == 0 {
-        Ok(())
-    } else {
-        Err(ErrorReported)
-    }
-}

src/librustc_errors/lib.rs

@@ -307,7 +307,12 @@ pub use diagnostic_builder::DiagnosticBuilder;
 pub struct Handler {
     pub flags: HandlerFlags,
 
+    /// The number of errors that have been emitted, including duplicates.
+    ///
+    /// This is not necessarily the count that's reported to the user once
+    /// compilation ends.
     err_count: AtomicUsize,
+    deduplicated_err_count: AtomicUsize,
     emitter: Lock<Box<dyn Emitter + sync::Send>>,
     continue_after_error: AtomicBool,
     delayed_span_bugs: Lock<Vec<Diagnostic>>,
@@ -352,7 +357,7 @@ pub struct HandlerFlags {
 
 impl Drop for Handler {
     fn drop(&mut self) {
-        if self.err_count() == 0 {
+        if !self.has_errors() {
             let mut bugs = self.delayed_span_bugs.borrow_mut();
             let has_bugs = !bugs.is_empty();
             for bug in bugs.drain(..) {
@@ -407,6 +412,7 @@ impl Handler {
         Handler {
             flags,
             err_count: AtomicUsize::new(0),
+            deduplicated_err_count: AtomicUsize::new(0),
             emitter: Lock::new(e),
             continue_after_error: AtomicBool::new(true),
             delayed_span_bugs: Lock::new(Vec::new()),
@@ -428,6 +434,7 @@ impl Handler {
     pub fn reset_err_count(&self) {
         // actually frees the underlying memory (which `clear` would not do)
         *self.emitted_diagnostics.borrow_mut() = Default::default();
+        self.deduplicated_err_count.store(0, SeqCst);
         self.err_count.store(0, SeqCst);
     }
 
@@ -660,10 +667,10 @@ impl Handler {
     }
 
     pub fn print_error_count(&self, registry: &Registry) {
-        let s = match self.err_count() {
+        let s = match self.deduplicated_err_count.load(SeqCst) {
             0 => return,
             1 => "aborting due to previous error".to_string(),
-            _ => format!("aborting due to {} previous errors", self.err_count())
+            count => format!("aborting due to {} previous errors", count)
         };
         if self.treat_err_as_bug() {
             return;
@@ -705,10 +712,9 @@ impl Handler {
     }
 
     pub fn abort_if_errors(&self) {
-        if self.err_count() == 0 {
-            return;
+        if self.has_errors() {
+            FatalError.raise();
         }
-        FatalError.raise();
     }
     pub fn emit(&self, msp: &MultiSpan, msg: &str, lvl: Level) {
         if lvl == Warning && !self.flags.can_emit_warnings {
@@ -770,9 +776,12 @@ impl Handler {
         if self.emitted_diagnostics.borrow_mut().insert(diagnostic_hash) {
             self.emitter.borrow_mut().emit_diagnostic(db);
             if db.is_error() {
-                self.bump_err_count();
+                self.deduplicated_err_count.fetch_add(1, SeqCst);
             }
         }
+        if db.is_error() {
+            self.bump_err_count();
+        }
     }
 
     pub fn emit_artifact_notification(&self, path: &Path, artifact_type: &str) {

src/librustc_interface/passes.rs

@@ -959,7 +959,7 @@ fn analysis<'tcx>(tcx: TyCtxt<'tcx>, cnum: CrateNum) -> Result<()> {
     // lot of annoying errors in the compile-fail tests (basically,
     // lint warnings and so on -- kindck used to do this abort, but
     // kindck is gone now). -nmatsakis
-    if sess.err_count() > 0 {
+    if sess.has_errors() {
         return Err(ErrorReported);
     }
 

src/librustc_mir/const_eval.rs

@@ -15,7 +15,6 @@ use rustc::ty::{self, TyCtxt, query::TyCtxtAt};
 use rustc::ty::layout::{self, LayoutOf, VariantIdx};
 use rustc::ty::subst::Subst;
 use rustc::traits::Reveal;
-use rustc::util::common::ErrorReported;
 use rustc_data_structures::fx::FxHashMap;
 
 use syntax::source_map::{Span, DUMMY_SP};
@@ -655,19 +654,12 @@ pub fn const_eval_raw_provider<'tcx>(
         if tcx.is_static(def_id) {
             // Ensure that if the above error was either `TooGeneric` or `Reported`
             // an error must be reported.
-            let reported_err = tcx.sess.track_errors(|| {
-                err.report_as_error(ecx.tcx,
-                                    "could not evaluate static initializer")
-            });
-            match reported_err {
-                Ok(v) => {
-                    tcx.sess.delay_span_bug(err.span,
-                                        &format!("static eval failure did not emit an error: {:#?}",
-                                        v));
-                    v
-                },
-                Err(ErrorReported) => ErrorHandled::Reported,
-            }
+            let v = err.report_as_error(ecx.tcx, "could not evaluate static initializer");
+            tcx.sess.delay_span_bug(
+                err.span,
+                &format!("static eval failure did not emit an error: {:#?}", v)
+            );
+            v
         } else if def_id.is_local() {
             // constant defined in this crate, we can figure out a lint level!
             match tcx.def_kind(def_id) {

src/librustc_save_analysis/span_utils.rs

@@ -2,25 +2,19 @@ use rustc::session::Session;
 
 use crate::generated_code;
 
-use std::cell::Cell;
-
 use syntax::parse::lexer::{self, StringReader};
 use syntax::parse::token::{self, TokenKind};
 use syntax_pos::*;
 
 #[derive(Clone)]
 pub struct SpanUtils<'a> {
     pub sess: &'a Session,
-    // FIXME given that we clone SpanUtils all over the place, this err_count is
-    // probably useless and any logic relying on it is bogus.
-    pub err_count: Cell<isize>,
 }
 
 impl<'a> SpanUtils<'a> {
     pub fn new(sess: &'a Session) -> SpanUtils<'a> {
         SpanUtils {
             sess,
-            err_count: Cell::new(0),
         }
     }
 

src/librustc_typeck/check/expr.rs

@@ -565,7 +565,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 // else an error would have been flagged by the
                 // `loops` pass for using break with an expression
                 // where you are not supposed to.
-                assert!(expr_opt.is_none() || self.tcx.sess.err_count() > 0);
+                assert!(expr_opt.is_none() || self.tcx.sess.has_errors());
             }
 
             ctxt.may_break = true;
@@ -577,10 +577,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             // this can only happen if the `break` was not
             // inside a loop at all, which is caught by the
             // loop-checking pass.
-            if self.tcx.sess.err_count() == 0 {
-                self.tcx.sess.delay_span_bug(expr.span,
-                    "break was outside loop, but no error was emitted");
-            }
+            self.tcx.sess.delay_span_bug(expr.span,
+                "break was outside loop, but no error was emitted");
 
             // We still need to assign a type to the inner expression to
             // prevent the ICE in #43162.

src/librustc_typeck/check/mod.rs

@@ -527,6 +527,8 @@ pub struct FnCtxt<'a, 'tcx> {
     /// checking this function. On exit, if we find that *more* errors
     /// have been reported, we will skip regionck and other work that
     /// expects the types within the function to be consistent.
+    // FIXME(matthewjasper) This should not exist, and it's not correct
+    // if type checking is run in parallel.
     err_count_on_creation: usize,
 
     ret_coercion: Option<RefCell<DynamicCoerceMany<'tcx>>>,
@@ -696,11 +698,9 @@ impl ItemLikeVisitor<'tcx> for CheckItemTypesVisitor<'tcx> {
     fn visit_impl_item(&mut self, _: &'tcx hir::ImplItem) { }
 }
 
-pub fn check_wf_new<'tcx>(tcx: TyCtxt<'tcx>) -> Result<(), ErrorReported> {
-    tcx.sess.track_errors(|| {
-        let mut visit = wfcheck::CheckTypeWellFormedVisitor::new(tcx);
-        tcx.hir().krate().par_visit_all_item_likes(&mut visit);
-    })
+pub fn check_wf_new<'tcx>(tcx: TyCtxt<'tcx>) {
+    let mut visit = wfcheck::CheckTypeWellFormedVisitor::new(tcx);
+    tcx.hir().krate().par_visit_all_item_likes(&mut visit);
 }
 
 fn check_mod_item_types<'tcx>(tcx: TyCtxt<'tcx>, module_def_id: DefId) {
@@ -2128,8 +2128,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         &self.tcx.sess
     }
 
-    pub fn err_count_since_creation(&self) -> usize {
-        self.tcx.sess.err_count() - self.err_count_on_creation
+    pub fn errors_reported_since_creation(&self) -> bool {
+        self.tcx.sess.err_count() > self.err_count_on_creation
     }
 
     /// Produces warning on the given node, if the current point in the
@@ -4375,7 +4375,7 @@ pub fn check_bounds_are_used<'tcx>(tcx: TyCtxt<'tcx>, generics: &ty::Generics, t
         } else if let ty::Error = leaf_ty.sty {
             // If there is already another error, do not emit
             // an error for not using a type Parameter.
-            assert!(tcx.sess.err_count() > 0);
+            assert!(tcx.sess.has_errors());
             return;
         }
     }

src/librustc_typeck/check/regionck.rs

@@ -123,7 +123,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         // standalone expr (e.g., the `E` in a type like `[u32; E]`).
         rcx.outlives_environment.save_implied_bounds(id);
 
-        if self.err_count_since_creation() == 0 {
+        if !self.errors_reported_since_creation() {
             // regionck assumes typeck succeeded
             rcx.visit_body(body);
             rcx.visit_region_obligations(id);
@@ -173,7 +173,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             self.param_env,
         );
 
-        if self.err_count_since_creation() == 0 {
+        if !self.errors_reported_since_creation() {
             // regionck assumes typeck succeeded
             rcx.visit_fn_body(fn_id, body, self.tcx.hir().span(fn_id));
         }

src/librustc_typeck/lib.rs

@@ -320,6 +320,7 @@ pub fn check_crate<'tcx>(tcx: TyCtxt<'tcx>) -> Result<(), ErrorReported> {
 
     // this ensures that later parts of type checking can assume that items
     // have valid types and not error
+    // FIXME(matthewjasper) We shouldn't need to do this.
     tcx.sess.track_errors(|| {
         time(tcx.sess, "type collecting", || {
             for &module in tcx.hir().krate().modules.keys() {
@@ -352,7 +353,9 @@ pub fn check_crate<'tcx>(tcx: TyCtxt<'tcx>) -> Result<(), ErrorReported> {
         })?;
     }
 
-    time(tcx.sess, "wf checking", || check::check_wf_new(tcx))?;
+    tcx.sess.track_errors(|| {
+        time(tcx.sess, "wf checking", || check::check_wf_new(tcx));
+    })?;
 
     time(tcx.sess, "item-types checking", || {
         for &module in tcx.hir().krate().modules.keys() {