From e5fa6ec845e458984e361e601d603fa050e992e4 Mon Sep 17 00:00:00 2001 From: David Wood Date: Tue, 20 Feb 2024 10:25:06 +0000 Subject: [PATCH 01/21] triagebot: add queue notifications Signed-off-by: David Wood --- triagebot.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/triagebot.toml b/triagebot.toml index 1a30399e46c03..6634909d3fdcf 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -423,6 +423,10 @@ changelog-branch = "master" [shortcut] +[mentions."triagebot.toml"] +message = "`triagebot.toml` has been modified, there may have been changes to the review queue." +cc = ["@davidtwco", "@wesleywiser"] + [mentions."compiler/rustc_codegen_cranelift"] cc = ["@bjorn3"] From e13410225be8aad07da0a7853504e95185b0bc10 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Tue, 20 Feb 2024 13:31:57 +1100 Subject: [PATCH 02/21] Improve internal docs for the `HeaderLine` callback struct --- src/tools/compiletest/src/header.rs | 34 ++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 7f765fd86c87f..0e013d64b4d4d 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -646,6 +646,8 @@ impl TestProps { } /// Extract a `(Option, directive)` directive from a line if comment is present. +/// +/// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( comment: &str, ln: &'line str, @@ -790,16 +792,32 @@ const DIAGNOSTICS_DIRECTIVE_NAMES: &[&str] = &[ "unset-rustc-env", ]; -/// Arguments passed to the callback in [`iter_header`]. +/// The broken-down contents of a line containing a test header directive, +/// which [`iter_header`] passes to its callback function. +/// +/// For example: +/// +/// ```text +/// //@ compile-flags: -O +/// ^^^^^^^^^^^^^^^^^ directive +/// ^^^^^^^^^^^^^^^^^^^^^ original_line +/// +/// //@ [foo] compile-flags: -O +/// ^^^ header_revision +/// ^^^^^^^^^^^^^^^^^ directive +/// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ original_line +/// ``` struct HeaderLine<'ln> { - /// Contents of the square brackets preceding this header, if present. - header_revision: Option<&'ln str>, + line_number: usize, /// Raw line from the test file, including comment prefix and any revision. original_line: &'ln str, - /// Remainder of the directive line, after the initial comment prefix - /// (`//` or `//@` or `#`) and revision (if any) have been stripped. + /// Some header directives start with a revision name in square brackets + /// (e.g. `[foo]`), and only apply to that revision of the test. + /// If present, this field contains the revision name (e.g. `foo`). + header_revision: Option<&'ln str>, + /// The main part of the header directive, after removing the comment prefix + /// and the optional revision specifier. directive: &'ln str, - line_number: usize, } fn iter_header( @@ -831,7 +849,7 @@ fn iter_header( ]; // Process the extra implied directives, with a dummy line number of 0. for directive in extra_directives { - it(HeaderLine { header_revision: None, original_line: "", directive, line_number: 0 }); + it(HeaderLine { line_number: 0, original_line: "", header_revision: None, directive }); } } @@ -865,7 +883,7 @@ fn iter_header( // First try to accept `ui_test` style comments } else if let Some((header_revision, directive)) = line_directive(comment, ln) { - it(HeaderLine { header_revision, original_line, directive, line_number }); + it(HeaderLine { line_number, original_line, header_revision, directive }); } else if mode == Mode::Ui && suite == "ui" && !REVISION_MAGIC_COMMENT_RE.is_match(ln) { let Some((_, rest)) = line_directive("//", ln) else { continue; From 99fb653d1d0f79961506413de549bebd7b798ba8 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 17 Feb 2024 23:58:29 +1100 Subject: [PATCH 03/21] Consistently refer to a test's `revision` instead of `cfg` Compiletest code sometimes refers to a test's revision as its `cfg`. This results in two different names for the same thing, one of which is ambiguous with other kinds of configuration (such as compiletest's own config). This patch replaces those occurrences of `cfg` with `revision`. --- src/tools/compiletest/src/errors.rs | 27 +++++++++++++---------- src/tools/compiletest/src/header.rs | 26 +++++++++++----------- src/tools/compiletest/src/header/tests.rs | 13 ++++++++--- src/tools/compiletest/src/lib.rs | 19 +++++----------- 4 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/tools/compiletest/src/errors.rs b/src/tools/compiletest/src/errors.rs index e0ec76aa027b7..140c3aa0a64ec 100644 --- a/src/tools/compiletest/src/errors.rs +++ b/src/tools/compiletest/src/errors.rs @@ -72,9 +72,9 @@ enum WhichLine { /// and also //~^ ERROR message one for the preceding line, and /// //~| ERROR message two for that same line. /// -/// If cfg is not None (i.e., in an incremental test), then we look -/// for `//[X]~` instead, where `X` is the current `cfg`. -pub fn load_errors(testfile: &Path, cfg: Option<&str>) -> Vec { +/// If revision is not None, then we look +/// for `//[X]~` instead, where `X` is the current revision. +pub fn load_errors(testfile: &Path, revision: Option<&str>) -> Vec { let rdr = BufReader::new(File::open(testfile).unwrap()); // `last_nonfollow_error` tracks the most recently seen @@ -90,7 +90,7 @@ pub fn load_errors(testfile: &Path, cfg: Option<&str>) -> Vec { rdr.lines() .enumerate() .filter_map(|(line_num, line)| { - parse_expected(last_nonfollow_error, line_num + 1, &line.unwrap(), cfg).map( + parse_expected(last_nonfollow_error, line_num + 1, &line.unwrap(), revision).map( |(which, error)| { match which { FollowPrevious(_) => {} @@ -108,24 +108,27 @@ fn parse_expected( last_nonfollow_error: Option, line_num: usize, line: &str, - cfg: Option<&str>, + test_revision: Option<&str>, ) -> Option<(WhichLine, Error)> { // Matches comments like: // //~ // //~| // //~^ // //~^^^^^ - // //[cfg1]~ - // //[cfg1,cfg2]~^^ + // //[rev1]~ + // //[rev1,rev2]~^^ static RE: Lazy = - Lazy::new(|| Regex::new(r"//(?:\[(?P[\w,]+)])?~(?P\||\^*)").unwrap()); + Lazy::new(|| Regex::new(r"//(?:\[(?P[\w,]+)])?~(?P\||\^*)").unwrap()); let captures = RE.captures(line)?; - match (cfg, captures.name("cfgs")) { - // Only error messages that contain our `cfg` between the square brackets apply to us. - (Some(cfg), Some(filter)) if !filter.as_str().split(',').any(|s| s == cfg) => return None, - (Some(_), Some(_)) => {} + match (test_revision, captures.name("revs")) { + // Only error messages that contain our revision between the square brackets apply to us. + (Some(test_revision), Some(revision_filters)) => { + if !revision_filters.as_str().split(',').any(|r| r == test_revision) { + return None; + } + } (None, Some(_)) => panic!("Only tests with revisions should use `//[X]~`"), diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 0e013d64b4d4d..50e8cfd60a9fb 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -289,20 +289,20 @@ impl TestProps { } } - pub fn from_aux_file(&self, testfile: &Path, cfg: Option<&str>, config: &Config) -> Self { + pub fn from_aux_file(&self, testfile: &Path, revision: Option<&str>, config: &Config) -> Self { let mut props = TestProps::new(); // copy over select properties to the aux build: props.incremental_dir = self.incremental_dir.clone(); props.ignore_pass = true; - props.load_from(testfile, cfg, config); + props.load_from(testfile, revision, config); props } - pub fn from_file(testfile: &Path, cfg: Option<&str>, config: &Config) -> Self { + pub fn from_file(testfile: &Path, revision: Option<&str>, config: &Config) -> Self { let mut props = TestProps::new(); - props.load_from(testfile, cfg, config); + props.load_from(testfile, revision, config); match (props.pass_mode, props.fail_mode) { (None, None) if config.mode == Mode::Ui => props.fail_mode = Some(FailMode::Check), @@ -315,9 +315,9 @@ impl TestProps { /// Loads properties from `testfile` into `props`. If a property is /// tied to a particular revision `foo` (indicated by writing - /// `//[foo]`), then the property is ignored unless `cfg` is + /// `//@[foo]`), then the property is ignored unless `test_revision` is /// `Some("foo")`. - fn load_from(&mut self, testfile: &Path, cfg: Option<&str>, config: &Config) { + fn load_from(&mut self, testfile: &Path, test_revision: Option<&str>, config: &Config) { let mut has_edition = false; if !testfile.is_dir() { let file = File::open(testfile).unwrap(); @@ -331,7 +331,7 @@ impl TestProps { testfile, file, &mut |HeaderLine { header_revision, directive: ln, .. }| { - if header_revision.is_some() && header_revision != cfg { + if header_revision.is_some() && header_revision != test_revision { return; } @@ -455,7 +455,7 @@ impl TestProps { &mut self.check_test_line_numbers_match, ); - self.update_pass_mode(ln, cfg, config); + self.update_pass_mode(ln, test_revision, config); self.update_fail_mode(ln, config); config.set_name_directive(ln, IGNORE_PASS, &mut self.ignore_pass); @@ -645,7 +645,7 @@ impl TestProps { } } -/// Extract a `(Option, directive)` directive from a line if comment is present. +/// Extract an `(Option, directive)` directive from a line if comment is present. /// /// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( @@ -664,8 +664,8 @@ pub fn line_directive<'line>( ); }; - let lncfg = &ln[1..close_brace]; - Some((Some(lncfg), ln[(close_brace + 1)..].trim_start())) + let line_revision = &ln[1..close_brace]; + Some((Some(line_revision), ln[(close_brace + 1)..].trim_start())) } else { Some((None, ln)) } @@ -1176,7 +1176,7 @@ pub fn make_test_description( name: test::TestName, path: &Path, src: R, - cfg: Option<&str>, + test_revision: Option<&str>, poisoned: &mut bool, ) -> test::TestDesc { let mut ignore = false; @@ -1192,7 +1192,7 @@ pub fn make_test_description( path, src, &mut |HeaderLine { header_revision, original_line, directive: ln, line_number }| { - if header_revision.is_some() && header_revision != cfg { + if header_revision.is_some() && header_revision != test_revision { return; } diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs index 274006ae8c16c..f76fb406cea28 100644 --- a/src/tools/compiletest/src/header/tests.rs +++ b/src/tools/compiletest/src/header/tests.rs @@ -10,12 +10,19 @@ fn make_test_description( name: test::TestName, path: &Path, src: R, - cfg: Option<&str>, + revision: Option<&str>, ) -> test::TestDesc { let cache = HeadersCache::load(config); let mut poisoned = false; - let test = - crate::header::make_test_description(config, &cache, name, path, src, cfg, &mut poisoned); + let test = crate::header::make_test_description( + config, + &cache, + name, + path, + src, + revision, + &mut poisoned, + ); if poisoned { panic!("poisoned!"); } diff --git a/src/tools/compiletest/src/lib.rs b/src/tools/compiletest/src/lib.rs index 667358b1a6e31..57a82eb37ede2 100644 --- a/src/tools/compiletest/src/lib.rs +++ b/src/tools/compiletest/src/lib.rs @@ -745,7 +745,7 @@ fn make_test( let revisions = if early_props.revisions.is_empty() || config.mode == Mode::Incremental { vec![None] } else { - early_props.revisions.iter().map(Some).collect() + early_props.revisions.iter().map(|r| Some(r.as_str())).collect() }; revisions @@ -753,20 +753,13 @@ fn make_test( .map(|revision| { let src_file = std::fs::File::open(&test_path).expect("open test file to parse ignores"); - let cfg = revision.map(|v| &**v); let test_name = crate::make_test_name(&config, testpaths, revision); let mut desc = make_test_description( - &config, cache, test_name, &test_path, src_file, cfg, poisoned, + &config, cache, test_name, &test_path, src_file, revision, poisoned, ); // Ignore tests that already run and are up to date with respect to inputs. if !config.force_rerun { - desc.ignore |= is_up_to_date( - &config, - testpaths, - &early_props, - revision.map(|s| s.as_str()), - inputs, - ); + desc.ignore |= is_up_to_date(&config, testpaths, &early_props, revision, inputs); } test::TestDescAndFn { desc, @@ -879,7 +872,7 @@ impl Stamp { fn make_test_name( config: &Config, testpaths: &TestPaths, - revision: Option<&String>, + revision: Option<&str>, ) -> test::TestName { // Print the name of the file, relative to the repository root. // `src_base` looks like `/path/to/rust/tests/ui` @@ -907,11 +900,11 @@ fn make_test_name( fn make_test_closure( config: Arc, testpaths: &TestPaths, - revision: Option<&String>, + revision: Option<&str>, ) -> test::TestFn { let config = config.clone(); let testpaths = testpaths.clone(); - let revision = revision.cloned(); + let revision = revision.map(str::to_owned); test::DynTestFn(Box::new(move || { runtest::run(config, &testpaths, revision.as_deref()); Ok(()) From 544d09132bef3d483a0e10b4bb25e25079107e37 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 21 Feb 2024 11:49:01 +1100 Subject: [PATCH 04/21] Flatten the parse logic in `line_directive` --- src/tools/compiletest/src/header.rs | 31 ++++++++++++----------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 50e8cfd60a9fb..44e5d8dea7dcc 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -650,27 +650,22 @@ impl TestProps { /// See [`HeaderLine`] for a diagram. pub fn line_directive<'line>( comment: &str, - ln: &'line str, + original_line: &'line str, ) -> Option<(Option<&'line str>, &'line str)> { - let ln = ln.trim_start(); - if ln.starts_with(comment) { - let ln = ln[comment.len()..].trim_start(); - if ln.starts_with('[') { - // A comment like `//[foo]` is specific to revision `foo` - let Some(close_brace) = ln.find(']') else { - panic!( - "malformed condition directive: expected `{}[foo]`, found `{}`", - comment, ln - ); - }; + // Ignore lines that don't start with the comment prefix. + let after_comment = original_line.trim_start().strip_prefix(comment)?.trim_start(); + + if let Some(after_open_bracket) = after_comment.strip_prefix('[') { + // A comment like `//@[foo]` only applies to revision `foo`. + let Some((line_revision, directive)) = after_open_bracket.split_once(']') else { + panic!( + "malformed condition directive: expected `{comment}[foo]`, found `{original_line}`" + ) + }; - let line_revision = &ln[1..close_brace]; - Some((Some(line_revision), ln[(close_brace + 1)..].trim_start())) - } else { - Some((None, ln)) - } + Some((Some(line_revision), directive.trim_start())) } else { - None + Some((None, after_comment)) } } From ec91209f964182655cc1330c0a6f79c3e8bca7df Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 18 Feb 2024 20:38:07 +1100 Subject: [PATCH 05/21] coverage: Eagerly deduplicate covspans with the same span --- .../src/coverage/spans/from_mir.rs | 15 ++++++++++----- tests/coverage/closure_macro.cov-map | 9 ++++----- tests/coverage/closure_macro_async.cov-map | 9 ++++----- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs index 2db358379fe51..b91ab811918a7 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs @@ -52,14 +52,19 @@ pub(super) fn mir_to_initial_sorted_coverage_spans( // - Span A extends further left, or // - Both have the same start and span A extends further right .then_with(|| Ord::cmp(&a.span.hi(), &b.span.hi()).reverse()) - // If both spans are equal, sort the BCBs in dominator order, - // so that dominating BCBs come before other BCBs they dominate. - .then_with(|| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb)) - // If two spans are otherwise identical, put closure spans first, - // as this seems to be what the refinement step expects. + // If two spans have the same lo & hi, put closure spans first, + // as they take precedence over non-closure spans. .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse()) + // After deduplication, we want to keep only the most-dominated BCB. + .then_with(|| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb).reverse()) }); + // Among covspans with the same span, keep only one. Closure spans take + // precedence, otherwise keep the one with the most-dominated BCB. + // (Ideally we should try to preserve _all_ non-dominating BCBs, but that + // requires a lot more complexity in the span refiner, for little benefit.) + initial_spans.dedup_by(|b, a| a.span.source_equal(b.span)); + initial_spans } diff --git a/tests/coverage/closure_macro.cov-map b/tests/coverage/closure_macro.cov-map index 571e5564b659c..e43ed1f76f366 100644 --- a/tests/coverage/closure_macro.cov-map +++ b/tests/coverage/closure_macro.cov-map @@ -7,18 +7,17 @@ Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 29, 1) to (start + 2, 2) Function name: closure_macro::main -Raw bytes (43): 0x[01, 01, 02, 01, 05, 05, 02, 07, 01, 21, 01, 01, 21, 02, 02, 09, 00, 0f, 05, 00, 12, 00, 13, 02, 00, 12, 00, 13, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02] +Raw bytes (38): 0x[01, 01, 02, 01, 05, 05, 02, 06, 01, 21, 01, 01, 21, 02, 02, 09, 00, 12, 02, 00, 0f, 00, 54, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02] Number of files: 1 - file 0 => global file 1 Number of expressions: 2 - expression 0 operands: lhs = Counter(0), rhs = Counter(1) - expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub) -Number of file 0 mappings: 7 +Number of file 0 mappings: 6 - Code(Counter(0)) at (prev + 33, 1) to (start + 1, 33) -- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 15) +- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 18) = (c0 - c1) -- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) -- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19) +- Code(Expression(0, Sub)) at (prev + 0, 15) to (start + 0, 84) = (c0 - c1) - Code(Counter(1)) at (prev + 0, 84) to (start + 0, 85) - Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 2, 11) diff --git a/tests/coverage/closure_macro_async.cov-map b/tests/coverage/closure_macro_async.cov-map index 49ec767eab33e..212b67a8a3e1d 100644 --- a/tests/coverage/closure_macro_async.cov-map +++ b/tests/coverage/closure_macro_async.cov-map @@ -15,18 +15,17 @@ Number of file 0 mappings: 1 - Code(Counter(0)) at (prev + 35, 1) to (start + 0, 43) Function name: closure_macro_async::test::{closure#0} -Raw bytes (43): 0x[01, 01, 02, 01, 05, 05, 02, 07, 01, 23, 2b, 01, 21, 02, 02, 09, 00, 0f, 05, 00, 12, 00, 13, 02, 00, 12, 00, 13, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02] +Raw bytes (38): 0x[01, 01, 02, 01, 05, 05, 02, 06, 01, 23, 2b, 01, 21, 02, 02, 09, 00, 12, 02, 00, 0f, 00, 54, 05, 00, 54, 00, 55, 02, 02, 09, 02, 0b, 07, 03, 01, 00, 02] Number of files: 1 - file 0 => global file 1 Number of expressions: 2 - expression 0 operands: lhs = Counter(0), rhs = Counter(1) - expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub) -Number of file 0 mappings: 7 +Number of file 0 mappings: 6 - Code(Counter(0)) at (prev + 35, 43) to (start + 1, 33) -- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 15) +- Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 0, 18) = (c0 - c1) -- Code(Counter(1)) at (prev + 0, 18) to (start + 0, 19) -- Code(Expression(0, Sub)) at (prev + 0, 18) to (start + 0, 19) +- Code(Expression(0, Sub)) at (prev + 0, 15) to (start + 0, 84) = (c0 - c1) - Code(Counter(1)) at (prev + 0, 84) to (start + 0, 85) - Code(Expression(0, Sub)) at (prev + 2, 9) to (start + 2, 11) From c40261da11a58247822c9297993eb2142d8d53a2 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 18 Feb 2024 20:49:47 +1100 Subject: [PATCH 06/21] coverage: Remove `pending_dups` from the span refiner --- .../rustc_mir_transform/src/coverage/spans.rs | 180 ++---------------- 1 file changed, 16 insertions(+), 164 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index 934e77e7deb0a..c2e348cb7131b 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -61,7 +61,7 @@ pub(super) fn generate_coverage_spans( hir_info, basic_coverage_blocks, ); - let coverage_spans = SpansRefiner::refine_sorted_spans(basic_coverage_blocks, sorted_spans); + let coverage_spans = SpansRefiner::refine_sorted_spans(sorted_spans); mappings.extend(coverage_spans.into_iter().map(|RefinedCovspan { bcb, span, .. }| { // Each span produced by the generator represents an ordinary code region. BcbMapping { kind: BcbMappingKind::Code(bcb), span } @@ -88,8 +88,6 @@ pub(super) fn generate_coverage_spans( #[derive(Debug)] struct CurrCovspan { - /// This is used as the basis for [`PrevCovspan::original_span`], so it must - /// not be modified. span: Span, bcb: BasicCoverageBlock, is_closure: bool, @@ -102,7 +100,7 @@ impl CurrCovspan { fn into_prev(self) -> PrevCovspan { let Self { span, bcb, is_closure } = self; - PrevCovspan { original_span: span, span, bcb, merged_spans: vec![span], is_closure } + PrevCovspan { span, bcb, merged_spans: vec![span], is_closure } } fn into_refined(self) -> RefinedCovspan { @@ -115,7 +113,6 @@ impl CurrCovspan { #[derive(Debug)] struct PrevCovspan { - original_span: Span, span: Span, bcb: BasicCoverageBlock, /// List of all the original spans from MIR that have been merged into this @@ -142,35 +139,8 @@ impl PrevCovspan { } } - fn into_dup(self) -> DuplicateCovspan { - let Self { original_span, span, bcb, merged_spans: _, is_closure } = self; - // Only unmodified spans end up in `pending_dups`. - debug_assert_eq!(original_span, span); - DuplicateCovspan { span, bcb, is_closure } - } - - fn refined_copy(&self) -> RefinedCovspan { - let &Self { original_span: _, span, bcb, merged_spans: _, is_closure } = self; - RefinedCovspan { span, bcb, is_closure } - } - - fn into_refined(self) -> RefinedCovspan { - self.refined_copy() - } -} - -#[derive(Debug)] -struct DuplicateCovspan { - span: Span, - bcb: BasicCoverageBlock, - is_closure: bool, -} - -impl DuplicateCovspan { - /// Returns a copy of this covspan, as a [`RefinedCovspan`]. - /// Should only be called in places that would otherwise clone this covspan. fn refined_copy(&self) -> RefinedCovspan { - let &Self { span, bcb, is_closure } = self; + let &Self { span, bcb, merged_spans: _, is_closure } = self; RefinedCovspan { span, bcb, is_closure } } @@ -205,10 +175,7 @@ impl RefinedCovspan { /// * Merge spans that represent continuous (both in source code and control flow), non-branching /// execution /// * Carve out (leave uncovered) any span that will be counted by another MIR (notably, closures) -struct SpansRefiner<'a> { - /// The BasicCoverageBlock Control Flow Graph (BCB CFG). - basic_coverage_blocks: &'a CoverageGraph, - +struct SpansRefiner { /// The initial set of coverage spans, sorted by `Span` (`lo` and `hi`) and by relative /// dominance between the `BasicCoverageBlock`s of equal `Span`s. sorted_spans_iter: std::vec::IntoIter, @@ -223,36 +190,22 @@ struct SpansRefiner<'a> { /// If that `curr` was discarded, `prev` retains its value from the previous iteration. some_prev: Option, - /// One or more coverage spans with the same `Span` but different `BasicCoverageBlock`s, and - /// no `BasicCoverageBlock` in this list dominates another `BasicCoverageBlock` in the list. - /// If a new `curr` span also fits this criteria (compared to an existing list of - /// `pending_dups`), that `curr` moves to `prev` before possibly being added to - /// the `pending_dups` list, on the next iteration. As a result, if `prev` and `pending_dups` - /// have the same `Span`, the criteria for `pending_dups` holds for `prev` as well: a `prev` - /// with a matching `Span` does not dominate any `pending_dup` and no `pending_dup` dominates a - /// `prev` with a matching `Span`) - pending_dups: Vec, - /// The final coverage spans to add to the coverage map. A `Counter` or `Expression` /// will also be injected into the MIR for each BCB that has associated spans. refined_spans: Vec, } -impl<'a> SpansRefiner<'a> { +impl SpansRefiner { /// Takes the initial list of (sorted) spans extracted from MIR, and "refines" /// them by merging compatible adjacent spans, removing redundant spans, /// and carving holes in spans when they overlap in unwanted ways. - fn refine_sorted_spans( - basic_coverage_blocks: &'a CoverageGraph, - sorted_spans: Vec, - ) -> Vec { + fn refine_sorted_spans(sorted_spans: Vec) -> Vec { + let sorted_spans_len = sorted_spans.len(); let this = Self { - basic_coverage_blocks, sorted_spans_iter: sorted_spans.into_iter(), some_curr: None, some_prev: None, - pending_dups: Vec::new(), - refined_spans: Vec::with_capacity(basic_coverage_blocks.num_nodes() * 2), + refined_spans: Vec::with_capacity(sorted_spans_len), }; this.to_refined_spans() @@ -292,21 +245,11 @@ impl<'a> SpansRefiner<'a> { self.take_curr(); // Discards curr. } else if curr.is_closure { self.carve_out_span_for_closure(); - } else if prev.original_span == prev.span && prev.span == curr.span { - // Prev and curr have the same span, and prev's span hasn't - // been modified by other spans. - self.update_pending_dups(); } else { self.cutoff_prev_at_overlapping_curr(); } } - // Drain any remaining dups into the output. - for dup in self.pending_dups.drain(..) { - debug!(" ...adding at least one pending dup={:?}", dup); - self.refined_spans.push(dup.into_refined()); - } - // There is usually a final span remaining in `prev` after the loop ends, // so add it to the output as well. if let Some(prev) = self.some_prev.take() { @@ -359,36 +302,6 @@ impl<'a> SpansRefiner<'a> { self.some_prev.take().unwrap_or_else(|| bug!("some_prev is None (take_prev)")) } - /// If there are `pending_dups` but `prev` is not a matching dup (`prev.span` doesn't match the - /// `pending_dups` spans), then one of the following two things happened during the previous - /// iteration: - /// * the previous `curr` span (which is now `prev`) was not a duplicate of the pending_dups - /// (in which case there should be at least two spans in `pending_dups`); or - /// * the `span` of `prev` was modified by `curr_mut().merge_from(prev)` (in which case - /// `pending_dups` could have as few as one span) - /// In either case, no more spans will match the span of `pending_dups`, so - /// add the `pending_dups` if they don't overlap `curr`, and clear the list. - fn maybe_flush_pending_dups(&mut self) { - let Some(last_dup) = self.pending_dups.last() else { return }; - if last_dup.span == self.prev().span { - return; - } - - debug!( - " SAME spans, but pending_dups are NOT THE SAME, so BCBs matched on \ - previous iteration, or prev started a new disjoint span" - ); - if last_dup.span.hi() <= self.curr().span.lo() { - for dup in self.pending_dups.drain(..) { - debug!(" ...adding at least one pending={:?}", dup); - self.refined_spans.push(dup.into_refined()); - } - } else { - self.pending_dups.clear(); - } - assert!(self.pending_dups.is_empty()); - } - /// Advance `prev` to `curr` (if any), and `curr` to the next coverage span in sorted order. fn next_coverage_span(&mut self) -> bool { if let Some(curr) = self.some_curr.take() { @@ -408,7 +321,6 @@ impl<'a> SpansRefiner<'a> { ); } else { self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_closure)); - self.maybe_flush_pending_dups(); return true; } } @@ -433,13 +345,6 @@ impl<'a> SpansRefiner<'a> { let mut pre_closure = self.prev().refined_copy(); pre_closure.span = pre_closure.span.with_hi(left_cutoff); debug!(" prev overlaps a closure. Adding span for pre_closure={:?}", pre_closure); - - for mut dup in self.pending_dups.iter().map(DuplicateCovspan::refined_copy) { - dup.span = dup.span.with_hi(left_cutoff); - debug!(" ...and at least one pre_closure dup={:?}", dup); - self.refined_spans.push(dup); - } - self.refined_spans.push(pre_closure); } @@ -448,58 +353,9 @@ impl<'a> SpansRefiner<'a> { self.prev_mut().span = self.prev().span.with_lo(right_cutoff); debug!(" Mutated prev.span to start after the closure. prev={:?}", self.prev()); - for dup in &mut self.pending_dups { - debug!(" ...and at least one overlapping dup={:?}", dup); - dup.span = dup.span.with_lo(right_cutoff); - } - // Prevent this curr from becoming prev. let closure_covspan = self.take_curr().into_refined(); self.refined_spans.push(closure_covspan); // since self.prev() was already updated - } else { - self.pending_dups.clear(); - } - } - - /// Called if `curr.span` equals `prev.original_span` (and potentially equal to all - /// `pending_dups` spans, if any). Keep in mind, `prev.span()` may have been changed. - /// If prev.span() was merged into other spans (with matching BCB, for instance), - /// `prev.span.hi()` will be greater than (further right of) `prev.original_span.hi()`. - /// If prev.span() was split off to the right of a closure, prev.span().lo() will be - /// greater than prev.original_span.lo(). The actual span of `prev.original_span` is - /// not as important as knowing that `prev()` **used to have the same span** as `curr()`, - /// which means their sort order is still meaningful for determining the dominator - /// relationship. - /// - /// When two coverage spans have the same `Span`, dominated spans can be discarded; but if - /// neither coverage span dominates the other, both (or possibly more than two) are held, - /// until their disposition is determined. In this latter case, the `prev` dup is moved into - /// `pending_dups` so the new `curr` dup can be moved to `prev` for the next iteration. - fn update_pending_dups(&mut self) { - let prev_bcb = self.prev().bcb; - let curr_bcb = self.curr().bcb; - - // Equal coverage spans are ordered by dominators before dominated (if any), so it should be - // impossible for `curr` to dominate any previous coverage span. - debug_assert!(!self.basic_coverage_blocks.dominates(curr_bcb, prev_bcb)); - - // `prev` is a duplicate of `curr`, so add it to the list of pending dups. - // If it dominates `curr`, it will be removed by the subsequent discard step. - let prev = self.take_prev().into_dup(); - debug!(?prev, "adding prev to pending dups"); - self.pending_dups.push(prev); - - let initial_pending_count = self.pending_dups.len(); - if initial_pending_count > 0 { - self.pending_dups - .retain(|dup| !self.basic_coverage_blocks.dominates(dup.bcb, curr_bcb)); - - let n_discarded = initial_pending_count - self.pending_dups.len(); - if n_discarded > 0 { - debug!( - " discarded {n_discarded} of {initial_pending_count} pending_dups that dominated curr", - ); - } } } @@ -516,19 +372,15 @@ impl<'a> SpansRefiner<'a> { if it has statements that end before curr; prev={:?}", self.prev() ); - if self.pending_dups.is_empty() { - let curr_span = self.curr().span; - self.prev_mut().cutoff_statements_at(curr_span.lo()); - if self.prev().merged_spans.is_empty() { - debug!(" ... no non-overlapping statements to add"); - } else { - debug!(" ... adding modified prev={:?}", self.prev()); - let prev = self.take_prev().into_refined(); - self.refined_spans.push(prev); - } + + let curr_span = self.curr().span; + self.prev_mut().cutoff_statements_at(curr_span.lo()); + if self.prev().merged_spans.is_empty() { + debug!(" ... no non-overlapping statements to add"); } else { - // with `pending_dups`, `prev` cannot have any statements that don't overlap - self.pending_dups.clear(); + debug!(" ... adding modified prev={:?}", self.prev()); + let prev = self.take_prev().into_refined(); + self.refined_spans.push(prev); } } } From 3a83b279bef65f8b727d33d1c7a80e5d2432f5aa Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sun, 18 Feb 2024 20:54:10 +1100 Subject: [PATCH 07/21] coverage: Simplify (non-closure) covspans truncating each other --- compiler/rustc_mir_transform/src/coverage/spans.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/spans.rs b/compiler/rustc_mir_transform/src/coverage/spans.rs index c2e348cb7131b..98fb1d8e1c940 100644 --- a/compiler/rustc_mir_transform/src/coverage/spans.rs +++ b/compiler/rustc_mir_transform/src/coverage/spans.rs @@ -132,11 +132,13 @@ impl PrevCovspan { self.merged_spans.push(other.span); } - fn cutoff_statements_at(&mut self, cutoff_pos: BytePos) { + fn cutoff_statements_at(mut self, cutoff_pos: BytePos) -> Option { self.merged_spans.retain(|span| span.hi() <= cutoff_pos); if let Some(max_hi) = self.merged_spans.iter().map(|span| span.hi()).max() { self.span = self.span.with_hi(max_hi); } + + if self.merged_spans.is_empty() { None } else { Some(self.into_refined()) } } fn refined_copy(&self) -> RefinedCovspan { @@ -374,13 +376,11 @@ impl SpansRefiner { ); let curr_span = self.curr().span; - self.prev_mut().cutoff_statements_at(curr_span.lo()); - if self.prev().merged_spans.is_empty() { - debug!(" ... no non-overlapping statements to add"); - } else { - debug!(" ... adding modified prev={:?}", self.prev()); - let prev = self.take_prev().into_refined(); + if let Some(prev) = self.take_prev().cutoff_statements_at(curr_span.lo()) { + debug!("after cutoff, adding {prev:?}"); self.refined_spans.push(prev); + } else { + debug!("prev was eliminated by cutoff"); } } } From 5e0e5b1efb8a5776231b07c71e7c03ef016654b5 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 20 Feb 2024 02:55:40 +0100 Subject: [PATCH 08/21] Fix liveness analysis in the presence of never patterns --- compiler/rustc_hir/src/pat_util.rs | 11 +++++++++-- compiler/rustc_passes/src/liveness.rs | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_hir/src/pat_util.rs b/compiler/rustc_hir/src/pat_util.rs index e605032718623..1eaab3d2acac3 100644 --- a/compiler/rustc_hir/src/pat_util.rs +++ b/compiler/rustc_hir/src/pat_util.rs @@ -71,14 +71,21 @@ impl hir::Pat<'_> { /// Call `f` on every "binding" in a pattern, e.g., on `a` in /// `match foo() { Some(a) => (), None => () }`. /// - /// When encountering an or-pattern `p_0 | ... | p_n` only `p_0` will be visited. + /// When encountering an or-pattern `p_0 | ... | p_n` only the first non-never pattern will be + /// visited. If they're all never patterns we visit nothing, which is ok since a never pattern + /// cannot have bindings. pub fn each_binding_or_first( &self, f: &mut impl FnMut(hir::BindingAnnotation, HirId, Span, Ident), ) { self.walk(|p| match &p.kind { PatKind::Or(ps) => { - ps[0].each_binding_or_first(f); + for p in *ps { + if !p.is_never_pattern() { + p.each_binding_or_first(f); + break; + } + } false } PatKind::Binding(bm, _, ident, _) => { diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index 3a8dc3775206c..487407014d178 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -526,8 +526,8 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { } fn define_bindings_in_pat(&mut self, pat: &hir::Pat<'_>, mut succ: LiveNode) -> LiveNode { - // In an or-pattern, only consider the first pattern; any later patterns - // must have the same bindings, and we also consider the first pattern + // In an or-pattern, only consider the first non-never pattern; any later patterns + // must have the same bindings, and we also consider that pattern // to be the "authoritative" set of ids. pat.each_binding_or_first(&mut |_, hir_id, pat_sp, ident| { let ln = self.live_node(hir_id, pat_sp); From f25c90a83f661d39d9d55e61eaaf4b1beaabaf20 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Wed, 21 Feb 2024 11:17:07 +0000 Subject: [PATCH 09/21] Unify dylib loading between proc macros and codegen backends As bonus this makes the errors when failing to load a proc macro more informative to match the backend loading errors. In addition it makes it slightly easier to patch rustc to work on platforms that don't support dynamic linking like wasm. --- Cargo.lock | 1 - compiler/rustc_interface/Cargo.toml | 1 - compiler/rustc_interface/src/lib.rs | 1 - compiler/rustc_interface/src/util.rs | 33 +++++---------- compiler/rustc_metadata/messages.ftl | 2 +- compiler/rustc_metadata/src/creader.rs | 56 +++++++++++++++++++------- compiler/rustc_metadata/src/errors.rs | 1 + compiler/rustc_metadata/src/lib.rs | 2 + compiler/rustc_metadata/src/locator.rs | 8 ++-- 9 files changed, 60 insertions(+), 45 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 61f9c130e38d7..4dbe4b1b82285 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4047,7 +4047,6 @@ dependencies = [ name = "rustc_interface" version = "0.0.0" dependencies = [ - "libloading", "rustc-rayon", "rustc-rayon-core", "rustc_ast", diff --git a/compiler/rustc_interface/Cargo.toml b/compiler/rustc_interface/Cargo.toml index a238eacda44ba..0e90836145efa 100644 --- a/compiler/rustc_interface/Cargo.toml +++ b/compiler/rustc_interface/Cargo.toml @@ -5,7 +5,6 @@ edition = "2021" [dependencies] # tidy-alphabetical-start -libloading = "0.8.0" rustc-rayon = { version = "0.5.0", optional = true } rustc-rayon-core = { version = "0.5.0", optional = true } rustc_ast = { path = "../rustc_ast" } diff --git a/compiler/rustc_interface/src/lib.rs b/compiler/rustc_interface/src/lib.rs index 24c2e29053488..d0ce23dacb5ef 100644 --- a/compiler/rustc_interface/src/lib.rs +++ b/compiler/rustc_interface/src/lib.rs @@ -1,5 +1,4 @@ #![feature(decl_macro)] -#![feature(error_iter)] #![feature(generic_nonzero)] #![feature(lazy_cell)] #![feature(let_chains)] diff --git a/compiler/rustc_interface/src/util.rs b/compiler/rustc_interface/src/util.rs index 087c43075f175..823614e1f0619 100644 --- a/compiler/rustc_interface/src/util.rs +++ b/compiler/rustc_interface/src/util.rs @@ -1,10 +1,10 @@ use crate::errors; use info; -use libloading::Library; use rustc_ast as ast; use rustc_codegen_ssa::traits::CodegenBackend; #[cfg(parallel_compiler)] use rustc_data_structures::sync; +use rustc_metadata::{load_symbol_from_dylib, DylibError}; use rustc_parse::validate_attr; use rustc_session as session; use rustc_session::config::{self, Cfg, CrateType, OutFileName, OutputFilenames, OutputTypes}; @@ -17,7 +17,6 @@ use rustc_span::symbol::{sym, Symbol}; use session::EarlyDiagCtxt; use std::env; use std::env::consts::{DLL_PREFIX, DLL_SUFFIX}; -use std::mem; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::OnceLock; @@ -162,29 +161,19 @@ pub(crate) fn run_in_thread_pool_with_globals R + Send, R: Send>( } fn load_backend_from_dylib(early_dcx: &EarlyDiagCtxt, path: &Path) -> MakeBackendFn { - fn format_err(e: &(dyn std::error::Error + 'static)) -> String { - e.sources().map(|e| format!(": {e}")).collect() - } - let lib = unsafe { Library::new(path) }.unwrap_or_else(|err| { - let err = format!("couldn't load codegen backend {path:?}{}", format_err(&err)); - early_dcx.early_fatal(err); - }); - - let backend_sym = unsafe { lib.get::(b"__rustc_codegen_backend") } - .unwrap_or_else(|e| { + match unsafe { load_symbol_from_dylib::(path, "__rustc_codegen_backend") } { + Ok(backend_sym) => backend_sym, + Err(DylibError::DlOpen(path, err)) => { + let err = format!("couldn't load codegen backend {path}{err}"); + early_dcx.early_fatal(err); + } + Err(DylibError::DlSym(_path, err)) => { let e = format!( - "`__rustc_codegen_backend` symbol lookup in the codegen backend failed{}", - format_err(&e) + "`__rustc_codegen_backend` symbol lookup in the codegen backend failed{err}", ); early_dcx.early_fatal(e); - }); - - // Intentionally leak the dynamic library. We can't ever unload it - // since the library can make things that will live arbitrarily long. - let backend_sym = unsafe { backend_sym.into_raw() }; - mem::forget(lib); - - *backend_sym + } + } } /// Get the codegen backend based on the name and specified sysroot. diff --git a/compiler/rustc_metadata/messages.ftl b/compiler/rustc_metadata/messages.ftl index 8da6f0007f03a..a1c6fba4d435e 100644 --- a/compiler/rustc_metadata/messages.ftl +++ b/compiler/rustc_metadata/messages.ftl @@ -45,7 +45,7 @@ metadata_crate_not_panic_runtime = the crate `{$crate_name}` is not a panic runtime metadata_dl_error = - {$err} + {$path}{$err} metadata_empty_link_name = link name must not be empty diff --git a/compiler/rustc_metadata/src/creader.rs b/compiler/rustc_metadata/src/creader.rs index f6d3dba247090..8fea1a722239b 100644 --- a/compiler/rustc_metadata/src/creader.rs +++ b/compiler/rustc_metadata/src/creader.rs @@ -692,20 +692,8 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { path: &Path, stable_crate_id: StableCrateId, ) -> Result<&'static [ProcMacro], CrateError> { - // Make sure the path contains a / or the linker will search for it. - let path = try_canonicalize(path).unwrap(); - let lib = load_dylib(&path, 5).map_err(|err| CrateError::DlOpen(err))?; - let sym_name = self.sess.generate_proc_macro_decls_symbol(stable_crate_id); - let sym = unsafe { lib.get::<*const &[ProcMacro]>(sym_name.as_bytes()) } - .map_err(|err| CrateError::DlSym(err.to_string()))?; - - // Intentionally leak the dynamic library. We can't ever unload it - // since the library can make things that will live arbitrarily long. - let sym = unsafe { sym.into_raw() }; - std::mem::forget(lib); - - Ok(unsafe { **sym }) + Ok(unsafe { *load_symbol_from_dylib::<*const &[ProcMacro]>(path, &sym_name)? }) } fn inject_panic_runtime(&mut self, krate: &ast::Crate) { @@ -1116,6 +1104,10 @@ fn alloc_error_handler_spans(krate: &ast::Crate) -> Vec { f.spans } +fn format_dlopen_err(e: &(dyn std::error::Error + 'static)) -> String { + e.sources().map(|e| format!(": {e}")).collect() +} + // On Windows the compiler would sometimes intermittently fail to open the // proc-macro DLL with `Error::LoadLibraryExW`. It is suspected that something in the // system still holds a lock on the file, so we retry a few times before calling it @@ -1154,9 +1146,43 @@ fn load_dylib(path: &Path, max_attempts: usize) -> Result for CrateError { + fn from(err: DylibError) -> CrateError { + match err { + DylibError::DlOpen(path, err) => CrateError::DlOpen(path, err), + DylibError::DlSym(path, err) => CrateError::DlSym(path, err), + } + } +} + +pub unsafe fn load_symbol_from_dylib( + path: &Path, + sym_name: &str, +) -> Result { + // Make sure the path contains a / or the linker will search for it. + let path = try_canonicalize(path).unwrap(); + let lib = + load_dylib(&path, 5).map_err(|err| DylibError::DlOpen(path.display().to_string(), err))?; + + let sym = unsafe { lib.get::(sym_name.as_bytes()) } + .map_err(|err| DylibError::DlSym(path.display().to_string(), format_dlopen_err(&err)))?; + + // Intentionally leak the dynamic library. We can't ever unload it + // since the library can make things that will live arbitrarily long. + let sym = unsafe { sym.into_raw() }; + std::mem::forget(lib); + + Ok(*sym) +} diff --git a/compiler/rustc_metadata/src/errors.rs b/compiler/rustc_metadata/src/errors.rs index 7e0a4fb72d45c..9a05d9ac0def1 100644 --- a/compiler/rustc_metadata/src/errors.rs +++ b/compiler/rustc_metadata/src/errors.rs @@ -535,6 +535,7 @@ pub struct StableCrateIdCollision { pub struct DlError { #[primary_span] pub span: Span, + pub path: String, pub err: String, } diff --git a/compiler/rustc_metadata/src/lib.rs b/compiler/rustc_metadata/src/lib.rs index 70ad859895724..f133a2f5f73b2 100644 --- a/compiler/rustc_metadata/src/lib.rs +++ b/compiler/rustc_metadata/src/lib.rs @@ -3,6 +3,7 @@ #![feature(rustdoc_internals)] #![allow(internal_features)] #![feature(decl_macro)] +#![feature(error_iter)] #![feature(extract_if)] #![feature(coroutines)] #![feature(generic_nonzero)] @@ -39,6 +40,7 @@ pub mod errors; pub mod fs; pub mod locator; +pub use creader::{load_symbol_from_dylib, DylibError}; pub use fs::{emit_wrapper_file, METADATA_FILENAME}; pub use native_libs::find_native_static_library; pub use rmeta::{encode_metadata, rendered_const, EncodedMetadata, METADATA_HEADER}; diff --git a/compiler/rustc_metadata/src/locator.rs b/compiler/rustc_metadata/src/locator.rs index 1ab965e28769f..15f56db6278b1 100644 --- a/compiler/rustc_metadata/src/locator.rs +++ b/compiler/rustc_metadata/src/locator.rs @@ -921,8 +921,8 @@ pub(crate) enum CrateError { MultipleCandidates(Symbol, CrateFlavor, Vec), SymbolConflictsCurrent(Symbol), StableCrateIdCollision(Symbol, Symbol), - DlOpen(String), - DlSym(String), + DlOpen(String, String), + DlSym(String, String), LocatorCombined(Box), NotFound(Symbol), } @@ -967,8 +967,8 @@ impl CrateError { CrateError::StableCrateIdCollision(crate_name0, crate_name1) => { dcx.emit_err(errors::StableCrateIdCollision { span, crate_name0, crate_name1 }); } - CrateError::DlOpen(s) | CrateError::DlSym(s) => { - dcx.emit_err(errors::DlError { span, err: s }); + CrateError::DlOpen(path, err) | CrateError::DlSym(path, err) => { + dcx.emit_err(errors::DlError { span, path, err }); } CrateError::LocatorCombined(locator) => { let crate_name = locator.crate_name; From a17211b05c883eaeb4057f0a9207947bcbcc3688 Mon Sep 17 00:00:00 2001 From: Petr Sumbera Date: Wed, 21 Feb 2024 16:49:01 +0100 Subject: [PATCH 10/21] Solaris linker does not support --strip-debug Fixes #121381 --- compiler/rustc_codegen_ssa/src/back/linker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_codegen_ssa/src/back/linker.rs b/compiler/rustc_codegen_ssa/src/back/linker.rs index 9f06f398288f2..1f3383815e226 100644 --- a/compiler/rustc_codegen_ssa/src/back/linker.rs +++ b/compiler/rustc_codegen_ssa/src/back/linker.rs @@ -626,7 +626,7 @@ impl<'a> Linker for GccLinker<'a> { // it does support --strip-all as a compatibility alias for -s. // The --strip-debug case is handled by running an external // `strip` utility as a separate step after linking. - if self.sess.target.os != "illumos" { + if !self.sess.target.is_like_solaris { self.linker_arg("--strip-debug"); } } From a233b1656e0311e6a9842546d2f4273a95ebf6d6 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 21 Feb 2024 17:16:35 +0000 Subject: [PATCH 11/21] Add an ATB test --- .../associated-type-bounds/no-gat-position.rs | 18 ++++++++++++++++++ .../no-gat-position.stderr | 9 +++++++++ 2 files changed, 27 insertions(+) create mode 100644 tests/ui/associated-type-bounds/no-gat-position.rs create mode 100644 tests/ui/associated-type-bounds/no-gat-position.stderr diff --git a/tests/ui/associated-type-bounds/no-gat-position.rs b/tests/ui/associated-type-bounds/no-gat-position.rs new file mode 100644 index 0000000000000..01740e6242e98 --- /dev/null +++ b/tests/ui/associated-type-bounds/no-gat-position.rs @@ -0,0 +1,18 @@ +#![feature(associated_type_bounds)] + +// Test for . + +pub trait Iter { + type Item<'a>: 'a where Self: 'a; + + fn next<'a>(&'a mut self) -> Option>; + //~^ ERROR associated type bindings are not allowed here +} + +impl Iter for () { + type Item<'a> = &'a mut [()]; + + fn next<'a>(&'a mut self) -> Option> { None } +} + +fn main() {} diff --git a/tests/ui/associated-type-bounds/no-gat-position.stderr b/tests/ui/associated-type-bounds/no-gat-position.stderr new file mode 100644 index 0000000000000..5692b2c7d0902 --- /dev/null +++ b/tests/ui/associated-type-bounds/no-gat-position.stderr @@ -0,0 +1,9 @@ +error[E0229]: associated type bindings are not allowed here + --> $DIR/no-gat-position.rs:8:56 + | +LL | fn next<'a>(&'a mut self) -> Option>; + | ^^^^^^^^^ associated type not allowed here + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0229`. From f8fbb7060c746cafc0d503b119e4ece019e1dcb5 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Wed, 21 Feb 2024 17:31:53 +0000 Subject: [PATCH 12/21] Add a non-lifetime-binders test --- .../bad-suggestion-on-missing-assoc.rs | 26 ++++++++++++++ .../bad-suggestion-on-missing-assoc.stderr | 34 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.rs create mode 100644 tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.stderr diff --git a/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.rs b/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.rs new file mode 100644 index 0000000000000..fc64381b961d4 --- /dev/null +++ b/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.rs @@ -0,0 +1,26 @@ +#![feature(generic_const_exprs)] +//~^ WARN the feature `generic_const_exprs` is incomplete +#![feature(non_lifetime_binders)] +//~^ WARN the feature `non_lifetime_binders` is incomplete + +// Test for , +// which originally relied on associated_type_bounds, but was +// minimized away from that. + +trait TraitA { + type AsA; +} +trait TraitB { + type AsB; +} +trait TraitC {} + +fn foo() +where + for T: TraitA>, + //~^ ERROR defaults for generic parameters are not allowed in `for<...>` binders + //~| ERROR `impl Trait` is not allowed in bounds +{ +} + +fn main() {} diff --git a/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.stderr b/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.stderr new file mode 100644 index 0000000000000..a4a79413a9be6 --- /dev/null +++ b/tests/ui/traits/non_lifetime_binders/bad-suggestion-on-missing-assoc.stderr @@ -0,0 +1,34 @@ +warning: the feature `generic_const_exprs` is incomplete and may not be safe to use and/or cause compiler crashes + --> $DIR/bad-suggestion-on-missing-assoc.rs:1:12 + | +LL | #![feature(generic_const_exprs)] + | ^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #76560 for more information + = note: `#[warn(incomplete_features)]` on by default + +warning: the feature `non_lifetime_binders` is incomplete and may not be safe to use and/or cause compiler crashes + --> $DIR/bad-suggestion-on-missing-assoc.rs:3:12 + | +LL | #![feature(non_lifetime_binders)] + | ^^^^^^^^^^^^^^^^^^^^ + | + = note: see issue #108185 for more information + +error: defaults for generic parameters are not allowed in `for<...>` binders + --> $DIR/bad-suggestion-on-missing-assoc.rs:20:9 + | +LL | for T: TraitA>, + | ^^^^^^^^^^^^^^^^^^^^^^ + +error[E0562]: `impl Trait` is not allowed in bounds + --> $DIR/bad-suggestion-on-missing-assoc.rs:20:49 + | +LL | for T: TraitA>, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `impl Trait` is only allowed in arguments and return types of functions and methods + +error: aborting due to 2 previous errors; 2 warnings emitted + +For more information about this error, try `rustc --explain E0562`. From 1e16927ef37eb841d6a38b5e61438931d20c43fb Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 16 Feb 2024 10:35:53 +1100 Subject: [PATCH 13/21] Remove an out-of-date comment. --- compiler/rustc_errors/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 052d9b3a78376..64b88adedbb54 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -1271,7 +1271,6 @@ impl DiagCtxtInner { fn emit_stashed_diagnostics(&mut self) { let has_errors = !self.err_guars.is_empty(); for (_, diag) in std::mem::take(&mut self.stashed_diagnostics).into_iter() { - // Decrement the count tracking the stash; emitting will increment it. if diag.is_error() { if diag.is_lint.is_none() { self.stashed_err_count -= 1; From 203b4332bb3da3af092344b3459416aeab4c90ef Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 16 Feb 2024 14:50:07 +1100 Subject: [PATCH 14/21] Remove dead `expect_error_or_delayed_bug` method. --- compiler/rustc_middle/src/ty/context.rs | 5 ----- compiler/rustc_type_ir/src/interner.rs | 3 --- 2 files changed, 8 deletions(-) diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index cc734e7157fa4..9d59f779470f6 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -153,11 +153,6 @@ impl<'tcx> Interner for TyCtxt<'tcx> { ) -> Self::Const { Const::new_bound(self, debruijn, var, ty) } - - fn expect_error_or_delayed_bug() { - let has_errors = ty::tls::with(|tcx| tcx.dcx().has_errors_or_lint_errors_or_delayed_bugs()); - assert!(has_errors.is_some()); - } } type InternedSet<'tcx, T> = ShardedHashMap, ()>; diff --git a/compiler/rustc_type_ir/src/interner.rs b/compiler/rustc_type_ir/src/interner.rs index 7728ee0e842a8..7d2c42a6dbe91 100644 --- a/compiler/rustc_type_ir/src/interner.rs +++ b/compiler/rustc_type_ir/src/interner.rs @@ -95,9 +95,6 @@ pub trait Interner: Sized { fn mk_bound_ty(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Ty; fn mk_bound_region(self, debruijn: DebruijnIndex, var: BoundVar) -> Self::Region; fn mk_bound_const(self, debruijn: DebruijnIndex, var: BoundVar, ty: Self::Ty) -> Self::Const; - - /// Assert that an error has been delayed or emitted. - fn expect_error_or_delayed_bug(); } /// Common capabilities of placeholder kinds From 9919c3dab3e4eabe466612de5f6c472e3e27ceb6 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 16 Feb 2024 15:26:42 +1100 Subject: [PATCH 15/21] Remove `EarlyDiagCtxt::abort_if_errors`. Its one use isn't necessary, because it's not possible for errors to have been emitted at that point. --- compiler/rustc_driver_impl/src/lib.rs | 5 ++--- compiler/rustc_session/src/session.rs | 4 ---- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_driver_impl/src/lib.rs b/compiler/rustc_driver_impl/src/lib.rs index 60f11b1bdd490..9b2d760282f1c 100644 --- a/compiler/rustc_driver_impl/src/lib.rs +++ b/compiler/rustc_driver_impl/src/lib.rs @@ -349,11 +349,10 @@ fn run_compiler( }, }; - callbacks.config(&mut config); - - default_early_dcx.abort_if_errors(); drop(default_early_dcx); + callbacks.config(&mut config); + interface::run_compiler(config, |compiler| { let sess = &compiler.sess; let codegen_backend = &*compiler.codegen_backend; diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 9d1133c487f65..422bbadf6a8a0 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -1410,10 +1410,6 @@ impl EarlyDiagCtxt { Self { dcx: DiagCtxt::with_emitter(emitter) } } - pub fn abort_if_errors(&self) { - self.dcx.abort_if_errors() - } - /// Swap out the underlying dcx once we acquire the user's preference on error emission /// format. Any errors prior to that will cause an abort and all stashed diagnostics of the /// previous dcx will be emitted. From 46f49833566381887ba74e3f756271a6e8723636 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 09:36:08 +1100 Subject: [PATCH 16/21] Adjust the `has_errors*` methods. Currently `has_errors` excludes lint errors. This commit changes it to include lint errors. The motivation for this is that for most places it doesn't matter whether lint errors are included or not. But there are multiple places where they must be includes, and only one place where they must not be included. So it makes sense for `has_errors` to do the thing that fits the most situations, and the new `has_errors_excluding_lint_errors` method in the one exceptional place. The same change is made for `err_count`. Annoyingly, this requires the introduction of `err_count_excluding_lint_errs` for one place, to preserve existing error printing behaviour. But I still think the change is worthwhile overall. --- compiler/rustc_errors/src/lib.rs | 46 +++++++++++-------- compiler/rustc_incremental/src/persist/fs.rs | 2 +- .../rustc_incremental/src/persist/save.rs | 4 +- compiler/rustc_infer/src/infer/mod.rs | 9 ++-- compiler/rustc_interface/src/passes.rs | 2 +- compiler/rustc_metadata/src/creader.rs | 2 +- .../rustc_query_system/src/dep_graph/graph.rs | 2 +- compiler/rustc_session/src/session.rs | 3 +- src/librustdoc/core.rs | 3 +- src/librustdoc/doctest.rs | 3 +- 10 files changed, 41 insertions(+), 35 deletions(-) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index 64b88adedbb54..a6b0a0e8f1751 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -754,13 +754,20 @@ impl DiagCtxt { self.inner.borrow_mut().emit_stashed_diagnostics() } - /// This excludes lint errors, delayed bugs, and stashed errors. + /// This excludes lint errors, delayed bugs and stashed errors. #[inline] - pub fn err_count(&self) -> usize { + pub fn err_count_excluding_lint_errs(&self) -> usize { self.inner.borrow().err_guars.len() } - /// This excludes normal errors, lint errors and delayed bugs. Unless + /// This excludes delayed bugs and stashed errors. + #[inline] + pub fn err_count(&self) -> usize { + let inner = self.inner.borrow(); + inner.err_guars.len() + inner.lint_err_guars.len() + } + + /// This excludes normal errors, lint errors, and delayed bugs. Unless /// absolutely necessary, avoid using this. It's dubious because stashed /// errors can later be cancelled, so the presence of a stashed error at /// some point of time doesn't guarantee anything -- there are no @@ -769,21 +776,21 @@ impl DiagCtxt { self.inner.borrow().stashed_err_count } - /// This excludes lint errors, delayed bugs, and stashed errors. - pub fn has_errors(&self) -> Option { - self.inner.borrow().has_errors() + /// This excludes lint errors, delayed bugs, and stashed errors. Unless + /// absolutely necessary, prefer `has_errors` to this method. + pub fn has_errors_excluding_lint_errors(&self) -> Option { + self.inner.borrow().has_errors_excluding_lint_errors() } - /// This excludes delayed bugs and stashed errors. Unless absolutely - /// necessary, prefer `has_errors` to this method. - pub fn has_errors_or_lint_errors(&self) -> Option { - self.inner.borrow().has_errors_or_lint_errors() + /// This excludes delayed bugs and stashed errors. + pub fn has_errors(&self) -> Option { + self.inner.borrow().has_errors() } /// This excludes stashed errors. Unless absolutely necessary, prefer - /// `has_errors` or `has_errors_or_lint_errors` to this method. - pub fn has_errors_or_lint_errors_or_delayed_bugs(&self) -> Option { - self.inner.borrow().has_errors_or_lint_errors_or_delayed_bugs() + /// `has_errors` to this method. + pub fn has_errors_or_delayed_bugs(&self) -> Option { + self.inner.borrow().has_errors_or_delayed_bugs() } pub fn print_error_count(&self, registry: &Registry) { @@ -1328,7 +1335,7 @@ impl DiagCtxtInner { DelayedBug => { // If we have already emitted at least one error, we don't need // to record the delayed bug, because it'll never be used. - return if let Some(guar) = self.has_errors_or_lint_errors() { + return if let Some(guar) = self.has_errors() { Some(guar) } else { let backtrace = std::backtrace::Backtrace::capture(); @@ -1444,17 +1451,16 @@ impl DiagCtxtInner { .is_some_and(|c| self.err_guars.len() + self.lint_err_guars.len() + 1 >= c.get()) } - fn has_errors(&self) -> Option { + fn has_errors_excluding_lint_errors(&self) -> Option { self.err_guars.get(0).copied() } - fn has_errors_or_lint_errors(&self) -> Option { - self.has_errors().or_else(|| self.lint_err_guars.get(0).copied()) + fn has_errors(&self) -> Option { + self.has_errors_excluding_lint_errors().or_else(|| self.lint_err_guars.get(0).copied()) } - fn has_errors_or_lint_errors_or_delayed_bugs(&self) -> Option { - self.has_errors_or_lint_errors() - .or_else(|| self.delayed_bugs.get(0).map(|(_, guar)| guar).copied()) + fn has_errors_or_delayed_bugs(&self) -> Option { + self.has_errors().or_else(|| self.delayed_bugs.get(0).map(|(_, guar)| guar).copied()) } /// Translate `message` eagerly with `args` to `SubdiagnosticMessage::Eager`. diff --git a/compiler/rustc_incremental/src/persist/fs.rs b/compiler/rustc_incremental/src/persist/fs.rs index 23d29916922ff..dd9c16d006a96 100644 --- a/compiler/rustc_incremental/src/persist/fs.rs +++ b/compiler/rustc_incremental/src/persist/fs.rs @@ -312,7 +312,7 @@ pub fn finalize_session_directory(sess: &Session, svh: Option) { let incr_comp_session_dir: PathBuf = sess.incr_comp_session_dir().clone(); - if sess.dcx().has_errors_or_lint_errors_or_delayed_bugs().is_some() { + if sess.dcx().has_errors_or_delayed_bugs().is_some() { // If there have been any errors during compilation, we don't want to // publish this session directory. Rather, we'll just delete it. diff --git a/compiler/rustc_incremental/src/persist/save.rs b/compiler/rustc_incremental/src/persist/save.rs index ff0c58d09de2d..32759f5284af7 100644 --- a/compiler/rustc_incremental/src/persist/save.rs +++ b/compiler/rustc_incremental/src/persist/save.rs @@ -32,7 +32,7 @@ pub fn save_dep_graph(tcx: TyCtxt<'_>) { return; } // This is going to be deleted in finalize_session_directory, so let's not create it. - if sess.dcx().has_errors_or_lint_errors_or_delayed_bugs().is_some() { + if sess.dcx().has_errors_or_delayed_bugs().is_some() { return; } @@ -87,7 +87,7 @@ pub fn save_work_product_index( return; } // This is going to be deleted in finalize_session_directory, so let's not create it - if sess.dcx().has_errors_or_lint_errors().is_some() { + if sess.dcx().has_errors().is_some() { return; } diff --git a/compiler/rustc_infer/src/infer/mod.rs b/compiler/rustc_infer/src/infer/mod.rs index 243558b11a863..f131d11c41159 100644 --- a/compiler/rustc_infer/src/infer/mod.rs +++ b/compiler/rustc_infer/src/infer/mod.rs @@ -713,7 +713,7 @@ impl<'tcx> InferCtxtBuilder<'tcx> { reported_trait_errors: Default::default(), reported_signature_mismatch: Default::default(), tainted_by_errors: Cell::new(None), - err_count_on_creation: tcx.dcx().err_count(), + err_count_on_creation: tcx.dcx().err_count_excluding_lint_errs(), stashed_err_count_on_creation: tcx.dcx().stashed_err_count(), universe: Cell::new(ty::UniverseIndex::ROOT), intercrate, @@ -1268,8 +1268,11 @@ impl<'tcx> InferCtxt<'tcx> { pub fn tainted_by_errors(&self) -> Option { if let Some(guar) = self.tainted_by_errors.get() { Some(guar) - } else if self.dcx().err_count() > self.err_count_on_creation { - // Errors reported since this infcx was made. + } else if self.dcx().err_count_excluding_lint_errs() > self.err_count_on_creation { + // Errors reported since this infcx was made. Lint errors are + // excluded to avoid some being swallowed in the presence of + // non-lint errors. (It's arguable whether or not this exclusion is + // important.) let guar = self.dcx().has_errors().unwrap(); self.set_tainted_by_errors(guar); Some(guar) diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index 60d13f02ad7b5..858db594b4773 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -772,7 +772,7 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> { // lot of annoying errors in the ui tests (basically, // lint warnings and so on -- kindck used to do this abort, but // kindck is gone now). -nmatsakis - if let Some(reported) = sess.dcx().has_errors() { + if let Some(reported) = sess.dcx().has_errors_excluding_lint_errors() { return Err(reported); } else if sess.dcx().stashed_err_count() > 0 { // Without this case we sometimes get delayed bug ICEs and I don't diff --git a/compiler/rustc_metadata/src/creader.rs b/compiler/rustc_metadata/src/creader.rs index f6d3dba247090..baa9af87a83c9 100644 --- a/compiler/rustc_metadata/src/creader.rs +++ b/compiler/rustc_metadata/src/creader.rs @@ -926,7 +926,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { what: &str, needs_dep: &dyn Fn(&CrateMetadata) -> bool, ) { - // don't perform this validation if the session has errors, as one of + // Don't perform this validation if the session has errors, as one of // those errors may indicate a circular dependency which could cause // this to stack overflow. if self.dcx().has_errors().is_some() { diff --git a/compiler/rustc_query_system/src/dep_graph/graph.rs b/compiler/rustc_query_system/src/dep_graph/graph.rs index b6ac54a9ab59b..7dc1a1f791752 100644 --- a/compiler/rustc_query_system/src/dep_graph/graph.rs +++ b/compiler/rustc_query_system/src/dep_graph/graph.rs @@ -817,7 +817,7 @@ impl DepGraphData { None => {} } - if let None = qcx.dep_context().sess().dcx().has_errors_or_lint_errors_or_delayed_bugs() { + if let None = qcx.dep_context().sess().dcx().has_errors_or_delayed_bugs() { panic!("try_mark_previous_green() - Forcing the DepNode should have set its color") } diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 422bbadf6a8a0..f8a1d79659d95 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -313,8 +313,7 @@ impl Session { } pub fn compile_status(&self) -> Result<(), ErrorGuaranteed> { - // We must include lint errors here. - if let Some(reported) = self.dcx().has_errors_or_lint_errors() { + if let Some(reported) = self.dcx().has_errors() { self.dcx().emit_stashed_diagnostics(); Err(reported) } else { diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index efad5a8d808dd..3f7edd049a713 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -452,8 +452,7 @@ pub(crate) fn run_global_ctxt( tcx.sess.time("check_lint_expectations", || tcx.check_expectations(Some(sym::rustdoc))); - // We must include lint errors here. - if tcx.dcx().has_errors_or_lint_errors().is_some() { + if tcx.dcx().has_errors().is_some() { rustc_errors::FatalError.raise(); } diff --git a/src/librustdoc/doctest.rs b/src/librustdoc/doctest.rs index f9d4d1af1140f..09a90b62d97be 100644 --- a/src/librustdoc/doctest.rs +++ b/src/librustdoc/doctest.rs @@ -153,8 +153,7 @@ pub(crate) fn run(options: RustdocOptions) -> Result<(), ErrorGuaranteed> { collector }); - // We must include lint errors here. - if compiler.sess.dcx().has_errors_or_lint_errors().is_some() { + if compiler.sess.dcx().has_errors().is_some() { FatalError.raise(); } From 72b172bdf631483d0f802c54c8dc8246f6b4e00e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 10:00:19 +1100 Subject: [PATCH 17/21] Overhaul the handling of errors at the top-level. Currently `emit_stashed_diagnostic` is called from four(!) different places: `print_error_count`, `DiagCtxtInner::drop`, `abort_if_errors`, and `compile_status`. And `flush_delayed` is called from two different places: `DiagCtxtInner::drop` and `Queries`. This is pretty gross! Each one should really be called from a single place, but there's a bunch of entanglements. This commit cleans up this mess. Specifically, it: - Removes all the existing calls to `emit_stashed_diagnostic`, and adds a single new call in `finish_diagnostics`. - Removes the early `flush_delayed` call in `codegen_and_build_linker`, replacing it with a simple early return if delayed bugs are present. - Changes `DiagCtxtInner::drop` and `DiagCtxtInner::flush_delayed` so they both assert that the stashed diagnostics are empty (i.e. processed beforehand). - Changes `interface::run_compiler` so that any errors emitted during `finish_diagnostics` (i.e. late-emitted stashed diagnostics) are counted and cannot be overlooked. This requires adding `ErrorGuaranteed` return values to several functions. - Removes the `stashed_err_count` call in `analysis`. This is possible now that we don't have to worry about calling `flush_delayed` early from `codegen_and_build_linker` when stashed diagnostics are pending. - Changes the `span_bug` case in `handle_tuple_field_pattern_match` to a `delayed_span_bug`, because it now can be reached due to the removal of the `stashed_err_count` call in `analysis`. - Slightly changes the expected output of three tests. If no errors are emitted but there are delayed bugs, the error count is no longer printed. This is because delayed bugs are now always printed after the error count is printed (or not printed, if the error count is zero). There is a lot going on in this commit. It's hard to break into smaller pieces because the existing code is very tangled. It took me a long time and a lot of effort to understand how the different pieces interact, and I think the new code is a lot simpler and easier to understand. --- compiler/rustc_errors/src/lib.rs | 32 ++++++++++------ compiler/rustc_interface/src/interface.rs | 37 ++++++++++++++++--- compiler/rustc_interface/src/passes.rs | 11 +++--- compiler/rustc_interface/src/queries.rs | 12 +++--- compiler/rustc_passes/src/dead.rs | 5 ++- compiler/rustc_session/src/session.rs | 14 ++++--- .../equality-in-canonical-query.clone.stderr | 2 - .../ui/inference/issue-80409.no-compat.stderr | 2 - ...equality_in_canonical_query.current.stderr | 2 - 9 files changed, 76 insertions(+), 41 deletions(-) diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index a6b0a0e8f1751..fafd636bb7091 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -471,9 +471,10 @@ struct DiagCtxtInner { emitted_diagnostics: FxHashSet, /// Stashed diagnostics emitted in one stage of the compiler that may be - /// stolen by other stages (e.g. to improve them and add more information). - /// The stashed diagnostics count towards the total error count. - /// When `.abort_if_errors()` is called, these are also emitted. + /// stolen and emitted/cancelled by other stages (e.g. to improve them and + /// add more information). All stashed diagnostics must be emitted with + /// `emit_stashed_diagnostics` by the time the `DiagCtxtInner` is dropped, + /// otherwise an assertion failure will occur. stashed_diagnostics: FxIndexMap<(Span, StashKey), Diagnostic>, future_breakage_diagnostics: Vec, @@ -558,7 +559,9 @@ pub struct DiagCtxtFlags { impl Drop for DiagCtxtInner { fn drop(&mut self) { - self.emit_stashed_diagnostics(); + // Any stashed diagnostics should have been handled by + // `emit_stashed_diagnostics` by now. + assert!(self.stashed_diagnostics.is_empty()); if self.err_guars.is_empty() { self.flush_delayed() @@ -750,7 +753,7 @@ impl DiagCtxt { } /// Emit all stashed diagnostics. - pub fn emit_stashed_diagnostics(&self) { + pub fn emit_stashed_diagnostics(&self) -> Option { self.inner.borrow_mut().emit_stashed_diagnostics() } @@ -796,7 +799,9 @@ impl DiagCtxt { pub fn print_error_count(&self, registry: &Registry) { let mut inner = self.inner.borrow_mut(); - inner.emit_stashed_diagnostics(); + // Any stashed diagnostics should have been handled by + // `emit_stashed_diagnostics` by now. + assert!(inner.stashed_diagnostics.is_empty()); if inner.treat_err_as_bug() { return; @@ -872,9 +877,7 @@ impl DiagCtxt { } pub fn abort_if_errors(&self) { - let mut inner = self.inner.borrow_mut(); - inner.emit_stashed_diagnostics(); - if !inner.err_guars.is_empty() { + if self.has_errors().is_some() { FatalError.raise(); } } @@ -1275,7 +1278,8 @@ impl DiagCtxt { // `DiagCtxtInner::foo`. impl DiagCtxtInner { /// Emit all stashed diagnostics. - fn emit_stashed_diagnostics(&mut self) { + fn emit_stashed_diagnostics(&mut self) -> Option { + let mut guar = None; let has_errors = !self.err_guars.is_empty(); for (_, diag) in std::mem::take(&mut self.stashed_diagnostics).into_iter() { if diag.is_error() { @@ -1290,8 +1294,9 @@ impl DiagCtxtInner { continue; } } - self.emit_diagnostic(diag); + guar = guar.or(self.emit_diagnostic(diag)); } + guar } // Return value is only `Some` if the level is `Error` or `DelayedBug`. @@ -1493,6 +1498,11 @@ impl DiagCtxtInner { } fn flush_delayed(&mut self) { + // Stashed diagnostics must be emitted before delayed bugs are flushed. + // Otherwise, we might ICE prematurely when errors would have + // eventually happened. + assert!(self.stashed_diagnostics.is_empty()); + if self.delayed_bugs.is_empty() { return; } diff --git a/compiler/rustc_interface/src/interface.rs b/compiler/rustc_interface/src/interface.rs index 8a4705e0056e1..cd7957c3bce29 100644 --- a/compiler/rustc_interface/src/interface.rs +++ b/compiler/rustc_interface/src/interface.rs @@ -423,18 +423,43 @@ pub fn run_compiler(config: Config, f: impl FnOnce(&Compiler) -> R + Se Compiler { sess, codegen_backend, override_queries: config.override_queries }; rustc_span::set_source_map(compiler.sess.parse_sess.clone_source_map(), move || { - let r = { - let _sess_abort_error = defer(|| { - compiler.sess.finish_diagnostics(&config.registry); + // There are two paths out of `f`. + // - Normal exit. + // - Panic, e.g. triggered by `abort_if_errors`. + // + // We must run `finish_diagnostics` in both cases. + let res = { + // If `f` panics, `finish_diagnostics` will run during + // unwinding because of the `defer`. + let mut guar = None; + let sess_abort_guard = defer(|| { + guar = compiler.sess.finish_diagnostics(&config.registry); }); - f(&compiler) + let res = f(&compiler); + + // If `f` doesn't panic, `finish_diagnostics` will run + // normally when `sess_abort_guard` is dropped. + drop(sess_abort_guard); + + // If `finish_diagnostics` emits errors (e.g. stashed + // errors) we can't return an error directly, because the + // return type of this function is `R`, not `Result`. + // But we need to communicate the errors' existence to the + // caller, otherwise the caller might mistakenly think that + // no errors occurred and return a zero exit code. So we + // abort (panic) instead, similar to if `f` had panicked. + if guar.is_some() { + compiler.sess.dcx().abort_if_errors(); + } + + res }; let prof = compiler.sess.prof.clone(); - prof.generic_activity("drop_compiler").run(move || drop(compiler)); - r + + res }) }, ) diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index 858db594b4773..d35c2be1fb47d 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -772,12 +772,11 @@ fn analysis(tcx: TyCtxt<'_>, (): ()) -> Result<()> { // lot of annoying errors in the ui tests (basically, // lint warnings and so on -- kindck used to do this abort, but // kindck is gone now). -nmatsakis - if let Some(reported) = sess.dcx().has_errors_excluding_lint_errors() { - return Err(reported); - } else if sess.dcx().stashed_err_count() > 0 { - // Without this case we sometimes get delayed bug ICEs and I don't - // understand why. -nnethercote - return Err(sess.dcx().delayed_bug("some stashed error is waiting for use")); + // + // But we exclude lint errors from this, because lint errors are typically + // less serious and we're more likely to want to continue (#87337). + if let Some(guar) = sess.dcx().has_errors_excluding_lint_errors() { + return Err(guar); } sess.time("misc_checking_3", || { diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index 211bcb9da94db..8170c0a5a1a9f 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -222,12 +222,12 @@ impl<'tcx> Queries<'tcx> { pub fn codegen_and_build_linker(&'tcx self) -> Result { self.global_ctxt()?.enter(|tcx| { - // Don't do code generation if there were any errors - self.compiler.sess.compile_status()?; - - // If we have any delayed bugs, for example because we created TyKind::Error earlier, - // it's likely that codegen will only cause more ICEs, obscuring the original problem - self.compiler.sess.dcx().flush_delayed(); + // Don't do code generation if there were any errors. Likewise if + // there were any delayed bugs, because codegen will likely cause + // more ICEs, obscuring the original problem. + if let Some(guar) = self.compiler.sess.dcx().has_errors_or_delayed_bugs() { + return Err(guar); + } // Hook for UI tests. Self::check_for_rustc_errors_attr(tcx); diff --git a/compiler/rustc_passes/src/dead.rs b/compiler/rustc_passes/src/dead.rs index 486396b067726..a3106856a6701 100644 --- a/compiler/rustc_passes/src/dead.rs +++ b/compiler/rustc_passes/src/dead.rs @@ -237,7 +237,10 @@ impl<'tcx> MarkSymbolVisitor<'tcx> { ) { let variant = match self.typeck_results().node_type(lhs.hir_id).kind() { ty::Adt(adt, _) => adt.variant_of_res(res), - _ => span_bug!(lhs.span, "non-ADT in tuple struct pattern"), + _ => { + self.tcx.dcx().span_delayed_bug(lhs.span, "non-ADT in tuple struct pattern"); + return; + } }; let dotdot = dotdot.as_opt_usize().unwrap_or(pats.len()); let first_n = pats.iter().enumerate().take(dotdot); diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index f8a1d79659d95..48a18fca27ed2 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -258,7 +258,8 @@ impl Session { } } - fn check_miri_unleashed_features(&self) { + fn check_miri_unleashed_features(&self) -> Option { + let mut guar = None; let unleashed_features = self.miri_unleashed_features.lock(); if !unleashed_features.is_empty() { let mut must_err = false; @@ -279,18 +280,22 @@ impl Session { // If we should err, make sure we did. if must_err && self.dcx().has_errors().is_none() { // We have skipped a feature gate, and not run into other errors... reject. - self.dcx().emit_err(errors::NotCircumventFeature); + guar = Some(self.dcx().emit_err(errors::NotCircumventFeature)); } } + guar } /// Invoked all the way at the end to finish off diagnostics printing. - pub fn finish_diagnostics(&self, registry: &Registry) { - self.check_miri_unleashed_features(); + pub fn finish_diagnostics(&self, registry: &Registry) -> Option { + let mut guar = None; + guar = guar.or(self.check_miri_unleashed_features()); + guar = guar.or(self.dcx().emit_stashed_diagnostics()); self.dcx().print_error_count(registry); if self.opts.json_future_incompat { self.dcx().emit_future_breakage_report(); } + guar } /// Returns true if the crate is a testing one. @@ -314,7 +319,6 @@ impl Session { pub fn compile_status(&self) -> Result<(), ErrorGuaranteed> { if let Some(reported) = self.dcx().has_errors() { - self.dcx().emit_stashed_diagnostics(); Err(reported) } else { Ok(()) diff --git a/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr b/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr index 0e3cd2ff06099..e4c8aec397365 100644 --- a/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr +++ b/tests/ui/impl-trait/equality-in-canonical-query.clone.stderr @@ -21,5 +21,3 @@ LL | same_output(foo, rpit); query stack during panic: end of query stack -error: aborting due to 2 previous errors - diff --git a/tests/ui/inference/issue-80409.no-compat.stderr b/tests/ui/inference/issue-80409.no-compat.stderr index f9772b2d5a699..523ca229b06f4 100644 --- a/tests/ui/inference/issue-80409.no-compat.stderr +++ b/tests/ui/inference/issue-80409.no-compat.stderr @@ -12,5 +12,3 @@ LL | builder.state().on_entry(|_| {}); query stack during panic: end of query stack -error: aborting due to 1 previous error - diff --git a/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr b/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr index fd76526644bdd..069292239bc89 100644 --- a/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr +++ b/tests/ui/type-alias-impl-trait/rpit_tait_equality_in_canonical_query.current.stderr @@ -21,5 +21,3 @@ LL | query(get_rpit); query stack during panic: end of query stack -error: aborting due to 2 previous errors - From c2512a130f398d923229c3dc401be10c357a3b8d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 10:13:51 +1100 Subject: [PATCH 18/21] Inline and remove `Session::compile_status`. Because it's now simple enough that it doesn't provide much benefit. --- compiler/rustc_codegen_ssa/src/back/link.rs | 4 +++- compiler/rustc_driver_impl/src/lib.rs | 17 ++++++++++++----- compiler/rustc_interface/src/queries.rs | 4 +++- compiler/rustc_session/src/session.rs | 8 -------- src/tools/miri/src/bin/miri.rs | 2 +- 5 files changed, 19 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index 435b517e602b1..7e3f324fe14c2 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -487,7 +487,9 @@ fn collate_raw_dylibs<'a, 'b>( } } } - sess.compile_status()?; + if let Some(guar) = sess.dcx().has_errors() { + return Err(guar); + } Ok(dylib_table .into_iter() .map(|(name, imports)| { diff --git a/compiler/rustc_driver_impl/src/lib.rs b/compiler/rustc_driver_impl/src/lib.rs index 9b2d760282f1c..1057235c0a2a8 100644 --- a/compiler/rustc_driver_impl/src/lib.rs +++ b/compiler/rustc_driver_impl/src/lib.rs @@ -357,18 +357,25 @@ fn run_compiler( let sess = &compiler.sess; let codegen_backend = &*compiler.codegen_backend; + // This is used for early exits unrelated to errors. E.g. when just + // printing some information without compiling, or exiting immediately + // after parsing, etc. + let early_exit = || { + if let Some(guar) = sess.dcx().has_errors() { Err(guar) } else { Ok(()) } + }; + // This implements `-Whelp`. It should be handled very early, like // `--help`/`-Zhelp`/`-Chelp`. This is the earliest it can run, because // it must happen after lints are registered, during session creation. if sess.opts.describe_lints { describe_lints(sess); - return sess.compile_status(); + return early_exit(); } let early_dcx = EarlyDiagCtxt::new(sess.opts.error_format); if print_crate_info(&early_dcx, codegen_backend, sess, has_input) == Compilation::Stop { - return sess.compile_status(); + return early_exit(); } if !has_input { @@ -377,16 +384,16 @@ fn run_compiler( if !sess.opts.unstable_opts.ls.is_empty() { list_metadata(&early_dcx, sess, &*codegen_backend.metadata_loader()); - return sess.compile_status(); + return early_exit(); } if sess.opts.unstable_opts.link_only { process_rlink(sess, compiler); - return sess.compile_status(); + return early_exit(); } let linker = compiler.enter(|queries| { - let early_exit = || sess.compile_status().map(|_| None); + let early_exit = || early_exit().map(|_| None); queries.parse()?; if let Some(ppm) = &sess.opts.pretty { diff --git a/compiler/rustc_interface/src/queries.rs b/compiler/rustc_interface/src/queries.rs index 8170c0a5a1a9f..86858bfe41d81 100644 --- a/compiler/rustc_interface/src/queries.rs +++ b/compiler/rustc_interface/src/queries.rs @@ -261,7 +261,9 @@ impl Linker { let (codegen_results, work_products) = codegen_backend.join_codegen(self.ongoing_codegen, sess, &self.output_filenames); - sess.compile_status()?; + if let Some(guar) = sess.dcx().has_errors() { + return Err(guar); + } sess.time("serialize_work_products", || { rustc_incremental::save_work_product_index(sess, &self.dep_graph, work_products) diff --git a/compiler/rustc_session/src/session.rs b/compiler/rustc_session/src/session.rs index 48a18fca27ed2..02c7a0c6371f9 100644 --- a/compiler/rustc_session/src/session.rs +++ b/compiler/rustc_session/src/session.rs @@ -317,14 +317,6 @@ impl Session { err } - pub fn compile_status(&self) -> Result<(), ErrorGuaranteed> { - if let Some(reported) = self.dcx().has_errors() { - Err(reported) - } else { - Ok(()) - } - } - /// Record the fact that we called `trimmed_def_paths`, and do some /// checking about whether its cost was justified. pub fn record_trimmed_def_paths(&self) { diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index db4c4a28debb4..8a7133fea438d 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -68,7 +68,7 @@ impl rustc_driver::Callbacks for MiriCompilerCalls { queries: &'tcx rustc_interface::Queries<'tcx>, ) -> Compilation { queries.global_ctxt().unwrap().enter(|tcx| { - if tcx.sess.compile_status().is_err() { + if tcx.sess.dcx().has_errors().is_some() { tcx.dcx().fatal("miri cannot be run on programs that fail compilation"); } From 44006444c8c85a0102984a4323755da0084a681e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 10:27:23 +1100 Subject: [PATCH 19/21] Refactor `run_global_ctxt`. It currently is infallible and uses `abort_if_errors` and `FatalError.raise()` to signal errors. It's easy to instead return a `Result<_, ErrorGuaranteed>`, which is the more usual way of doing things. --- src/librustdoc/core.rs | 15 +++++++++------ src/librustdoc/lib.rs | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/librustdoc/core.rs b/src/librustdoc/core.rs index 3f7edd049a713..c662f054d0524 100644 --- a/src/librustdoc/core.rs +++ b/src/librustdoc/core.rs @@ -3,7 +3,7 @@ use rustc_data_structures::sync::Lrc; use rustc_data_structures::unord::UnordSet; use rustc_errors::emitter::{DynEmitter, HumanEmitter}; use rustc_errors::json::JsonEmitter; -use rustc_errors::{codes::*, TerminalUrl}; +use rustc_errors::{codes::*, ErrorGuaranteed, TerminalUrl}; use rustc_feature::UnstableFeatures; use rustc_hir::def::Res; use rustc_hir::def_id::{DefId, DefIdMap, DefIdSet, LocalDefId}; @@ -306,7 +306,7 @@ pub(crate) fn run_global_ctxt( show_coverage: bool, render_options: RenderOptions, output_format: OutputFormat, -) -> (clean::Crate, RenderOptions, Cache) { +) -> Result<(clean::Crate, RenderOptions, Cache), ErrorGuaranteed> { // Certain queries assume that some checks were run elsewhere // (see https://github.com/rust-lang/rust/pull/73566#issuecomment-656954425), // so type-check everything other than function bodies in this crate before running lints. @@ -331,7 +331,10 @@ pub(crate) fn run_global_ctxt( }); }); - tcx.dcx().abort_if_errors(); + if let Some(guar) = tcx.dcx().has_errors() { + return Err(guar); + } + tcx.sess.time("missing_docs", || rustc_lint::check_crate(tcx)); tcx.sess.time("check_mod_attrs", || { tcx.hir().for_each_module(|module| tcx.ensure().check_mod_attrs(module)) @@ -452,13 +455,13 @@ pub(crate) fn run_global_ctxt( tcx.sess.time("check_lint_expectations", || tcx.check_expectations(Some(sym::rustdoc))); - if tcx.dcx().has_errors().is_some() { - rustc_errors::FatalError.raise(); + if let Some(guar) = tcx.dcx().has_errors() { + return Err(guar); } krate = tcx.sess.time("create_format_cache", || Cache::populate(&mut ctxt, krate)); - (krate, ctxt.render_options, ctxt.cache) + Ok((krate, ctxt.render_options, ctxt.cache)) } /// Due to , diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index 18ea49c5baf1b..f88381ad788d6 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -787,7 +787,7 @@ fn main_args( gcx.enter(|tcx| { let (krate, render_opts, mut cache) = sess.time("run_global_ctxt", || { core::run_global_ctxt(tcx, show_coverage, render_options, output_format) - }); + })?; info!("finished with rustc"); if let Some(options) = scrape_examples_options { From 4da67fff61ccc460370df6047563f8091c7e66bd Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 10:23:58 +1100 Subject: [PATCH 20/21] Replace unnecessary `abort_if_errors`. Replace `abort_if_errors` calls that are certain to abort -- because we emit an error immediately beforehand -- with `FatalErro.raise()`. --- compiler/rustc_codegen_ssa/src/back/link.rs | 9 +++------ compiler/rustc_codegen_ssa/src/base.rs | 5 +---- compiler/rustc_errors/src/lib.rs | 4 ++++ compiler/rustc_interface/src/passes.rs | 4 +--- compiler/rustc_session/src/output.rs | 3 ++- .../src/traits/error_reporting/type_err_ctxt_ext.rs | 11 ++--------- 6 files changed, 13 insertions(+), 23 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/link.rs b/compiler/rustc_codegen_ssa/src/back/link.rs index 7e3f324fe14c2..1ad0dec064006 100644 --- a/compiler/rustc_codegen_ssa/src/back/link.rs +++ b/compiler/rustc_codegen_ssa/src/back/link.rs @@ -3,7 +3,7 @@ use rustc_ast::CRATE_NODE_ID; use rustc_data_structures::fx::{FxIndexMap, FxIndexSet}; use rustc_data_structures::memmap::Mmap; use rustc_data_structures::temp_dir::MaybeTempDir; -use rustc_errors::{DiagCtxt, ErrorGuaranteed}; +use rustc_errors::{DiagCtxt, ErrorGuaranteed, FatalError}; use rustc_fs_util::{fix_windows_verbatim_for_gcc, try_canonicalize}; use rustc_hir::def_id::{CrateNum, LOCAL_CRATE}; use rustc_metadata::find_native_static_library; @@ -722,10 +722,7 @@ fn link_dwarf_object<'a>( Ok(()) }) { Ok(()) => {} - Err(e) => { - sess.dcx().emit_err(errors::ThorinErrorWrapper(e)); - sess.dcx().abort_if_errors(); - } + Err(e) => sess.dcx().emit_fatal(errors::ThorinErrorWrapper(e)), } } @@ -1001,7 +998,7 @@ fn link_natively<'a>( sess.dcx().emit_note(errors::CheckInstalledVisualStudio); sess.dcx().emit_note(errors::InsufficientVSCodeProduct); } - sess.dcx().abort_if_errors(); + FatalError.raise(); } } diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 760b3f30ee51e..f7afd22a48cab 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -449,10 +449,7 @@ pub fn maybe_create_entry_wrapper<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let Some(llfn) = cx.declare_c_main(llfty) else { // FIXME: We should be smart and show a better diagnostic here. let span = cx.tcx().def_span(rust_main_def_id); - let dcx = cx.tcx().dcx(); - dcx.emit_err(errors::MultipleMainFunctions { span }); - dcx.abort_if_errors(); - bug!(); + cx.tcx().dcx().emit_fatal(errors::MultipleMainFunctions { span }); }; // `main` should respect same config for frame pointer elimination as rest of code diff --git a/compiler/rustc_errors/src/lib.rs b/compiler/rustc_errors/src/lib.rs index fafd636bb7091..7e3d15ffc921f 100644 --- a/compiler/rustc_errors/src/lib.rs +++ b/compiler/rustc_errors/src/lib.rs @@ -876,6 +876,10 @@ impl DiagCtxt { } } + /// This excludes delayed bugs and stashed errors. Used for early aborts + /// after errors occurred -- e.g. because continuing in the face of errors is + /// likely to lead to bad results, such as spurious/uninteresting + /// additional errors -- when returning an error `Result` is difficult. pub fn abort_if_errors(&self) { if self.has_errors().is_some() { FatalError.raise(); diff --git a/compiler/rustc_interface/src/passes.rs b/compiler/rustc_interface/src/passes.rs index d35c2be1fb47d..661401687593d 100644 --- a/compiler/rustc_interface/src/passes.rs +++ b/compiler/rustc_interface/src/passes.rs @@ -936,9 +936,7 @@ pub fn start_codegen<'tcx>( if tcx.sess.opts.output_types.contains_key(&OutputType::Mir) { if let Err(error) = rustc_mir_transform::dump_mir::emit_mir(tcx) { - let dcx = tcx.dcx(); - dcx.emit_err(errors::CantEmitMIR { error }); - dcx.abort_if_errors(); + tcx.dcx().emit_fatal(errors::CantEmitMIR { error }); } } diff --git a/compiler/rustc_session/src/output.rs b/compiler/rustc_session/src/output.rs index db976b3040487..74d26237f2463 100644 --- a/compiler/rustc_session/src/output.rs +++ b/compiler/rustc_session/src/output.rs @@ -6,6 +6,7 @@ use crate::errors::{ }; use crate::Session; use rustc_ast::{self as ast, attr}; +use rustc_errors::FatalError; use rustc_span::symbol::sym; use rustc_span::{Span, Symbol}; use std::path::Path; @@ -115,7 +116,7 @@ pub fn validate_crate_name(sess: &Session, s: Symbol, sp: Option) { } if err_count > 0 { - sess.dcx().abort_if_errors(); + FatalError.raise(); } } diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs index aa8bd5fdc866b..7186b96b40d04 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs @@ -22,7 +22,7 @@ use crate::traits::{ use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; use rustc_errors::{ codes::*, pluralize, struct_span_code_err, Applicability, DiagnosticBuilder, ErrorGuaranteed, - MultiSpan, StashKey, StringPart, + FatalError, MultiSpan, StashKey, StringPart, }; use rustc_hir as hir; use rustc_hir::def::{DefKind, Namespace, Res}; @@ -193,14 +193,7 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> { let mut err = self.build_overflow_error(predicate, span, suggest_increasing_limit); mutate(&mut err); err.emit(); - - self.dcx().abort_if_errors(); - // FIXME: this should be something like `build_overflow_error_fatal`, which returns - // `DiagnosticBuilder<', !>`. Then we don't even need anything after that `emit()`. - unreachable!( - "did not expect compilation to continue after `abort_if_errors`, \ - since an error was definitely emitted!" - ); + FatalError.raise(); } fn build_overflow_error( From f16c226af3ad4d21021433745214c71e95f10236 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 19 Feb 2024 13:51:53 +1100 Subject: [PATCH 21/21] Inline and remove `abort_on_err`. It's clumsy and doesn't improve readability. --- compiler/rustc_driver_impl/src/lib.rs | 17 ++++------------- compiler/rustc_driver_impl/src/pretty.rs | 14 ++++++++++---- src/librustdoc/lib.rs | 5 ++--- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_driver_impl/src/lib.rs b/compiler/rustc_driver_impl/src/lib.rs index 1057235c0a2a8..692c059beb0c4 100644 --- a/compiler/rustc_driver_impl/src/lib.rs +++ b/compiler/rustc_driver_impl/src/lib.rs @@ -144,16 +144,6 @@ pub const EXIT_FAILURE: i32 = 1; pub const DEFAULT_BUG_REPORT_URL: &str = "https://github.com/rust-lang/rust/issues/new\ ?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md"; -pub fn abort_on_err(result: Result, sess: &Session) -> T { - match result { - Err(..) => { - sess.dcx().abort_if_errors(); - panic!("error reported but abort_if_errors didn't abort???"); - } - Ok(x) => x, - } -} - pub trait Callbacks { /// Called before creating the compiler instance fn config(&mut self, _config: &mut interface::Config) {} @@ -665,10 +655,11 @@ fn process_rlink(sess: &Session, compiler: &interface::Compiler) { }; } }; - let result = compiler.codegen_backend.link(sess, codegen_results, &outputs); - abort_on_err(result, sess); + if compiler.codegen_backend.link(sess, codegen_results, &outputs).is_err() { + FatalError.raise(); + } } else { - dcx.emit_fatal(RlinkNotAFile {}) + dcx.emit_fatal(RlinkNotAFile {}); } } diff --git a/compiler/rustc_driver_impl/src/pretty.rs b/compiler/rustc_driver_impl/src/pretty.rs index e5a7d5501151a..ff5ffd2454a1f 100644 --- a/compiler/rustc_driver_impl/src/pretty.rs +++ b/compiler/rustc_driver_impl/src/pretty.rs @@ -2,6 +2,7 @@ use rustc_ast as ast; use rustc_ast_pretty::pprust as pprust_ast; +use rustc_errors::FatalError; use rustc_hir as hir; use rustc_hir_pretty as pprust_hir; use rustc_middle::bug; @@ -18,7 +19,6 @@ use std::fmt::Write; pub use self::PpMode::*; pub use self::PpSourceMode::*; -use crate::abort_on_err; struct AstNoAnn; @@ -243,7 +243,9 @@ impl<'tcx> PrintExtra<'tcx> { pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) { if ppm.needs_analysis() { - abort_on_err(ex.tcx().analysis(()), sess); + if ex.tcx().analysis(()).is_err() { + FatalError.raise(); + } } let (src, src_name) = get_source(sess); @@ -334,7 +336,9 @@ pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) { ThirTree => { let tcx = ex.tcx(); let mut out = String::new(); - abort_on_err(rustc_hir_analysis::check_crate(tcx), tcx.sess); + if rustc_hir_analysis::check_crate(tcx).is_err() { + FatalError.raise(); + } debug!("pretty printing THIR tree"); for did in tcx.hir().body_owners() { let _ = writeln!(out, "{:?}:\n{}\n", did, tcx.thir_tree(did)); @@ -344,7 +348,9 @@ pub fn print<'tcx>(sess: &Session, ppm: PpMode, ex: PrintExtra<'tcx>) { ThirFlat => { let tcx = ex.tcx(); let mut out = String::new(); - abort_on_err(rustc_hir_analysis::check_crate(tcx), tcx.sess); + if rustc_hir_analysis::check_crate(tcx).is_err() { + FatalError.raise(); + } debug!("pretty printing THIR flat"); for did in tcx.hir().body_owners() { let _ = writeln!(out, "{:?}:\n{}\n", did, tcx.thir_flat(did)); diff --git a/src/librustdoc/lib.rs b/src/librustdoc/lib.rs index f88381ad788d6..33837fe5652ce 100644 --- a/src/librustdoc/lib.rs +++ b/src/librustdoc/lib.rs @@ -78,8 +78,7 @@ use std::io::{self, IsTerminal}; use std::process; use std::sync::{atomic::AtomicBool, Arc}; -use rustc_driver::abort_on_err; -use rustc_errors::ErrorGuaranteed; +use rustc_errors::{ErrorGuaranteed, FatalError}; use rustc_interface::interface; use rustc_middle::ty::TyCtxt; use rustc_session::config::{make_crate_type_option, ErrorOutputType, RustcOptGroup}; @@ -779,7 +778,7 @@ fn main_args( } compiler.enter(|queries| { - let mut gcx = abort_on_err(queries.global_ctxt(), sess); + let Ok(mut gcx) = queries.global_ctxt() else { FatalError.raise() }; if sess.dcx().has_errors().is_some() { sess.dcx().fatal("Compilation failed, aborting rustdoc"); }