From 9261fd0983daf700525861358deabbdcc0878709 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 27 Feb 2023 20:47:06 +0000 Subject: [PATCH 01/26] Nested impl traits trigger opaque_hidden_inferred_bound too much --- .../src/opaque_hidden_inferred_bound.rs | 23 +++++++++++++++---- tests/ui/impl-trait/nested-return-type2.rs | 1 - .../ui/impl-trait/nested-return-type2.stderr | 17 -------------- tests/ui/impl-trait/nested-return-type3.rs | 1 - .../ui/impl-trait/nested-return-type3.stderr | 17 -------------- 5 files changed, 19 insertions(+), 40 deletions(-) delete mode 100644 tests/ui/impl-trait/nested-return-type2.stderr delete mode 100644 tests/ui/impl-trait/nested-return-type3.stderr diff --git a/compiler/rustc_lint/src/opaque_hidden_inferred_bound.rs b/compiler/rustc_lint/src/opaque_hidden_inferred_bound.rs index 883a56cb3ce6b..bff8fc236544f 100644 --- a/compiler/rustc_lint/src/opaque_hidden_inferred_bound.rs +++ b/compiler/rustc_lint/src/opaque_hidden_inferred_bound.rs @@ -2,7 +2,7 @@ use rustc_hir as hir; use rustc_infer::infer::TyCtxtInferExt; use rustc_macros::{LintDiagnostic, Subdiagnostic}; use rustc_middle::ty::{ - self, fold::BottomUpFolder, print::TraitPredPrintModifiersAndPath, Ty, TypeFoldable, + self, fold::BottomUpFolder, print::TraitPredPrintModifiersAndPath, DefIdTree, Ty, TypeFoldable, }; use rustc_span::Span; use rustc_trait_selection::traits; @@ -27,6 +27,8 @@ declare_lint! { /// ### Example /// /// ```rust + /// #![feature(type_alias_impl_trait)] + /// /// trait Duh {} /// /// impl Duh for i32 {} @@ -41,7 +43,9 @@ declare_lint! { /// type Assoc = F; /// } /// - /// fn test() -> impl Trait { + /// type Tait = impl Sized; + /// + /// fn test() -> impl Trait { /// 42 /// } /// ``` @@ -54,7 +58,7 @@ declare_lint! { /// /// Although the hidden type, `i32` does satisfy this bound, we do not /// consider the return type to be well-formed with this lint. It can be - /// fixed by changing `impl Sized` into `impl Sized + Send`. + /// fixed by changing `Tait = impl Sized` into `Tait = impl Sized + Send`. pub OPAQUE_HIDDEN_INFERRED_BOUND, Warn, "detects the use of nested `impl Trait` types in associated type bounds that are not general enough" @@ -64,7 +68,7 @@ declare_lint_pass!(OpaqueHiddenInferredBound => [OPAQUE_HIDDEN_INFERRED_BOUND]); impl<'tcx> LateLintPass<'tcx> for OpaqueHiddenInferredBound { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'tcx>) { - let hir::ItemKind::OpaqueTy(_) = &item.kind else { return; }; + let hir::ItemKind::OpaqueTy(opaque) = &item.kind else { return; }; let def_id = item.owner_id.def_id.to_def_id(); let infcx = &cx.tcx.infer_ctxt().build(); // For every projection predicate in the opaque type's explicit bounds, @@ -81,6 +85,17 @@ impl<'tcx> LateLintPass<'tcx> for OpaqueHiddenInferredBound { // have opaques in them anyways. let Some(proj_term) = proj.term.ty() else { continue }; + // HACK: `impl Trait` from an RPIT is "ok"... + if let ty::Alias(ty::Opaque, opaque_ty) = *proj_term.kind() + && cx.tcx.parent(opaque_ty.def_id) == def_id + && matches!( + opaque.origin, + hir::OpaqueTyOrigin::FnReturn(_) | hir::OpaqueTyOrigin::AsyncFn(_) + ) + { + continue; + } + let proj_ty = cx.tcx.mk_projection(proj.projection_ty.def_id, proj.projection_ty.substs); // For every instance of the projection type in the bounds, diff --git a/tests/ui/impl-trait/nested-return-type2.rs b/tests/ui/impl-trait/nested-return-type2.rs index fe883ce6fc8ed..e1d5511379e7a 100644 --- a/tests/ui/impl-trait/nested-return-type2.rs +++ b/tests/ui/impl-trait/nested-return-type2.rs @@ -26,7 +26,6 @@ impl R> Trait for F { // Lazy TAIT would error out, but we inserted a hack to make it work again, // keeping backwards compatibility. fn foo() -> impl Trait { - //~^ WARN opaque type `impl Trait` does not satisfy its associated type bounds || 42 } diff --git a/tests/ui/impl-trait/nested-return-type2.stderr b/tests/ui/impl-trait/nested-return-type2.stderr deleted file mode 100644 index 09ad3bd05c1b3..0000000000000 --- a/tests/ui/impl-trait/nested-return-type2.stderr +++ /dev/null @@ -1,17 +0,0 @@ -warning: opaque type `impl Trait` does not satisfy its associated type bounds - --> $DIR/nested-return-type2.rs:28:24 - | -LL | type Assoc: Duh; - | --- this associated type bound is unsatisfied for `impl Send` -... -LL | fn foo() -> impl Trait { - | ^^^^^^^^^^^^^^^^^ - | - = note: `#[warn(opaque_hidden_inferred_bound)]` on by default -help: add this bound - | -LL | fn foo() -> impl Trait { - | +++++ - -warning: 1 warning emitted - diff --git a/tests/ui/impl-trait/nested-return-type3.rs b/tests/ui/impl-trait/nested-return-type3.rs index 5a764fc4c285a..74b4dae22ebfd 100644 --- a/tests/ui/impl-trait/nested-return-type3.rs +++ b/tests/ui/impl-trait/nested-return-type3.rs @@ -13,7 +13,6 @@ impl Trait for F { } fn foo() -> impl Trait { - //~^ WARN opaque type `impl Trait` does not satisfy its associated type bounds 42 } diff --git a/tests/ui/impl-trait/nested-return-type3.stderr b/tests/ui/impl-trait/nested-return-type3.stderr deleted file mode 100644 index 632de71aa4c88..0000000000000 --- a/tests/ui/impl-trait/nested-return-type3.stderr +++ /dev/null @@ -1,17 +0,0 @@ -warning: opaque type `impl Trait` does not satisfy its associated type bounds - --> $DIR/nested-return-type3.rs:15:24 - | -LL | type Assoc: Duh; - | --- this associated type bound is unsatisfied for `impl Send` -... -LL | fn foo() -> impl Trait { - | ^^^^^^^^^^^^^^^^^ - | - = note: `#[warn(opaque_hidden_inferred_bound)]` on by default -help: add this bound - | -LL | fn foo() -> impl Trait { - | +++++ - -warning: 1 warning emitted - From d7049cabd0a370ac5af8b4f03ce836b3f9384d98 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Wed, 1 Mar 2023 14:47:55 +0100 Subject: [PATCH 02/26] add the --json flag to compiletest --- src/tools/compiletest/src/common.rs | 4 ++-- src/tools/compiletest/src/main.rs | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/tools/compiletest/src/common.rs b/src/tools/compiletest/src/common.rs index 7fe2e6257d9e7..7691f5c32b21e 100644 --- a/src/tools/compiletest/src/common.rs +++ b/src/tools/compiletest/src/common.rs @@ -9,7 +9,7 @@ use std::str::FromStr; use crate::util::{add_dylib_path, PathBufExt}; use lazycell::LazyCell; -use test::ColorConfig; +use test::{ColorConfig, OutputFormat}; #[derive(Clone, Copy, PartialEq, Debug)] pub enum Mode { @@ -337,7 +337,7 @@ pub struct Config { pub verbose: bool, /// Print one character per test instead of one line - pub quiet: bool, + pub format: OutputFormat, /// Whether to use colors in test. pub color: ColorConfig, diff --git a/src/tools/compiletest/src/main.rs b/src/tools/compiletest/src/main.rs index 1760c29ec66b7..167cd5c5dc170 100644 --- a/src/tools/compiletest/src/main.rs +++ b/src/tools/compiletest/src/main.rs @@ -114,6 +114,7 @@ pub fn parse_config(args: Vec) -> Config { ) .optflag("", "quiet", "print one character per test instead of one line") .optopt("", "color", "coloring: auto, always, never", "WHEN") + .optflag("", "json", "emit json output instead of plaintext output") .optopt("", "logfile", "file to log test execution to", "FILE") .optopt("", "target", "the target to build for", "TARGET") .optopt("", "host", "the host to build for", "HOST") @@ -281,7 +282,12 @@ pub fn parse_config(args: Vec) -> Config { && !opt_str2(matches.opt_str("adb-test-dir")).is_empty(), lldb_python_dir: matches.opt_str("lldb-python-dir"), verbose: matches.opt_present("verbose"), - quiet: matches.opt_present("quiet"), + format: match (matches.opt_present("quiet"), matches.opt_present("json")) { + (true, true) => panic!("--quiet and --json are incompatible"), + (true, false) => test::OutputFormat::Terse, + (false, true) => test::OutputFormat::Json, + (false, false) => test::OutputFormat::Pretty, + }, only_modified: matches.opt_present("only-modified"), color, remote_test_client: matches.opt_str("remote-test-client").map(PathBuf::from), @@ -339,7 +345,7 @@ pub fn log_config(config: &Config) { logv(c, format!("ar: {}", config.ar)); logv(c, format!("linker: {:?}", config.linker)); logv(c, format!("verbose: {}", config.verbose)); - logv(c, format!("quiet: {}", config.quiet)); + logv(c, format!("format: {:?}", config.format)); logv(c, "\n".to_string()); } @@ -501,7 +507,7 @@ pub fn test_opts(config: &Config) -> test::TestOpts { filters: config.filters.clone(), filter_exact: config.filter_exact, run_ignored: if config.run_ignored { test::RunIgnored::Yes } else { test::RunIgnored::No }, - format: if config.quiet { test::OutputFormat::Terse } else { test::OutputFormat::Pretty }, + format: config.format, logfile: config.logfile.clone(), run_tests: true, bench_benchmarks: true, From d2f38065f3fb89fb0361c7f1a1a34c070e10297a Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 11:13:48 +0100 Subject: [PATCH 03/26] render compiletest output with render_tests --- src/bootstrap/lib.rs | 1 + src/bootstrap/render_tests.rs | 220 ++++++++++++++++++++++++++++++++++ src/bootstrap/test.rs | 8 +- 3 files changed, 224 insertions(+), 5 deletions(-) create mode 100644 src/bootstrap/render_tests.rs diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index f4abdf1cc5758..f10d640a2090c 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -55,6 +55,7 @@ mod format; mod install; mod metadata; mod native; +mod render_tests; mod run; mod sanity; mod setup; diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs new file mode 100644 index 0000000000000..6a0600fb708cc --- /dev/null +++ b/src/bootstrap/render_tests.rs @@ -0,0 +1,220 @@ +//! This module renders the JSON output of libtest into a human-readable form, trying to be as +//! similar to libtest's native output as possible. +//! +//! This is needed because we need to use libtest in JSON mode to extract granluar information +//! about the executed tests. Doing so suppresses the human-readable output, and (compared to Cargo +//! and rustc) libtest doesn't include the rendered human-readable output as a JSON field. We had +//! to reimplement all the rendering logic in this module because of that. + +use crate::builder::Builder; +use std::io::{BufRead, BufReader}; +use std::process::{ChildStdout, Command, Stdio}; +use std::time::Duration; + +pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { + if builder.config.dry_run() { + return true; + } + + if !run_tests(builder, cmd) { + if builder.fail_fast { + crate::detail_exit(1); + } else { + let mut failures = builder.delayed_failures.borrow_mut(); + failures.push(format!("{cmd:?}")); + false + } + } else { + true + } +} + +fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { + cmd.stdout(Stdio::piped()); + + builder.verbose(&format!("running: {cmd:?}")); + + let mut process = cmd.spawn().unwrap(); + let stdout = process.stdout.take().unwrap(); + let handle = std::thread::spawn(move || Renderer::new(stdout).render_all()); + + let result = process.wait().unwrap(); + handle.join().expect("test formatter thread failed"); + + if !result.success() && builder.is_verbose() { + println!( + "\n\ncommand did not execute successfully: {cmd:?}\n\ + expected success, got: {result}" + ); + } + + result.success() +} + +struct Renderer { + stdout: BufReader, + failures: Vec, +} + +impl Renderer { + fn new(stdout: ChildStdout) -> Self { + Self { stdout: BufReader::new(stdout), failures: Vec::new() } + } + + fn render_all(mut self) { + let mut line = String::new(); + loop { + line.clear(); + match self.stdout.read_line(&mut line) { + Ok(_) => {} + Err(err) if err.kind() == std::io::ErrorKind::UnexpectedEof => break, + Err(err) => panic!("failed to read output of test runner: {err}"), + } + if line.is_empty() { + break; + } + + self.render_message(match serde_json::from_str(&line) { + Ok(parsed) => parsed, + Err(err) => { + panic!("failed to parse libtest json output; error: {err}, line: {line:?}"); + } + }); + } + } + + fn render_test_outcome(&self, outcome: Outcome<'_>, test: &TestOutcome) { + // TODO: add support for terse output + self.render_test_outcome_verbose(outcome, test); + } + + fn render_test_outcome_verbose(&self, outcome: Outcome<'_>, test: &TestOutcome) { + if let Some(exec_time) = test.exec_time { + println!( + "test {} ... {outcome} (in {:.2?})", + test.name, + Duration::from_secs_f64(exec_time) + ); + } else { + println!("test {} ... {outcome}", test.name); + } + } + + fn render_suite_outcome(&self, outcome: Outcome<'_>, suite: &SuiteOutcome) { + if !self.failures.is_empty() { + println!("\nfailures:\n"); + for failure in &self.failures { + if let Some(stdout) = &failure.stdout { + println!("---- {} stdout ----", failure.name); + println!("{stdout}"); + } + } + + println!("\nfailures:"); + for failure in &self.failures { + println!(" {}", failure.name); + } + } + + println!( + "\ntest result: {outcome}. {} passed; {} failed; {} ignored; {} measured; \ + {} filtered out; finished in {:.2?}\n", + suite.passed, + suite.failed, + suite.ignored, + suite.measured, + suite.filtered_out, + Duration::from_secs_f64(suite.exec_time) + ); + } + + fn render_message(&mut self, message: Message) { + match message { + Message::Suite(SuiteMessage::Started { test_count }) => { + println!("\nrunning {test_count} tests"); + } + Message::Suite(SuiteMessage::Ok(outcome)) => { + self.render_suite_outcome(Outcome::Ok, &outcome); + } + Message::Suite(SuiteMessage::Failed(outcome)) => { + self.render_suite_outcome(Outcome::Failed, &outcome); + } + Message::Test(TestMessage::Ok(outcome)) => { + self.render_test_outcome(Outcome::Ok, &outcome); + } + Message::Test(TestMessage::Ignored(outcome)) => { + self.render_test_outcome( + Outcome::Ignored { reason: outcome.reason.as_deref() }, + &outcome, + ); + } + Message::Test(TestMessage::Failed(outcome)) => { + self.render_test_outcome(Outcome::Failed, &outcome); + self.failures.push(outcome); + } + Message::Test(TestMessage::Started) => {} // Not useful + Message::Test(TestMessage::Bench) => todo!("benchmarks are not supported yet"), + } + } +} + +enum Outcome<'a> { + Ok, + Failed, + Ignored { reason: Option<&'a str> }, +} + +impl std::fmt::Display for Outcome<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Outcome::Ok => f.write_str("ok"), + Outcome::Failed => f.write_str("FAILED"), + Outcome::Ignored { reason: None } => f.write_str("ignored"), + Outcome::Ignored { reason: Some(reason) } => write!(f, "ignored, {reason}"), + } + } +} + +#[derive(serde_derive::Deserialize)] +#[serde(tag = "type", rename_all = "snake_case")] +enum Message { + Suite(SuiteMessage), + Test(TestMessage), +} + +#[derive(serde_derive::Deserialize)] +#[serde(tag = "event", rename_all = "snake_case")] +enum SuiteMessage { + Ok(SuiteOutcome), + Failed(SuiteOutcome), + Started { test_count: usize }, +} + +#[derive(serde_derive::Deserialize)] +struct SuiteOutcome { + passed: usize, + failed: usize, + ignored: usize, + measured: usize, + filtered_out: usize, + exec_time: f64, +} + +#[derive(serde_derive::Deserialize)] +#[serde(tag = "event", rename_all = "snake_case")] +enum TestMessage { + Ok(TestOutcome), + Failed(TestOutcome), + Ignored(TestOutcome), + // Ignored messages: + Bench, + Started, +} + +#[derive(serde_derive::Deserialize)] +struct TestOutcome { + name: String, + exec_time: Option, + stdout: Option, + reason: Option, +} diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index b4f1506dc8f30..1576326dfb040 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -1616,9 +1616,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the cmd.arg("--verbose"); } - if !builder.config.verbose_tests { - cmd.arg("--quiet"); - } + cmd.arg("--json"); let mut llvm_components_passed = false; let mut copts_passed = false; @@ -1769,7 +1767,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the suite, mode, &compiler.host, target )); let _time = util::timeit(&builder); - try_run(builder, &mut cmd); + crate::render_tests::try_run_tests(builder, &mut cmd); if let Some(compare_mode) = compare_mode { cmd.arg("--compare-mode").arg(compare_mode); @@ -1778,7 +1776,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the suite, mode, compare_mode, &compiler.host, target )); let _time = util::timeit(&builder); - try_run(builder, &mut cmd); + crate::render_tests::try_run_tests(builder, &mut cmd); } } } From f96774bfbb0cfcb9d52d9bd0c0d14f30d3409232 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 11:30:14 +0100 Subject: [PATCH 04/26] add support for terse output --- src/bootstrap/render_tests.rs | 63 +++++++++++++++++++++++++++++++---- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index 6a0600fb708cc..ae2147f7550f3 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -7,10 +7,12 @@ //! to reimplement all the rendering logic in this module because of that. use crate::builder::Builder; -use std::io::{BufRead, BufReader}; +use std::io::{BufRead, BufReader, Write}; use std::process::{ChildStdout, Command, Stdio}; use std::time::Duration; +const TERSE_TESTS_PER_LINE: usize = 88; + pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { if builder.config.dry_run() { return true; @@ -36,7 +38,8 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { let mut process = cmd.spawn().unwrap(); let stdout = process.stdout.take().unwrap(); - let handle = std::thread::spawn(move || Renderer::new(stdout).render_all()); + let verbose = builder.config.verbose_tests; + let handle = std::thread::spawn(move || Renderer::new(stdout, verbose).render_all()); let result = process.wait().unwrap(); handle.join().expect("test formatter thread failed"); @@ -54,11 +57,22 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { struct Renderer { stdout: BufReader, failures: Vec, + verbose: bool, + tests_count: Option, + executed_tests: usize, + terse_tests_in_line: usize, } impl Renderer { - fn new(stdout: ChildStdout) -> Self { - Self { stdout: BufReader::new(stdout), failures: Vec::new() } + fn new(stdout: ChildStdout, verbose: bool) -> Self { + Self { + stdout: BufReader::new(stdout), + failures: Vec::new(), + verbose, + tests_count: None, + executed_tests: 0, + terse_tests_in_line: 0, + } } fn render_all(mut self) { @@ -83,9 +97,13 @@ impl Renderer { } } - fn render_test_outcome(&self, outcome: Outcome<'_>, test: &TestOutcome) { - // TODO: add support for terse output - self.render_test_outcome_verbose(outcome, test); + fn render_test_outcome(&mut self, outcome: Outcome<'_>, test: &TestOutcome) { + self.executed_tests += 1; + if self.verbose { + self.render_test_outcome_verbose(outcome, test); + } else { + self.render_test_outcome_terse(outcome, test); + } } fn render_test_outcome_verbose(&self, outcome: Outcome<'_>, test: &TestOutcome) { @@ -100,7 +118,35 @@ impl Renderer { } } + fn render_test_outcome_terse(&mut self, outcome: Outcome<'_>, _: &TestOutcome) { + if self.terse_tests_in_line != 0 && self.terse_tests_in_line % TERSE_TESTS_PER_LINE == 0 { + if let Some(total) = self.tests_count { + let total = total.to_string(); + let executed = format!("{:>width$}", self.executed_tests - 1, width = total.len()); + print!(" {executed}/{total}"); + } + println!(); + self.terse_tests_in_line = 0; + } + + self.terse_tests_in_line += 1; + print!( + "{}", + match outcome { + Outcome::Ok => ".", + Outcome::Failed => "F", + Outcome::Ignored { .. } => "i", + } + ); + let _ = std::io::stdout().flush(); + } + fn render_suite_outcome(&self, outcome: Outcome<'_>, suite: &SuiteOutcome) { + // The terse output doesn't end with a newline, so we need to add it ourselves. + if !self.verbose { + println!(); + } + if !self.failures.is_empty() { println!("\nfailures:\n"); for failure in &self.failures { @@ -132,6 +178,9 @@ impl Renderer { match message { Message::Suite(SuiteMessage::Started { test_count }) => { println!("\nrunning {test_count} tests"); + self.executed_tests = 0; + self.terse_tests_in_line = 0; + self.tests_count = Some(test_count); } Message::Suite(SuiteMessage::Ok(outcome)) => { self.render_suite_outcome(Outcome::Ok, &outcome); From f816d3a75479ba073570a4e998735be28770a307 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 12:23:14 +0100 Subject: [PATCH 05/26] add a splash of color --- src/bootstrap/Cargo.lock | 22 ++++++++++++ src/bootstrap/Cargo.toml | 2 ++ src/bootstrap/config.rs | 6 ++++ src/bootstrap/lib.rs | 18 ++++++++++ src/bootstrap/render_tests.rs | 63 +++++++++++++++++++---------------- 5 files changed, 82 insertions(+), 29 deletions(-) diff --git a/src/bootstrap/Cargo.lock b/src/bootstrap/Cargo.lock index e861d520c5343..8195823efaffa 100644 --- a/src/bootstrap/Cargo.lock +++ b/src/bootstrap/Cargo.lock @@ -11,6 +11,17 @@ dependencies = [ "memchr", ] +[[package]] +name = "atty" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9b39be18770d11421cdb1b9947a45dd3f37e93092cbf377614828a319d5fee8" +dependencies = [ + "hermit-abi", + "libc", + "winapi", +] + [[package]] name = "autocfg" version = "1.1.0" @@ -36,6 +47,7 @@ dependencies = [ name = "bootstrap" version = "0.0.0" dependencies = [ + "atty", "build_helper", "cc", "cmake", @@ -59,6 +71,7 @@ dependencies = [ "walkdir", "winapi", "xz2", + "yansi-term", ] [[package]] @@ -800,3 +813,12 @@ name = "yansi" version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec" + +[[package]] +name = "yansi-term" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fe5c30ade05e61656247b2e334a031dfd0cc466fadef865bdcdea8d537951bf1" +dependencies = [ + "winapi", +] diff --git a/src/bootstrap/Cargo.toml b/src/bootstrap/Cargo.toml index 663987f113c8b..e704799867b39 100644 --- a/src/bootstrap/Cargo.toml +++ b/src/bootstrap/Cargo.toml @@ -30,6 +30,7 @@ path = "bin/sccache-plus-cl.rs" test = false [dependencies] +atty = "0.2.14" build_helper = { path = "../tools/build_helper" } cmake = "0.1.38" fd-lock = "3.0.8" @@ -52,6 +53,7 @@ opener = "0.5" once_cell = "1.7.2" xz2 = "0.1" walkdir = "2" +yansi-term = "0.1.2" # Dependencies needed by the build-metrics feature sysinfo = { version = "0.26.0", optional = true } diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 05e742549499b..bfa237aa14d86 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -87,6 +87,9 @@ pub struct Config { pub patch_binaries_for_nix: bool, pub stage0_metadata: Stage0Metadata, + pub stdout_is_tty: bool, + pub stderr_is_tty: bool, + pub on_fail: Option, pub stage: u32, pub keep_stage: Vec, @@ -822,6 +825,9 @@ impl Config { config.deny_warnings = true; config.bindir = "bin".into(); + config.stdout_is_tty = atty::is(atty::Stream::Stdout); + config.stderr_is_tty = atty::is(atty::Stream::Stderr); + // set by build.rs config.build = TargetSelection::from_user(&env!("BUILD_TRIPLE")); diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index f10d640a2090c..0eff8769b6091 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -89,6 +89,7 @@ pub use crate::builder::PathSet; use crate::cache::{Interned, INTERNER}; pub use crate::config::Config; pub use crate::flags::Subcommand; +use yansi_term::Color; const LLVM_TOOLS: &[&str] = &[ "llvm-cov", // used to generate coverage report @@ -1575,6 +1576,23 @@ to download LLVM rather than building it. self.config.ninja_in_file } + + pub fn color_for_stdout(&self, color: Color, message: &str) -> String { + self.color_for_inner(color, message, self.config.stdout_is_tty) + } + + pub fn color_for_stderr(&self, color: Color, message: &str) -> String { + self.color_for_inner(color, message, self.config.stderr_is_tty) + } + + fn color_for_inner(&self, color: Color, message: &str, is_tty: bool) -> String { + let use_color = match self.config.color { + flags::Color::Always => true, + flags::Color::Never => false, + flags::Color::Auto => is_tty, + }; + if use_color { color.paint(message).to_string() } else { message.to_string() } + } } #[cfg(unix)] diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index ae2147f7550f3..b5ab486f3ac68 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -10,6 +10,7 @@ use crate::builder::Builder; use std::io::{BufRead, BufReader, Write}; use std::process::{ChildStdout, Command, Stdio}; use std::time::Duration; +use yansi_term::Color; const TERSE_TESTS_PER_LINE: usize = 88; @@ -37,13 +38,12 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { builder.verbose(&format!("running: {cmd:?}")); let mut process = cmd.spawn().unwrap(); - let stdout = process.stdout.take().unwrap(); - let verbose = builder.config.verbose_tests; - let handle = std::thread::spawn(move || Renderer::new(stdout, verbose).render_all()); - let result = process.wait().unwrap(); - handle.join().expect("test formatter thread failed"); + // This runs until the stdout of the child is closed, which means the child exited. We don't + // run this on another thread since the builder is not Sync. + Renderer::new(process.stdout.take().unwrap(), builder).render_all(); + let result = process.wait().unwrap(); if !result.success() && builder.is_verbose() { println!( "\n\ncommand did not execute successfully: {cmd:?}\n\ @@ -54,21 +54,21 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { result.success() } -struct Renderer { +struct Renderer<'a> { stdout: BufReader, failures: Vec, - verbose: bool, + builder: &'a Builder<'a>, tests_count: Option, executed_tests: usize, terse_tests_in_line: usize, } -impl Renderer { - fn new(stdout: ChildStdout, verbose: bool) -> Self { +impl<'a> Renderer<'a> { + fn new(stdout: ChildStdout, builder: &'a Builder<'a>) -> Self { Self { stdout: BufReader::new(stdout), failures: Vec::new(), - verbose, + builder, tests_count: None, executed_tests: 0, terse_tests_in_line: 0, @@ -99,7 +99,7 @@ impl Renderer { fn render_test_outcome(&mut self, outcome: Outcome<'_>, test: &TestOutcome) { self.executed_tests += 1; - if self.verbose { + if self.builder.config.verbose_tests { self.render_test_outcome_verbose(outcome, test); } else { self.render_test_outcome_terse(outcome, test); @@ -109,12 +109,13 @@ impl Renderer { fn render_test_outcome_verbose(&self, outcome: Outcome<'_>, test: &TestOutcome) { if let Some(exec_time) = test.exec_time { println!( - "test {} ... {outcome} (in {:.2?})", + "test {} ... {} (in {:.2?})", test.name, + outcome.long(self.builder), Duration::from_secs_f64(exec_time) ); } else { - println!("test {} ... {outcome}", test.name); + println!("test {} ... {}", test.name, outcome.long(self.builder)); } } @@ -130,20 +131,13 @@ impl Renderer { } self.terse_tests_in_line += 1; - print!( - "{}", - match outcome { - Outcome::Ok => ".", - Outcome::Failed => "F", - Outcome::Ignored { .. } => "i", - } - ); + print!("{}", outcome.short(self.builder)); let _ = std::io::stdout().flush(); } fn render_suite_outcome(&self, outcome: Outcome<'_>, suite: &SuiteOutcome) { // The terse output doesn't end with a newline, so we need to add it ourselves. - if !self.verbose { + if !self.builder.config.verbose_tests { println!(); } @@ -163,8 +157,9 @@ impl Renderer { } println!( - "\ntest result: {outcome}. {} passed; {} failed; {} ignored; {} measured; \ + "\ntest result: {}. {} passed; {} failed; {} ignored; {} measured; \ {} filtered out; finished in {:.2?}\n", + outcome.long(self.builder), suite.passed, suite.failed, suite.ignored, @@ -213,13 +208,23 @@ enum Outcome<'a> { Ignored { reason: Option<&'a str> }, } -impl std::fmt::Display for Outcome<'_> { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl Outcome<'_> { + fn short(&self, builder: &Builder<'_>) -> String { match self { - Outcome::Ok => f.write_str("ok"), - Outcome::Failed => f.write_str("FAILED"), - Outcome::Ignored { reason: None } => f.write_str("ignored"), - Outcome::Ignored { reason: Some(reason) } => write!(f, "ignored, {reason}"), + Outcome::Ok => builder.color_for_stdout(Color::Green, "."), + Outcome::Failed => builder.color_for_stdout(Color::Red, "F"), + Outcome::Ignored { .. } => builder.color_for_stdout(Color::Yellow, "i"), + } + } + + fn long(&self, builder: &Builder<'_>) -> String { + match self { + Outcome::Ok => builder.color_for_stdout(Color::Green, "ok"), + Outcome::Failed => builder.color_for_stdout(Color::Red, "FAILED"), + Outcome::Ignored { reason: None } => builder.color_for_stdout(Color::Yellow, "ignored"), + Outcome::Ignored { reason: Some(reason) } => { + builder.color_for_stdout(Color::Yellow, &format!("ignored, {reason}")) + } } } } From 9388c8e420d780b95c1286d0b17eed9b5446ee9a Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 13:00:46 +0100 Subject: [PATCH 06/26] record tests in build metrics --- src/bootstrap/metrics.rs | 44 +++++++++++++++++++++++++++++++---- src/bootstrap/render_tests.rs | 13 +++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index 2e62c950709df..5f254761aa19f 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -51,6 +51,7 @@ impl BuildMetrics { duration_excluding_children_sec: Duration::ZERO, children: Vec::new(), + tests: Vec::new(), }); } @@ -72,6 +73,16 @@ impl BuildMetrics { } } + pub(crate) fn record_test(&self, name: &str, outcome: TestOutcome) { + let mut state = self.state.borrow_mut(); + state + .running_steps + .last_mut() + .unwrap() + .tests + .push(Test { name: name.to_string(), outcome }); + } + fn collect_stats(&self, state: &mut MetricsState) { let step = state.running_steps.last_mut().unwrap(); @@ -125,6 +136,14 @@ impl BuildMetrics { } fn prepare_json_step(&self, step: StepMetrics) -> JsonNode { + let mut children = Vec::new(); + children.extend(step.children.into_iter().map(|child| self.prepare_json_step(child))); + children.extend( + step.tests + .into_iter() + .map(|test| JsonNode::Test { name: test.name, outcome: test.outcome }), + ); + JsonNode::RustbuildStep { type_: step.type_, debug_repr: step.debug_repr, @@ -135,11 +154,7 @@ impl BuildMetrics { / step.duration_excluding_children_sec.as_secs_f64(), }, - children: step - .children - .into_iter() - .map(|child| self.prepare_json_step(child)) - .collect(), + children, } } } @@ -161,6 +176,12 @@ struct StepMetrics { duration_excluding_children_sec: Duration, children: Vec, + tests: Vec, +} + +struct Test { + name: String, + outcome: TestOutcome, } #[derive(Serialize, Deserialize)] @@ -190,6 +211,19 @@ enum JsonNode { children: Vec, }, + Test { + name: String, + #[serde(flatten)] + outcome: TestOutcome, + }, +} + +#[derive(Serialize, Deserialize)] +#[serde(tag = "outcome", rename_all = "snake_case")] +pub(crate) enum TestOutcome { + Passed, + Failed, + Ignored { ignore_reason: Option }, } #[derive(Serialize, Deserialize)] diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index b5ab486f3ac68..7c702f19c09f7 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -99,6 +99,19 @@ impl<'a> Renderer<'a> { fn render_test_outcome(&mut self, outcome: Outcome<'_>, test: &TestOutcome) { self.executed_tests += 1; + + #[cfg(feature = "build-metrics")] + self.builder.metrics.record_test( + &test.name, + match outcome { + Outcome::Ok => crate::metrics::TestOutcome::Passed, + Outcome::Failed => crate::metrics::TestOutcome::Failed, + Outcome::Ignored { reason } => crate::metrics::TestOutcome::Ignored { + ignore_reason: reason.map(|s| s.to_string()), + }, + }, + ); + if self.builder.config.verbose_tests { self.render_test_outcome_verbose(outcome, test); } else { From b14b35555022b6df2fd6192693800f2032f61a33 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 14:50:01 +0100 Subject: [PATCH 07/26] add support for benchmarks --- src/bootstrap/render_tests.rs | 53 ++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index 7c702f19c09f7..bc16ed9cbaf5a 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -57,6 +57,7 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { struct Renderer<'a> { stdout: BufReader, failures: Vec, + benches: Vec, builder: &'a Builder<'a>, tests_count: Option, executed_tests: usize, @@ -67,6 +68,7 @@ impl<'a> Renderer<'a> { fn new(stdout: ChildStdout, builder: &'a Builder<'a>) -> Self { Self { stdout: BufReader::new(stdout), + benches: Vec::new(), failures: Vec::new(), builder, tests_count: None, @@ -104,7 +106,7 @@ impl<'a> Renderer<'a> { self.builder.metrics.record_test( &test.name, match outcome { - Outcome::Ok => crate::metrics::TestOutcome::Passed, + Outcome::Ok | Outcome::BenchOk => crate::metrics::TestOutcome::Passed, Outcome::Failed => crate::metrics::TestOutcome::Failed, Outcome::Ignored { reason } => crate::metrics::TestOutcome::Ignored { ignore_reason: reason.map(|s| s.to_string()), @@ -169,6 +171,26 @@ impl<'a> Renderer<'a> { } } + if !self.benches.is_empty() { + println!("\nbenchmarks:"); + + let mut rows = Vec::new(); + for bench in &self.benches { + rows.push(( + &bench.name, + format!("{:.2?}/iter", Duration::from_nanos(bench.median)), + format!("+/- {:.2?}", Duration::from_nanos(bench.deviation)), + )); + } + + let max_0 = rows.iter().map(|r| r.0.len()).max().unwrap_or(0); + let max_1 = rows.iter().map(|r| r.1.len()).max().unwrap_or(0); + let max_2 = rows.iter().map(|r| r.2.len()).max().unwrap_or(0); + for row in &rows { + println!(" {:max_1$} {:>max_2$}", row.0, row.1, row.2); + } + } + println!( "\ntest result: {}. {} passed; {} failed; {} ignored; {} measured; \ {} filtered out; finished in {:.2?}\n", @@ -196,6 +218,21 @@ impl<'a> Renderer<'a> { Message::Suite(SuiteMessage::Failed(outcome)) => { self.render_suite_outcome(Outcome::Failed, &outcome); } + Message::Bench(outcome) => { + // The formatting for benchmarks doesn't replicate 1:1 the formatting libtest + // outputs, mostly because libtest's formatting is broken in terse mode, which is + // the default used by our monorepo. We use a different formatting instead: + // successful benchmarks are just showed as "benchmarked"/"b", and the details are + // outputted at the bottom like failures. + let fake_test_outcome = TestOutcome { + name: outcome.name.clone(), + exec_time: None, + stdout: None, + reason: None, + }; + self.render_test_outcome(Outcome::BenchOk, &fake_test_outcome); + self.benches.push(outcome); + } Message::Test(TestMessage::Ok(outcome)) => { self.render_test_outcome(Outcome::Ok, &outcome); } @@ -210,13 +247,13 @@ impl<'a> Renderer<'a> { self.failures.push(outcome); } Message::Test(TestMessage::Started) => {} // Not useful - Message::Test(TestMessage::Bench) => todo!("benchmarks are not supported yet"), } } } enum Outcome<'a> { Ok, + BenchOk, Failed, Ignored { reason: Option<&'a str> }, } @@ -225,6 +262,7 @@ impl Outcome<'_> { fn short(&self, builder: &Builder<'_>) -> String { match self { Outcome::Ok => builder.color_for_stdout(Color::Green, "."), + Outcome::BenchOk => builder.color_for_stdout(Color::Cyan, "b"), Outcome::Failed => builder.color_for_stdout(Color::Red, "F"), Outcome::Ignored { .. } => builder.color_for_stdout(Color::Yellow, "i"), } @@ -233,6 +271,7 @@ impl Outcome<'_> { fn long(&self, builder: &Builder<'_>) -> String { match self { Outcome::Ok => builder.color_for_stdout(Color::Green, "ok"), + Outcome::BenchOk => builder.color_for_stdout(Color::Cyan, "benchmarked"), Outcome::Failed => builder.color_for_stdout(Color::Red, "FAILED"), Outcome::Ignored { reason: None } => builder.color_for_stdout(Color::Yellow, "ignored"), Outcome::Ignored { reason: Some(reason) } => { @@ -247,6 +286,7 @@ impl Outcome<'_> { enum Message { Suite(SuiteMessage), Test(TestMessage), + Bench(BenchOutcome), } #[derive(serde_derive::Deserialize)] @@ -273,11 +313,16 @@ enum TestMessage { Ok(TestOutcome), Failed(TestOutcome), Ignored(TestOutcome), - // Ignored messages: - Bench, Started, } +#[derive(serde_derive::Deserialize)] +struct BenchOutcome { + name: String, + median: u64, + deviation: u64, +} + #[derive(serde_derive::Deserialize)] struct TestOutcome { name: String, From 50b35836956bd39e4bb3144b9139317bc84caf4e Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 15:36:13 +0100 Subject: [PATCH 08/26] switch all tests to use render_tests --- src/bootstrap/render_tests.rs | 9 ++++++++ src/bootstrap/test.rs | 41 +++++++++++++++-------------------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index bc16ed9cbaf5a..b9aa378064c07 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -14,6 +14,15 @@ use yansi_term::Color; const TERSE_TESTS_PER_LINE: usize = 88; +pub(crate) fn add_flags_and_try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { + if cmd.get_args().position(|arg| arg == "--").is_none() { + cmd.arg("--"); + } + cmd.args(&["-Z", "unstable-options", "--format", "json"]); + + try_run_tests(builder, cmd) +} + pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { if builder.config.dry_run() { return true; diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 1576326dfb040..754bd6c9c8ce3 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -20,6 +20,7 @@ use crate::dist; use crate::doc::DocumentationFormat; use crate::flags::Subcommand; use crate::native; +use crate::render_tests::add_flags_and_try_run_tests; use crate::tool::{self, SourceType, Tool}; use crate::toolstate::ToolState; use crate::util::{self, add_link_lib_path, dylib_path, dylib_path_var, output, t}; @@ -123,7 +124,7 @@ impl Step for CrateJsonDocLint { SourceType::InTree, &[], ); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -172,7 +173,7 @@ You can skip linkcheck with --exclude src/tools/linkchecker" SourceType::InTree, &[], ); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); // Build all the default documentation. builder.default_doc(&[]); @@ -333,7 +334,7 @@ impl Step for Cargo { cargo.env("PATH", &path_for_cargo(builder, compiler)); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -392,7 +393,7 @@ impl Step for RustAnalyzer { cargo.add_rustc_lib_path(builder, compiler); cargo.arg("--").args(builder.config.cmd.test_args()); - builder.run(&mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -445,7 +446,7 @@ impl Step for Rustfmt { cargo.add_rustc_lib_path(builder, compiler); - builder.run(&mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -496,7 +497,7 @@ impl Step for RustDemangler { cargo.add_rustc_lib_path(builder, compiler); - builder.run(&mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -637,8 +638,7 @@ impl Step for Miri { // Forward test filters. cargo.arg("--").args(builder.config.cmd.test_args()); - let mut cargo = Command::from(cargo); - builder.run(&mut cargo); + add_flags_and_try_run_tests(builder, &mut cargo.into()); // # Run `cargo miri test`. // This is just a smoke test (Miri's own CI invokes this in a bunch of different ways and ensures @@ -711,7 +711,7 @@ impl Step for CompiletestTest { ); cargo.allow_features("test"); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -767,7 +767,7 @@ impl Step for Clippy { cargo.add_rustc_lib_path(builder, compiler); - if builder.try_run(&mut cargo.into()) { + if add_flags_and_try_run_tests(builder, &mut cargo.into()) { // The tests succeeded; nothing to do. return; } @@ -1189,7 +1189,7 @@ impl Step for TidySelfTest { SourceType::InTree, &[], ); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -2178,9 +2178,8 @@ impl Step for Crate { cargo.arg("--"); cargo.args(&builder.config.cmd.test_args()); - if !builder.config.verbose_tests { - cargo.arg("--quiet"); - } + cargo.arg("-Z").arg("unstable-options"); + cargo.arg("--format").arg("json"); if target.contains("emscripten") { cargo.env( @@ -2208,7 +2207,7 @@ impl Step for Crate { target )); let _time = util::timeit(&builder); - try_run(builder, &mut cargo.into()); + crate::render_tests::try_run_tests(builder, &mut cargo.into()); } } @@ -2328,7 +2327,7 @@ impl Step for CrateRustdoc { )); let _time = util::timeit(&builder); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -2389,17 +2388,13 @@ impl Step for CrateRustdocJsonTypes { cargo.arg("'-Ctarget-feature=-crt-static'"); } - if !builder.config.verbose_tests { - cargo.arg("--quiet"); - } - builder.info(&format!( "{} rustdoc-json-types stage{} ({} -> {})", test_kind, compiler.stage, &compiler.host, target )); let _time = util::timeit(&builder); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } } @@ -2568,7 +2563,7 @@ impl Step for Bootstrap { // rustbuild tests are racy on directory creation so just run them one at a time. // Since there's not many this shouldn't be a problem. cmd.arg("--test-threads=1"); - try_run(builder, &mut cmd); + add_flags_and_try_run_tests(builder, &mut cmd); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { @@ -2649,7 +2644,7 @@ impl Step for ReplacePlaceholderTest { SourceType::InTree, &[], ); - try_run(builder, &mut cargo.into()); + add_flags_and_try_run_tests(builder, &mut cargo.into()); } fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { From ad9a4442418deb24cbb107d0a56026d005bfa859 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 16:20:18 +0100 Subject: [PATCH 09/26] avoid overlapping stderr --- src/bootstrap/render_tests.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index b9aa378064c07..bdcd39e6b83eb 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -7,7 +7,7 @@ //! to reimplement all the rendering logic in this module because of that. use crate::builder::Builder; -use std::io::{BufRead, BufReader, Write}; +use std::io::{BufRead, BufReader, Cursor, Write}; use std::process::{ChildStdout, Command, Stdio}; use std::time::Duration; use yansi_term::Color; @@ -43,6 +43,7 @@ pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { cmd.stdout(Stdio::piped()); + cmd.stderr(Stdio::piped()); builder.verbose(&format!("running: {cmd:?}")); @@ -52,15 +53,20 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { // run this on another thread since the builder is not Sync. Renderer::new(process.stdout.take().unwrap(), builder).render_all(); - let result = process.wait().unwrap(); - if !result.success() && builder.is_verbose() { + let result = process.wait_with_output().unwrap(); + if !result.status.success() && builder.is_verbose() { println!( "\n\ncommand did not execute successfully: {cmd:?}\n\ - expected success, got: {result}" + expected success, got: {}", + result.status ); } - result.success() + // Show the stderr emitted by the test runner at the end. As of 2023-03-02 this is only the + // message at the end of a failed compiletest run. + std::io::copy(&mut Cursor::new(&result.stderr), &mut std::io::stderr().lock()).unwrap(); + + result.status.success() } struct Renderer<'a> { From f23e20569c5beedc3739e97d8a43bc9a53b9e3c9 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 2 Mar 2023 17:10:29 +0100 Subject: [PATCH 10/26] do not use render_tests for clippy Behind the scenes Clippy uses compiletest-rs, which doesn't support the --json flag we added to Rust's compiletest. --- src/bootstrap/test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 754bd6c9c8ce3..71e83e741e12b 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -767,7 +767,7 @@ impl Step for Clippy { cargo.add_rustc_lib_path(builder, compiler); - if add_flags_and_try_run_tests(builder, &mut cargo.into()) { + if builder.try_run(&mut cargo.into()) { // The tests succeeded; nothing to do. return; } From 9a1ff1b95808c6639b8d00f46f675daafad566e7 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 3 Mar 2023 10:03:23 +0100 Subject: [PATCH 11/26] handle non-json output in stdout --- src/bootstrap/render_tests.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index bdcd39e6b83eb..1d8e8827e3856 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -105,12 +105,19 @@ impl<'a> Renderer<'a> { break; } - self.render_message(match serde_json::from_str(&line) { - Ok(parsed) => parsed, - Err(err) => { - panic!("failed to parse libtest json output; error: {err}, line: {line:?}"); - } - }); + let trimmed = line.trim(); + if trimmed.starts_with("{") && trimmed.ends_with("}") { + self.render_message(match serde_json::from_str(&trimmed) { + Ok(parsed) => parsed, + Err(err) => { + panic!("failed to parse libtest json output; error: {err}, line: {line:?}"); + } + }); + } else { + // Handle non-JSON output, for example when --nocapture is passed. + print!("{line}"); + let _ = std::io::stdout().flush(); + } } } From 49582725603102c9734bdcb44608a725d5c963b3 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 3 Mar 2023 11:09:47 +0100 Subject: [PATCH 12/26] change approach to prevent interleaving compiletest message --- src/bootstrap/render_tests.rs | 7 +------ src/tools/compiletest/src/main.rs | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index 1d8e8827e3856..cdc40d8315161 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -7,7 +7,7 @@ //! to reimplement all the rendering logic in this module because of that. use crate::builder::Builder; -use std::io::{BufRead, BufReader, Cursor, Write}; +use std::io::{BufRead, BufReader, Write}; use std::process::{ChildStdout, Command, Stdio}; use std::time::Duration; use yansi_term::Color; @@ -43,7 +43,6 @@ pub(crate) fn try_run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { cmd.stdout(Stdio::piped()); - cmd.stderr(Stdio::piped()); builder.verbose(&format!("running: {cmd:?}")); @@ -62,10 +61,6 @@ fn run_tests(builder: &Builder<'_>, cmd: &mut Command) -> bool { ); } - // Show the stderr emitted by the test runner at the end. As of 2023-03-02 this is only the - // message at the end of a failed compiletest run. - std::io::copy(&mut Cursor::new(&result.stderr), &mut std::io::stderr().lock()).unwrap(); - result.status.success() } diff --git a/src/tools/compiletest/src/main.rs b/src/tools/compiletest/src/main.rs index 167cd5c5dc170..cbaa599f79308 100644 --- a/src/tools/compiletest/src/main.rs +++ b/src/tools/compiletest/src/main.rs @@ -422,7 +422,7 @@ pub fn run_tests(config: Config) { // easy to miss which tests failed, and as such fail to reproduce // the failure locally. - eprintln!( + println!( "Some tests failed in compiletest suite={}{} mode={} host={} target={}", config.suite, config.compare_mode.map(|c| format!(" compare_mode={:?}", c)).unwrap_or_default(), From 3248ab758a43844ae69172cb7d082735e473ad29 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Mon, 6 Mar 2023 09:59:43 +0100 Subject: [PATCH 13/26] fix name of the field containing the ignore reason --- src/bootstrap/render_tests.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index cdc40d8315161..4e7b85d97ee5d 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -245,7 +245,7 @@ impl<'a> Renderer<'a> { name: outcome.name.clone(), exec_time: None, stdout: None, - reason: None, + message: None, }; self.render_test_outcome(Outcome::BenchOk, &fake_test_outcome); self.benches.push(outcome); @@ -255,7 +255,7 @@ impl<'a> Renderer<'a> { } Message::Test(TestMessage::Ignored(outcome)) => { self.render_test_outcome( - Outcome::Ignored { reason: outcome.reason.as_deref() }, + Outcome::Ignored { reason: outcome.message.as_deref() }, &outcome, ); } @@ -345,5 +345,5 @@ struct TestOutcome { name: String, exec_time: Option, stdout: Option, - reason: Option, + message: Option, } From c015d0d2faf88683678572bcf31c5bdff955d267 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Tue, 7 Mar 2023 10:21:25 +0100 Subject: [PATCH 14/26] switch to termcolor --- src/bootstrap/Cargo.lock | 20 +++++----- src/bootstrap/Cargo.toml | 2 +- src/bootstrap/lib.rs | 30 ++++++++------ src/bootstrap/render_tests.rs | 73 +++++++++++++++++++++++------------ 4 files changed, 78 insertions(+), 47 deletions(-) diff --git a/src/bootstrap/Cargo.lock b/src/bootstrap/Cargo.lock index 8195823efaffa..69bb98135ca14 100644 --- a/src/bootstrap/Cargo.lock +++ b/src/bootstrap/Cargo.lock @@ -67,11 +67,11 @@ dependencies = [ "sha2", "sysinfo", "tar", + "termcolor", "toml", "walkdir", "winapi", "xz2", - "yansi-term", ] [[package]] @@ -649,6 +649,15 @@ dependencies = [ "xattr", ] +[[package]] +name = "termcolor" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "be55cf8942feac5c765c2c993422806843c9a9a45d4d5c407ad6dd2ea95eb9b6" +dependencies = [ + "winapi-util", +] + [[package]] name = "thread_local" version = "1.1.4" @@ -813,12 +822,3 @@ name = "yansi" version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09041cd90cf85f7f8b2df60c646f853b7f535ce68f85244eb6731cf89fa498ec" - -[[package]] -name = "yansi-term" -version = "0.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "fe5c30ade05e61656247b2e334a031dfd0cc466fadef865bdcdea8d537951bf1" -dependencies = [ - "winapi", -] diff --git a/src/bootstrap/Cargo.toml b/src/bootstrap/Cargo.toml index e704799867b39..83e63df50147c 100644 --- a/src/bootstrap/Cargo.toml +++ b/src/bootstrap/Cargo.toml @@ -47,13 +47,13 @@ serde_derive = "1.0.137" serde_json = "1.0.2" sha2 = "0.10" tar = "0.4" +termcolor = "1.2.0" toml = "0.5" ignore = "0.4.10" opener = "0.5" once_cell = "1.7.2" xz2 = "0.1" walkdir = "2" -yansi-term = "0.1.2" # Dependencies needed by the build-metrics feature sysinfo = { version = "0.26.0", optional = true } diff --git a/src/bootstrap/lib.rs b/src/bootstrap/lib.rs index 0eff8769b6091..9e1d88a6aefcc 100644 --- a/src/bootstrap/lib.rs +++ b/src/bootstrap/lib.rs @@ -89,7 +89,7 @@ pub use crate::builder::PathSet; use crate::cache::{Interned, INTERNER}; pub use crate::config::Config; pub use crate::flags::Subcommand; -use yansi_term::Color; +use termcolor::{ColorChoice, StandardStream, WriteColor}; const LLVM_TOOLS: &[&str] = &[ "llvm-cov", // used to generate coverage report @@ -1577,21 +1577,29 @@ to download LLVM rather than building it. self.config.ninja_in_file } - pub fn color_for_stdout(&self, color: Color, message: &str) -> String { - self.color_for_inner(color, message, self.config.stdout_is_tty) + pub fn colored_stdout R>(&self, f: F) -> R { + self.colored_stream_inner(StandardStream::stdout, self.config.stdout_is_tty, f) } - pub fn color_for_stderr(&self, color: Color, message: &str) -> String { - self.color_for_inner(color, message, self.config.stderr_is_tty) + pub fn colored_stderr R>(&self, f: F) -> R { + self.colored_stream_inner(StandardStream::stderr, self.config.stderr_is_tty, f) } - fn color_for_inner(&self, color: Color, message: &str, is_tty: bool) -> String { - let use_color = match self.config.color { - flags::Color::Always => true, - flags::Color::Never => false, - flags::Color::Auto => is_tty, + fn colored_stream_inner(&self, constructor: C, is_tty: bool, f: F) -> R + where + C: Fn(ColorChoice) -> StandardStream, + F: FnOnce(&mut dyn WriteColor) -> R, + { + let choice = match self.config.color { + flags::Color::Always => ColorChoice::Always, + flags::Color::Never => ColorChoice::Never, + flags::Color::Auto if !is_tty => ColorChoice::Never, + flags::Color::Auto => ColorChoice::Auto, }; - if use_color { color.paint(message).to_string() } else { message.to_string() } + let mut stream = constructor(choice); + let result = f(&mut stream); + stream.reset().unwrap(); + result } } diff --git a/src/bootstrap/render_tests.rs b/src/bootstrap/render_tests.rs index 4e7b85d97ee5d..fd78e449a49b8 100644 --- a/src/bootstrap/render_tests.rs +++ b/src/bootstrap/render_tests.rs @@ -10,7 +10,7 @@ use crate::builder::Builder; use std::io::{BufRead, BufReader, Write}; use std::process::{ChildStdout, Command, Stdio}; use std::time::Duration; -use yansi_term::Color; +use termcolor::{Color, ColorSpec, WriteColor}; const TERSE_TESTS_PER_LINE: usize = 88; @@ -139,16 +139,12 @@ impl<'a> Renderer<'a> { } fn render_test_outcome_verbose(&self, outcome: Outcome<'_>, test: &TestOutcome) { + print!("test {} ... ", test.name); + self.builder.colored_stdout(|stdout| outcome.write_long(stdout)).unwrap(); if let Some(exec_time) = test.exec_time { - println!( - "test {} ... {} (in {:.2?})", - test.name, - outcome.long(self.builder), - Duration::from_secs_f64(exec_time) - ); - } else { - println!("test {} ... {}", test.name, outcome.long(self.builder)); + print!(" ({exec_time:.2?})"); } + println!(); } fn render_test_outcome_terse(&mut self, outcome: Outcome<'_>, _: &TestOutcome) { @@ -163,7 +159,7 @@ impl<'a> Renderer<'a> { } self.terse_tests_in_line += 1; - print!("{}", outcome.short(self.builder)); + self.builder.colored_stdout(|stdout| outcome.write_short(stdout)).unwrap(); let _ = std::io::stdout().flush(); } @@ -208,10 +204,11 @@ impl<'a> Renderer<'a> { } } + print!("\ntest result: "); + self.builder.colored_stdout(|stdout| outcome.write_long(stdout)).unwrap(); println!( - "\ntest result: {}. {} passed; {} failed; {} ignored; {} measured; \ - {} filtered out; finished in {:.2?}\n", - outcome.long(self.builder), + ". {} passed; {} failed; {} ignored; {} measured; {} filtered out; \ + finished in {:.2?}\n", suite.passed, suite.failed, suite.ignored, @@ -276,25 +273,51 @@ enum Outcome<'a> { } impl Outcome<'_> { - fn short(&self, builder: &Builder<'_>) -> String { + fn write_short(&self, writer: &mut dyn WriteColor) -> Result<(), std::io::Error> { match self { - Outcome::Ok => builder.color_for_stdout(Color::Green, "."), - Outcome::BenchOk => builder.color_for_stdout(Color::Cyan, "b"), - Outcome::Failed => builder.color_for_stdout(Color::Red, "F"), - Outcome::Ignored { .. } => builder.color_for_stdout(Color::Yellow, "i"), + Outcome::Ok => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Green)))?; + write!(writer, ".")?; + } + Outcome::BenchOk => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Cyan)))?; + write!(writer, "b")?; + } + Outcome::Failed => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Red)))?; + write!(writer, "F")?; + } + Outcome::Ignored { .. } => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Yellow)))?; + write!(writer, "i")?; + } } + writer.reset() } - fn long(&self, builder: &Builder<'_>) -> String { + fn write_long(&self, writer: &mut dyn WriteColor) -> Result<(), std::io::Error> { match self { - Outcome::Ok => builder.color_for_stdout(Color::Green, "ok"), - Outcome::BenchOk => builder.color_for_stdout(Color::Cyan, "benchmarked"), - Outcome::Failed => builder.color_for_stdout(Color::Red, "FAILED"), - Outcome::Ignored { reason: None } => builder.color_for_stdout(Color::Yellow, "ignored"), - Outcome::Ignored { reason: Some(reason) } => { - builder.color_for_stdout(Color::Yellow, &format!("ignored, {reason}")) + Outcome::Ok => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Green)))?; + write!(writer, "ok")?; + } + Outcome::BenchOk => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Cyan)))?; + write!(writer, "benchmarked")?; + } + Outcome::Failed => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Red)))?; + write!(writer, "FAILED")?; + } + Outcome::Ignored { reason } => { + writer.set_color(&ColorSpec::new().set_fg(Some(Color::Yellow)))?; + write!(writer, "ignored")?; + if let Some(reason) = reason { + write!(writer, ", {reason}")?; + } } } + writer.reset() } } From f7a97027b2d116a1f799db0f67ab340ccc60b4df Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sat, 18 Mar 2023 19:18:51 +0400 Subject: [PATCH 15/26] rustdoc: Cleanup parent module tracking for doc links Keep ids of the documented items themselves, not their parent modules. Parent modules can be retreived from those ids when necessary. --- compiler/rustc_resolve/src/rustdoc.rs | 18 +-- src/librustdoc/clean/inline.rs | 75 ++++------- src/librustdoc/clean/mod.rs | 20 +-- src/librustdoc/clean/types/tests.rs | 2 +- src/librustdoc/clean/utils.rs | 4 +- .../passes/collect_intra_doc_links.rs | 126 ++++++------------ src/librustdoc/passes/collect_trait_impls.rs | 6 +- src/librustdoc/passes/propagate_doc_cfg.rs | 3 +- .../intra-doc/auxiliary/inner-crate-doc.rs | 1 + .../intra-doc/import-inline-merge-module.rs | 10 ++ 10 files changed, 100 insertions(+), 165 deletions(-) create mode 100644 tests/rustdoc-ui/intra-doc/auxiliary/inner-crate-doc.rs create mode 100644 tests/rustdoc-ui/intra-doc/import-inline-merge-module.rs diff --git a/compiler/rustc_resolve/src/rustdoc.rs b/compiler/rustc_resolve/src/rustdoc.rs index b8853c1744c92..0e40f794f1860 100644 --- a/compiler/rustc_resolve/src/rustdoc.rs +++ b/compiler/rustc_resolve/src/rustdoc.rs @@ -26,11 +26,13 @@ pub enum DocFragmentKind { #[derive(Clone, PartialEq, Eq, Debug)] pub struct DocFragment { pub span: Span, - /// The module this doc-comment came from. - /// - /// This allows distinguishing between the original documentation and a pub re-export. - /// If it is `None`, the item was not re-exported. - pub parent_module: Option, + /// The item this doc-comment came from. + /// Used to determine the scope in which doc links in this fragment are resolved. + /// Typically filled for reexport docs when they are merged into the docs of the + /// original reexported item. + /// If the id is not filled, which happens for the original reexported item, then + /// it has to be taken from somewhere else during doc link resolution. + pub item_id: Option, pub doc: Symbol, pub kind: DocFragmentKind, pub indent: usize, @@ -186,7 +188,7 @@ pub fn attrs_to_doc_fragments<'a>( ) -> (Vec, ast::AttrVec) { let mut doc_fragments = Vec::new(); let mut other_attrs = ast::AttrVec::new(); - for (attr, parent_module) in attrs { + for (attr, item_id) in attrs { if let Some((doc_str, comment_kind)) = attr.doc_str_and_comment_kind() { let doc = beautify_doc_string(doc_str, comment_kind); let kind = if attr.is_doc_comment() { @@ -194,7 +196,7 @@ pub fn attrs_to_doc_fragments<'a>( } else { DocFragmentKind::RawDoc }; - let fragment = DocFragment { span: attr.span, doc, kind, parent_module, indent: 0 }; + let fragment = DocFragment { span: attr.span, doc, kind, item_id, indent: 0 }; doc_fragments.push(fragment); } else if !doc_only { other_attrs.push(attr.clone()); @@ -216,7 +218,7 @@ pub fn prepare_to_doc_link_resolution( ) -> FxHashMap, String> { let mut res = FxHashMap::default(); for fragment in doc_fragments { - let out_str = res.entry(fragment.parent_module).or_default(); + let out_str = res.entry(fragment.item_id).or_default(); add_doc_fragment(out_str, fragment); } res diff --git a/src/librustdoc/clean/inline.rs b/src/librustdoc/clean/inline.rs index 148243683cbbf..768f8bb7bc899 100644 --- a/src/librustdoc/clean/inline.rs +++ b/src/librustdoc/clean/inline.rs @@ -36,15 +36,11 @@ use crate::formats::item_type::ItemType; /// /// The returned value is `None` if the definition could not be inlined, /// and `Some` of a vector of items if it was successfully expanded. -/// -/// `parent_module` refers to the parent of the *re-export*, not the original item. pub(crate) fn try_inline( cx: &mut DocContext<'_>, - parent_module: DefId, - import_def_id: Option, res: Res, name: Symbol, - attrs: Option<&[ast::Attribute]>, + attrs: Option<(&[ast::Attribute], Option)>, visited: &mut DefIdSet, ) -> Option> { let did = res.opt_def_id()?; @@ -55,38 +51,17 @@ pub(crate) fn try_inline( debug!("attrs={:?}", attrs); - let attrs_without_docs = attrs.map(|attrs| { - attrs.into_iter().filter(|a| a.doc_str().is_none()).cloned().collect::>() + let attrs_without_docs = attrs.map(|(attrs, def_id)| { + (attrs.into_iter().filter(|a| a.doc_str().is_none()).cloned().collect::>(), def_id) }); - // We need this ugly code because: - // - // ``` - // attrs_without_docs.map(|a| a.as_slice()) - // ``` - // - // will fail because it returns a temporary slice and: - // - // ``` - // attrs_without_docs.map(|s| { - // vec = s.as_slice(); - // vec - // }) - // ``` - // - // will fail because we're moving an uninitialized variable into a closure. - let vec; - let attrs_without_docs = match attrs_without_docs { - Some(s) => { - vec = s; - Some(vec.as_slice()) - } - None => None, - }; + let attrs_without_docs = + attrs_without_docs.as_ref().map(|(attrs, def_id)| (&attrs[..], *def_id)); + let import_def_id = attrs.and_then(|(_, def_id)| def_id); let kind = match res { Res::Def(DefKind::Trait, did) => { record_extern_fqn(cx, did, ItemType::Trait); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::TraitItem(Box::new(build_external_trait(cx, did))) } Res::Def(DefKind::Fn, did) => { @@ -95,27 +70,27 @@ pub(crate) fn try_inline( } Res::Def(DefKind::Struct, did) => { record_extern_fqn(cx, did, ItemType::Struct); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::StructItem(build_struct(cx, did)) } Res::Def(DefKind::Union, did) => { record_extern_fqn(cx, did, ItemType::Union); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::UnionItem(build_union(cx, did)) } Res::Def(DefKind::TyAlias, did) => { record_extern_fqn(cx, did, ItemType::Typedef); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::TypedefItem(build_type_alias(cx, did)) } Res::Def(DefKind::Enum, did) => { record_extern_fqn(cx, did, ItemType::Enum); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::EnumItem(build_enum(cx, did)) } Res::Def(DefKind::ForeignTy, did) => { record_extern_fqn(cx, did, ItemType::ForeignType); - build_impls(cx, Some(parent_module), did, attrs_without_docs, &mut ret); + build_impls(cx, did, attrs_without_docs, &mut ret); clean::ForeignTypeItem } // Never inline enum variants but leave them shown as re-exports. @@ -149,7 +124,7 @@ pub(crate) fn try_inline( _ => return None, }; - let (attrs, cfg) = merge_attrs(cx, Some(parent_module), load_attrs(cx, did), attrs); + let (attrs, cfg) = merge_attrs(cx, load_attrs(cx, did), attrs); cx.inlined.insert(did.into()); let mut item = clean::Item::from_def_id_and_attrs_and_parts(did, Some(name), kind, Box::new(attrs), cfg); @@ -316,9 +291,8 @@ fn build_type_alias(cx: &mut DocContext<'_>, did: DefId) -> Box /// Builds all inherent implementations of an ADT (struct/union/enum) or Trait item/path/reexport. pub(crate) fn build_impls( cx: &mut DocContext<'_>, - parent_module: Option, did: DefId, - attrs: Option<&[ast::Attribute]>, + attrs: Option<(&[ast::Attribute], Option)>, ret: &mut Vec, ) { let _prof_timer = cx.tcx.sess.prof.generic_activity("build_inherent_impls"); @@ -326,7 +300,7 @@ pub(crate) fn build_impls( // for each implementation of an item represented by `did`, build the clean::Item for that impl for &did in tcx.inherent_impls(did).iter() { - build_impl(cx, parent_module, did, attrs, ret); + build_impl(cx, did, attrs, ret); } // This pretty much exists expressly for `dyn Error` traits that exist in the `alloc` crate. @@ -340,28 +314,26 @@ pub(crate) fn build_impls( let type_ = if tcx.is_trait(did) { TraitSimplifiedType(did) } else { AdtSimplifiedType(did) }; for &did in tcx.incoherent_impls(type_) { - build_impl(cx, parent_module, did, attrs, ret); + build_impl(cx, did, attrs, ret); } } } -/// `parent_module` refers to the parent of the re-export, not the original item pub(crate) fn merge_attrs( cx: &mut DocContext<'_>, - parent_module: Option, old_attrs: &[ast::Attribute], - new_attrs: Option<&[ast::Attribute]>, + new_attrs: Option<(&[ast::Attribute], Option)>, ) -> (clean::Attributes, Option>) { // NOTE: If we have additional attributes (from a re-export), // always insert them first. This ensure that re-export // doc comments show up before the original doc comments // when we render them. - if let Some(inner) = new_attrs { + if let Some((inner, item_id)) = new_attrs { let mut both = inner.to_vec(); both.extend_from_slice(old_attrs); ( - if let Some(new_id) = parent_module { - Attributes::from_ast_with_additional(old_attrs, (inner, new_id)) + if let Some(item_id) = item_id { + Attributes::from_ast_with_additional(old_attrs, (inner, item_id)) } else { Attributes::from_ast(&both) }, @@ -375,9 +347,8 @@ pub(crate) fn merge_attrs( /// Inline an `impl`, inherent or of a trait. The `did` must be for an `impl`. pub(crate) fn build_impl( cx: &mut DocContext<'_>, - parent_module: Option, did: DefId, - attrs: Option<&[ast::Attribute]>, + attrs: Option<(&[ast::Attribute], Option)>, ret: &mut Vec, ) { if !cx.inlined.insert(did.into()) { @@ -539,7 +510,7 @@ pub(crate) fn build_impl( record_extern_trait(cx, did); } - let (merged_attrs, cfg) = merge_attrs(cx, parent_module, load_attrs(cx, did), attrs); + let (merged_attrs, cfg) = merge_attrs(cx, load_attrs(cx, did), attrs); trace!("merged_attrs={:?}", merged_attrs); trace!( @@ -635,7 +606,7 @@ fn build_module_items( cfg: None, inline_stmt_id: None, }); - } else if let Some(i) = try_inline(cx, did, None, res, item.ident.name, None, visited) { + } else if let Some(i) = try_inline(cx, res, item.ident.name, None, visited) { items.extend(i) } } diff --git a/src/librustdoc/clean/mod.rs b/src/librustdoc/clean/mod.rs index 989e091a0d2d8..dfe800050c524 100644 --- a/src/librustdoc/clean/mod.rs +++ b/src/librustdoc/clean/mod.rs @@ -2476,18 +2476,12 @@ fn clean_extern_crate<'tcx>( let krate_owner_def_id = krate.owner_id.to_def_id(); if please_inline { - let mut visited = DefIdSet::default(); - - let res = Res::Def(DefKind::Mod, crate_def_id); - if let Some(items) = inline::try_inline( cx, - cx.tcx.parent_module(krate.hir_id()).to_def_id(), - Some(krate_owner_def_id), - res, + Res::Def(DefKind::Mod, crate_def_id), name, - Some(attrs), - &mut visited, + Some((attrs, Some(krate_owner_def_id))), + &mut Default::default(), ) { return items; } @@ -2611,17 +2605,13 @@ fn clean_use_statement_inner<'tcx>( denied = true; } if !denied { - let mut visited = DefIdSet::default(); let import_def_id = import.owner_id.to_def_id(); - if let Some(mut items) = inline::try_inline( cx, - cx.tcx.parent_module(import.hir_id()).to_def_id(), - Some(import_def_id), path.res, name, - Some(attrs), - &mut visited, + Some((attrs, Some(import_def_id))), + &mut Default::default(), ) { items.push(Item::from_def_id_and_parts( import_def_id, diff --git a/src/librustdoc/clean/types/tests.rs b/src/librustdoc/clean/types/tests.rs index 20627c2cfc164..8f2331785f50d 100644 --- a/src/librustdoc/clean/types/tests.rs +++ b/src/librustdoc/clean/types/tests.rs @@ -10,7 +10,7 @@ use rustc_span::symbol::Symbol; fn create_doc_fragment(s: &str) -> Vec { vec![DocFragment { span: DUMMY_SP, - parent_module: None, + item_id: None, doc: Symbol::intern(s), kind: DocFragmentKind::SugaredDoc, indent: 0, diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs index cafb00df51ed2..cca50df0db213 100644 --- a/src/librustdoc/clean/utils.rs +++ b/src/librustdoc/clean/utils.rs @@ -195,12 +195,12 @@ pub(crate) fn build_deref_target_impls( if let Some(prim) = target.primitive_type() { let _prof_timer = cx.tcx.sess.prof.generic_activity("build_primitive_inherent_impls"); for did in prim.impls(tcx).filter(|did| !did.is_local()) { - inline::build_impl(cx, None, did, None, ret); + inline::build_impl(cx, did, None, ret); } } else if let Type::Path { path } = target { let did = path.def_id(); if !did.is_local() { - inline::build_impls(cx, None, did, None, ret); + inline::build_impls(cx, did, None, ret); } } } diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index 358f6ad566c25..e335cec04d2ec 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -28,7 +28,7 @@ use std::mem; use std::ops::Range; use crate::clean::{self, utils::find_nearest_parent_module}; -use crate::clean::{Crate, Item, ItemId, ItemLink, PrimitiveType}; +use crate::clean::{Crate, Item, ItemLink, PrimitiveType}; use crate::core::DocContext; use crate::html::markdown::{markdown_links, MarkdownLink}; use crate::lint::{BROKEN_INTRA_DOC_LINKS, PRIVATE_INTRA_DOC_LINKS}; @@ -42,8 +42,7 @@ pub(crate) const COLLECT_INTRA_DOC_LINKS: Pass = Pass { }; fn collect_intra_doc_links(krate: Crate, cx: &mut DocContext<'_>) -> Crate { - let mut collector = - LinkCollector { cx, mod_ids: Vec::new(), visited_links: FxHashMap::default() }; + let mut collector = LinkCollector { cx, visited_links: FxHashMap::default() }; collector.visit_crate(&krate); krate } @@ -149,7 +148,7 @@ impl TryFrom for Res { #[derive(Debug)] struct UnresolvedPath<'a> { /// Item on which the link is resolved, used for resolving `Self`. - item_id: ItemId, + item_id: DefId, /// The scope the link was resolved in. module_id: DefId, /// If part of the link resolved, this has the `Res`. @@ -225,7 +224,7 @@ impl UrlFragment { #[derive(Clone, Debug, Hash, PartialEq, Eq)] struct ResolutionInfo { - item_id: ItemId, + item_id: DefId, module_id: DefId, dis: Option, path_str: Box, @@ -242,11 +241,6 @@ struct DiagnosticInfo<'a> { struct LinkCollector<'a, 'tcx> { cx: &'a mut DocContext<'tcx>, - /// A stack of modules used to decide what scope to resolve in. - /// - /// The last module will be used if the parent scope of the current item is - /// unknown. - mod_ids: Vec, /// Cache the resolved links so we can avoid resolving (and emitting errors for) the same link. /// The link will be `None` if it could not be resolved (i.e. the error was cached). visited_links: FxHashMap)>>, @@ -262,7 +256,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { fn variant_field<'path>( &self, path_str: &'path str, - item_id: ItemId, + item_id: DefId, module_id: DefId, ) -> Result<(Res, DefId), UnresolvedPath<'path>> { let tcx = self.cx.tcx; @@ -334,35 +328,33 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { }) } - fn resolve_self_ty(&self, path_str: &str, ns: Namespace, item_id: ItemId) -> Option { + fn resolve_self_ty(&self, path_str: &str, ns: Namespace, item_id: DefId) -> Option { if ns != TypeNS || path_str != "Self" { return None; } let tcx = self.cx.tcx; - item_id - .as_def_id() - .map(|def_id| match tcx.def_kind(def_id) { - def_kind @ (DefKind::AssocFn - | DefKind::AssocConst - | DefKind::AssocTy - | DefKind::Variant - | DefKind::Field) => { - let parent_def_id = tcx.parent(def_id); - if def_kind == DefKind::Field && tcx.def_kind(parent_def_id) == DefKind::Variant - { - tcx.parent(parent_def_id) - } else { - parent_def_id - } + let self_id = match tcx.def_kind(item_id) { + def_kind @ (DefKind::AssocFn + | DefKind::AssocConst + | DefKind::AssocTy + | DefKind::Variant + | DefKind::Field) => { + let parent_def_id = tcx.parent(item_id); + if def_kind == DefKind::Field && tcx.def_kind(parent_def_id) == DefKind::Variant { + tcx.parent(parent_def_id) + } else { + parent_def_id } - _ => def_id, - }) - .and_then(|self_id| match tcx.def_kind(self_id) { - DefKind::Impl { .. } => self.def_id_to_res(self_id), - DefKind::Use => None, - def_kind => Some(Res::Def(def_kind, self_id)), - }) + } + _ => item_id, + }; + + match tcx.def_kind(self_id) { + DefKind::Impl { .. } => self.def_id_to_res(self_id), + DefKind::Use => None, + def_kind => Some(Res::Def(def_kind, self_id)), + } } /// Convenience wrapper around `doc_link_resolutions`. @@ -374,7 +366,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { &self, path_str: &str, ns: Namespace, - item_id: ItemId, + item_id: DefId, module_id: DefId, ) -> Option { if let res @ Some(..) = self.resolve_self_ty(path_str, ns, item_id) { @@ -401,7 +393,7 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> { &mut self, path_str: &'path str, ns: Namespace, - item_id: ItemId, + item_id: DefId, module_id: DefId, ) -> Result<(Res, Option), UnresolvedPath<'path>> { if let Some(res) = self.resolve_path(path_str, ns, item_id, module_id) { @@ -781,48 +773,31 @@ fn is_derive_trait_collision(ns: &PerNS DocVisitor for LinkCollector<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { - let parent_node = - item.item_id.as_def_id().and_then(|did| find_nearest_parent_module(self.cx.tcx, did)); - if parent_node.is_some() { - trace!("got parent node for {:?} {:?}, id {:?}", item.type_(), item.name, item.item_id); - } - - let inner_docs = item.inner_docs(self.cx.tcx); - - if item.is_mod() && inner_docs { - self.mod_ids.push(item.item_id.expect_def_id()); - } - // We want to resolve in the lexical scope of the documentation. // In the presence of re-exports, this is not the same as the module of the item. // Rather than merging all documentation into one, resolve it one attribute at a time // so we know which module it came from. - for (parent_module, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings) { + for (item_id, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings) { if !may_have_doc_links(&doc) { continue; } debug!("combined_docs={}", doc); // NOTE: if there are links that start in one crate and end in another, this will not resolve them. // This is a degenerate case and it's not supported by rustdoc. - let parent_node = parent_module.or(parent_node); + let item_id = item_id.unwrap_or_else(|| item.item_id.expect_def_id()); + let module_id = match self.cx.tcx.def_kind(item_id) { + DefKind::Mod if item.inner_docs(self.cx.tcx) => item_id, + _ => find_nearest_parent_module(self.cx.tcx, item_id).unwrap(), + }; for md_link in preprocessed_markdown_links(&doc) { - let link = self.resolve_link(item, &doc, parent_node, &md_link); + let link = self.resolve_link(item, item_id, module_id, &doc, &md_link); if let Some(link) = link { self.cx.cache.intra_doc_links.entry(item.item_id).or_default().push(link); } } } - if item.is_mod() { - if !inner_docs { - self.mod_ids.push(item.item_id.expect_def_id()); - } - - self.visit_item_recur(item); - self.mod_ids.pop(); - } else { - self.visit_item_recur(item) - } + self.visit_item_recur(item) } } @@ -954,8 +929,9 @@ impl LinkCollector<'_, '_> { fn resolve_link( &mut self, item: &Item, + item_id: DefId, + module_id: DefId, dox: &str, - parent_node: Option, link: &PreprocessedMarkdownLink, ) -> Option { let PreprocessedMarkdownLink(pp_link, ori_link) = link; @@ -972,25 +948,9 @@ impl LinkCollector<'_, '_> { pp_link.as_ref().map_err(|err| err.report(self.cx, diag_info.clone())).ok()?; let disambiguator = *disambiguator; - // In order to correctly resolve intra-doc links we need to - // pick a base AST node to work from. If the documentation for - // this module came from an inner comment (//!) then we anchor - // our name resolution *inside* the module. If, on the other - // hand it was an outer comment (///) then we anchor the name - // resolution in the parent module on the basis that the names - // used are more likely to be intended to be parent names. For - // this, we set base_node to None for inner comments since - // we've already pushed this node onto the resolution stack but - // for outer comments we explicitly try and resolve against the - // parent_node first. - let inner_docs = item.inner_docs(self.cx.tcx); - let base_node = - if item.is_mod() && inner_docs { self.mod_ids.last().copied() } else { parent_node }; - let module_id = base_node.expect("doc link without parent module"); - let (mut res, fragment) = self.resolve_with_disambiguator_cached( ResolutionInfo { - item_id: item.item_id, + item_id, module_id, dis: disambiguator, path_str: path_str.clone(), @@ -1231,11 +1191,11 @@ impl LinkCollector<'_, '_> { let disambiguator = key.dis; let path_str = &key.path_str; let item_id = key.item_id; - let base_node = key.module_id; + let module_id = key.module_id; match disambiguator.map(Disambiguator::ns) { Some(expected_ns) => { - match self.resolve(path_str, expected_ns, item_id, base_node) { + match self.resolve(path_str, expected_ns, item_id, module_id) { Ok(res) => Some(res), Err(err) => { // We only looked in one namespace. Try to give a better error if possible. @@ -1245,7 +1205,7 @@ impl LinkCollector<'_, '_> { for other_ns in [TypeNS, ValueNS, MacroNS] { if other_ns != expected_ns { if let Ok(res) = - self.resolve(path_str, other_ns, item_id, base_node) + self.resolve(path_str, other_ns, item_id, module_id) { err = ResolutionFailure::WrongNamespace { res: full_res(self.cx.tcx, res), @@ -1262,7 +1222,7 @@ impl LinkCollector<'_, '_> { None => { // Try everything! let mut candidate = |ns| { - self.resolve(path_str, ns, item_id, base_node) + self.resolve(path_str, ns, item_id, module_id) .map_err(ResolutionFailure::NotResolved) }; diff --git a/src/librustdoc/passes/collect_trait_impls.rs b/src/librustdoc/passes/collect_trait_impls.rs index d32e8185d3f96..8d204ddb79e39 100644 --- a/src/librustdoc/passes/collect_trait_impls.rs +++ b/src/librustdoc/passes/collect_trait_impls.rs @@ -49,7 +49,7 @@ pub(crate) fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) -> let _prof_timer = cx.tcx.sess.prof.generic_activity("build_extern_trait_impls"); for &cnum in cx.tcx.crates(()) { for &impl_def_id in cx.tcx.trait_impls_in_crate(cnum) { - inline::build_impl(cx, None, impl_def_id, None, &mut new_items_external); + inline::build_impl(cx, impl_def_id, None, &mut new_items_external); } } } @@ -75,7 +75,7 @@ pub(crate) fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) -> ); parent = cx.tcx.opt_parent(did); } - inline::build_impl(cx, None, impl_def_id, Some(&attr_buf), &mut new_items_local); + inline::build_impl(cx, impl_def_id, Some((&attr_buf, None)), &mut new_items_local); attr_buf.clear(); } } @@ -84,7 +84,7 @@ pub(crate) fn collect_trait_impls(mut krate: Crate, cx: &mut DocContext<'_>) -> for def_id in PrimitiveType::all_impls(cx.tcx) { // Try to inline primitive impls from other crates. if !def_id.is_local() { - inline::build_impl(cx, None, def_id, None, &mut new_items_external); + inline::build_impl(cx, def_id, None, &mut new_items_external); } } for (prim, did) in PrimitiveType::primitive_locations(cx.tcx) { diff --git a/src/librustdoc/passes/propagate_doc_cfg.rs b/src/librustdoc/passes/propagate_doc_cfg.rs index f35643af63738..8a33e51b3beb1 100644 --- a/src/librustdoc/passes/propagate_doc_cfg.rs +++ b/src/librustdoc/passes/propagate_doc_cfg.rs @@ -57,7 +57,8 @@ impl<'a, 'tcx> CfgPropagator<'a, 'tcx> { next_def_id = parent_def_id; } - let (_, cfg) = merge_attrs(self.cx, None, item.attrs.other_attrs.as_slice(), Some(&attrs)); + let (_, cfg) = + merge_attrs(self.cx, item.attrs.other_attrs.as_slice(), Some((&attrs, None))); item.cfg = cfg; } } diff --git a/tests/rustdoc-ui/intra-doc/auxiliary/inner-crate-doc.rs b/tests/rustdoc-ui/intra-doc/auxiliary/inner-crate-doc.rs new file mode 100644 index 0000000000000..15bf51e6f8e2b --- /dev/null +++ b/tests/rustdoc-ui/intra-doc/auxiliary/inner-crate-doc.rs @@ -0,0 +1 @@ +//! Inner doc comment diff --git a/tests/rustdoc-ui/intra-doc/import-inline-merge-module.rs b/tests/rustdoc-ui/intra-doc/import-inline-merge-module.rs new file mode 100644 index 0000000000000..4d6a325664578 --- /dev/null +++ b/tests/rustdoc-ui/intra-doc/import-inline-merge-module.rs @@ -0,0 +1,10 @@ +// Test for issue #108501. +// Module parent scope doesn't hijack import's parent scope for the import's doc links. + +// check-pass +// aux-build: inner-crate-doc.rs +// compile-flags: --extern inner_crate_doc --edition 2018 + +/// Import doc comment [inner_crate_doc] +#[doc(inline)] +pub use inner_crate_doc; From ae4781081a56f7c5ebcce69c23f400f85567cc65 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Sun, 19 Mar 2023 20:53:39 +0400 Subject: [PATCH 16/26] rustdoc: Factor out some doc link resolution code into a separate function --- .../passes/collect_intra_doc_links.rs | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/librustdoc/passes/collect_intra_doc_links.rs b/src/librustdoc/passes/collect_intra_doc_links.rs index e335cec04d2ec..ec9de8a43ee3d 100644 --- a/src/librustdoc/passes/collect_intra_doc_links.rs +++ b/src/librustdoc/passes/collect_intra_doc_links.rs @@ -773,30 +773,7 @@ fn is_derive_trait_collision(ns: &PerNS DocVisitor for LinkCollector<'a, 'tcx> { fn visit_item(&mut self, item: &Item) { - // We want to resolve in the lexical scope of the documentation. - // In the presence of re-exports, this is not the same as the module of the item. - // Rather than merging all documentation into one, resolve it one attribute at a time - // so we know which module it came from. - for (item_id, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings) { - if !may_have_doc_links(&doc) { - continue; - } - debug!("combined_docs={}", doc); - // NOTE: if there are links that start in one crate and end in another, this will not resolve them. - // This is a degenerate case and it's not supported by rustdoc. - let item_id = item_id.unwrap_or_else(|| item.item_id.expect_def_id()); - let module_id = match self.cx.tcx.def_kind(item_id) { - DefKind::Mod if item.inner_docs(self.cx.tcx) => item_id, - _ => find_nearest_parent_module(self.cx.tcx, item_id).unwrap(), - }; - for md_link in preprocessed_markdown_links(&doc) { - let link = self.resolve_link(item, item_id, module_id, &doc, &md_link); - if let Some(link) = link { - self.cx.cache.intra_doc_links.entry(item.item_id).or_default().push(link); - } - } - } - + self.resolve_links(item); self.visit_item_recur(item) } } @@ -923,6 +900,32 @@ fn preprocessed_markdown_links(s: &str) -> Vec { } impl LinkCollector<'_, '_> { + fn resolve_links(&mut self, item: &Item) { + // We want to resolve in the lexical scope of the documentation. + // In the presence of re-exports, this is not the same as the module of the item. + // Rather than merging all documentation into one, resolve it one attribute at a time + // so we know which module it came from. + for (item_id, doc) in prepare_to_doc_link_resolution(&item.attrs.doc_strings) { + if !may_have_doc_links(&doc) { + continue; + } + debug!("combined_docs={}", doc); + // NOTE: if there are links that start in one crate and end in another, this will not resolve them. + // This is a degenerate case and it's not supported by rustdoc. + let item_id = item_id.unwrap_or_else(|| item.item_id.expect_def_id()); + let module_id = match self.cx.tcx.def_kind(item_id) { + DefKind::Mod if item.inner_docs(self.cx.tcx) => item_id, + _ => find_nearest_parent_module(self.cx.tcx, item_id).unwrap(), + }; + for md_link in preprocessed_markdown_links(&doc) { + let link = self.resolve_link(item, item_id, module_id, &doc, &md_link); + if let Some(link) = link { + self.cx.cache.intra_doc_links.entry(item.item_id).or_default().push(link); + } + } + } + } + /// This is the entry point for resolving an intra-doc link. /// /// FIXME(jynelson): this is way too many arguments From 69a82f7f343f6fec675c1ed818103d10dd0ff1fc Mon Sep 17 00:00:00 2001 From: Alona Enraght-Moony Date: Sun, 19 Mar 2023 23:11:08 +0000 Subject: [PATCH 17/26] add myself to mailmap --- .mailmap | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.mailmap b/.mailmap index 715bc4d30855e..9148b79e98016 100644 --- a/.mailmap +++ b/.mailmap @@ -29,6 +29,8 @@ Alexander Ronald Altman Alexandre Martin Alexis Beingessner Alfie John Alfie John +Alona Enraght-Moony +Alona Enraght-Moony Amos Onn Ana-Maria Mihalache Anatoly Ikorsky @@ -415,7 +417,6 @@ Nicolas Abram Nicole Mazzuca Nif Ward Nika Layzell -Nixon Enraght-Moony NODA Kai oliver <16816606+o752d@users.noreply.github.com> Oliver Middleton From 9f80c75703a7497241d9d1d072969f44f136597c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 16 Mar 2023 23:27:09 +0000 Subject: [PATCH 18/26] Walk un-shifted nested `impl Trait` in trait when setting up default trait method assumptions --- compiler/rustc_ty_utils/src/ty.rs | 42 ++++++++++++------- .../default-method-binder-shifting.rs | 6 +++ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/compiler/rustc_ty_utils/src/ty.rs b/compiler/rustc_ty_utils/src/ty.rs index 9fed1e57c9213..f53952d25fadd 100644 --- a/compiler/rustc_ty_utils/src/ty.rs +++ b/compiler/rustc_ty_utils/src/ty.rs @@ -7,7 +7,10 @@ use rustc_middle::ty::{ TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor, }; use rustc_session::config::TraitSolver; -use rustc_span::def_id::{DefId, CRATE_DEF_ID}; +use rustc_span::{ + def_id::{DefId, CRATE_DEF_ID}, + DUMMY_SP, +}; use rustc_trait_selection::traits; fn sized_constraint_for_ty<'tcx>( @@ -275,16 +278,22 @@ impl<'tcx> TypeVisitor> for ImplTraitInTraitFinder<'_, 'tcx> { } fn visit_ty(&mut self, ty: Ty<'tcx>) -> std::ops::ControlFlow { - if let ty::Alias(ty::Projection, alias_ty) = *ty.kind() - && self.tcx.is_impl_trait_in_trait(alias_ty.def_id) - && self.tcx.impl_trait_in_trait_parent_fn(alias_ty.def_id) == self.fn_def_id - && self.seen.insert(alias_ty.def_id) + if let ty::Alias(ty::Projection, unshifted_alias_ty) = *ty.kind() + && self.tcx.is_impl_trait_in_trait(unshifted_alias_ty.def_id) + && self.tcx.impl_trait_in_trait_parent_fn(unshifted_alias_ty.def_id) == self.fn_def_id + && self.seen.insert(unshifted_alias_ty.def_id) { // We have entered some binders as we've walked into the // bounds of the RPITIT. Shift these binders back out when // constructing the top-level projection predicate. - let alias_ty = self.tcx.fold_regions(alias_ty, |re, _| { + let shifted_alias_ty = self.tcx.fold_regions(unshifted_alias_ty, |re, depth| { if let ty::ReLateBound(index, bv) = re.kind() { + if depth != ty::INNERMOST { + return self.tcx.mk_re_error_with_message( + DUMMY_SP, + "we shouldn't walk non-predicate binders with `impl Trait`...", + ); + } self.tcx.mk_re_late_bound(index.shifted_out_to_binder(self.depth), bv) } else { re @@ -295,26 +304,27 @@ impl<'tcx> TypeVisitor> for ImplTraitInTraitFinder<'_, 'tcx> { // the `type_of` of the trait's associated item. If we're using the old lowering // strategy, then just reinterpret the associated type like an opaque :^) let default_ty = if self.tcx.lower_impl_trait_in_trait_to_assoc_ty() { - self - .tcx - .type_of(alias_ty.def_id) - .subst(self.tcx, alias_ty.substs) + self.tcx.type_of(shifted_alias_ty.def_id).subst(self.tcx, shifted_alias_ty.substs) } else { - self.tcx.mk_alias(ty::Opaque, alias_ty) + self.tcx.mk_alias(ty::Opaque, shifted_alias_ty) }; self.predicates.push( ty::Binder::bind_with_vars( - ty::ProjectionPredicate { - projection_ty: alias_ty, - term: default_ty.into(), - }, + ty::ProjectionPredicate { projection_ty: shifted_alias_ty, term: default_ty.into() }, self.bound_vars, ) .to_predicate(self.tcx), ); - for bound in self.tcx.item_bounds(alias_ty.def_id).subst_iter(self.tcx, alias_ty.substs) + // We walk the *un-shifted* alias ty, because we're tracking the de bruijn + // binder depth, and if we were to walk `shifted_alias_ty` instead, we'd + // have to reset `self.depth` back to `ty::INNERMOST` or something. It's + // easier to just do this. + for bound in self + .tcx + .item_bounds(unshifted_alias_ty.def_id) + .subst_iter(self.tcx, unshifted_alias_ty.substs) { bound.visit_with(self); } diff --git a/tests/ui/impl-trait/in-trait/default-method-binder-shifting.rs b/tests/ui/impl-trait/in-trait/default-method-binder-shifting.rs index 75b0ec939847a..de82544f29338 100644 --- a/tests/ui/impl-trait/in-trait/default-method-binder-shifting.rs +++ b/tests/ui/impl-trait/in-trait/default-method-binder-shifting.rs @@ -13,4 +13,10 @@ trait Trait { fn method(&self) -> impl Trait; } +trait Trait2 { + type Type; + + fn method(&self) -> impl Trait2 + '_>; +} + fn main() {} From 239ec6cb11bf66d47fbc409ed8fca58370feb517 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 17 Mar 2023 00:50:47 +0000 Subject: [PATCH 19/26] drive-by: Fix a comment in TyCtxt::fold_regions and remove an unused method --- compiler/rustc_middle/src/ty/fold.rs | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/compiler/rustc_middle/src/ty/fold.rs b/compiler/rustc_middle/src/ty/fold.rs index d66f436f947a3..6205e2bf24dd1 100644 --- a/compiler/rustc_middle/src/ty/fold.rs +++ b/compiler/rustc_middle/src/ty/fold.rs @@ -51,9 +51,7 @@ where // Region folder impl<'tcx> TyCtxt<'tcx> { - /// Folds the escaping and free regions in `value` using `f`, and - /// sets `skipped_regions` to true if any late-bound region was found - /// and skipped. + /// Folds the escaping and free regions in `value` using `f`. pub fn fold_regions( self, value: T, @@ -64,17 +62,6 @@ impl<'tcx> TyCtxt<'tcx> { { value.fold_with(&mut RegionFolder::new(self, &mut f)) } - - pub fn super_fold_regions( - self, - value: T, - mut f: impl FnMut(ty::Region<'tcx>, ty::DebruijnIndex) -> ty::Region<'tcx>, - ) -> T - where - T: TypeSuperFoldable>, - { - value.super_fold_with(&mut RegionFolder::new(self, &mut f)) - } } /// Folds over the substructure of a type, visiting its component From 5b4fa5bf989e4d630512998789f4bb79f1d9f134 Mon Sep 17 00:00:00 2001 From: lcnr Date: Mon, 20 Mar 2023 10:46:43 +0100 Subject: [PATCH 20/26] fix typo --- compiler/rustc_hir_analysis/src/collect/type_of.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_hir_analysis/src/collect/type_of.rs b/compiler/rustc_hir_analysis/src/collect/type_of.rs index fe44fabf57df9..b5e8de3c9a7d2 100644 --- a/compiler/rustc_hir_analysis/src/collect/type_of.rs +++ b/compiler/rustc_hir_analysis/src/collect/type_of.rs @@ -593,7 +593,7 @@ fn find_opaque_ty_constraints_for_tait(tcx: TyCtxt<'_>, def_id: LocalDefId) -> T found: Option>, /// In the presence of dead code, typeck may figure out a hidden type - /// while borrowck will now. We collect these cases here and check at + /// while borrowck will not. We collect these cases here and check at /// the end that we actually found a type that matches (modulo regions). typeck_types: Vec>, } From 8e4e55e5246492e7add98ccf24f2f483163cdbf2 Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Mon, 20 Mar 2023 12:32:38 +0100 Subject: [PATCH 21/26] Support aggregate expressions --- .../src/build/custom/parse/instruction.rs | 25 +++++++ .../aggregate_exprs.adt.built.after.mir | 16 +++++ .../aggregate_exprs.array.built.after.mir | 15 ++++ .../building/custom/aggregate_exprs.rs | 71 +++++++++++++++++++ .../aggregate_exprs.tuple.built.after.mir | 10 +++ 5 files changed, 137 insertions(+) create mode 100644 tests/mir-opt/building/custom/aggregate_exprs.adt.built.after.mir create mode 100644 tests/mir-opt/building/custom/aggregate_exprs.array.built.after.mir create mode 100644 tests/mir-opt/building/custom/aggregate_exprs.rs create mode 100644 tests/mir-opt/building/custom/aggregate_exprs.tuple.built.after.mir diff --git a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs index 5e77f2dc1268d..e60e1a4c81507 100644 --- a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs +++ b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs @@ -166,6 +166,31 @@ impl<'tcx, 'body> ParseCtxt<'tcx, 'body> { let cast_kind = mir_cast_kind(source_ty, expr.ty); Ok(Rvalue::Cast(cast_kind, source, expr.ty)) }, + ExprKind::Tuple { fields } => Ok( + Rvalue::Aggregate( + Box::new(AggregateKind::Tuple), + fields.iter().map(|e| self.parse_operand(*e)).collect::>()? + ) + ), + ExprKind::Array { fields } => { + let elem_ty = match expr.ty.kind() { + ty::Array(ty, ..) => ty, + _ => unreachable!("ty is array"), + }; + Ok(Rvalue::Aggregate( + Box::new(AggregateKind::Array(*elem_ty)), + fields.iter().map(|e| self.parse_operand(*e)).collect::>()? + )) + }, + ExprKind::Adt(box AdtExpr{ adt_def, variant_index, substs, fields, .. }) => { + let is_union = adt_def.is_union(); + let active_field_index = is_union.then(|| fields[0].name.index()); + + Ok(Rvalue::Aggregate( + Box::new(AggregateKind::Adt(adt_def.did(), *variant_index, substs, None, active_field_index)), + fields.iter().map(|f| self.parse_operand(f.expr)).collect::>()? + )) + }, _ => self.parse_operand(expr_id).map(Rvalue::Use), ) } diff --git a/tests/mir-opt/building/custom/aggregate_exprs.adt.built.after.mir b/tests/mir-opt/building/custom/aggregate_exprs.adt.built.after.mir new file mode 100644 index 0000000000000..49e8c812c197a --- /dev/null +++ b/tests/mir-opt/building/custom/aggregate_exprs.adt.built.after.mir @@ -0,0 +1,16 @@ +// MIR for `adt` after built + +fn adt() -> Onion { + let mut _0: Onion; // return place in scope 0 at $DIR/aggregate_exprs.rs:+0:13: +0:18 + let mut _1: i32; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + let mut _2: Foo; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + let mut _3: Bar; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + + bb0: { + _1 = const 1_i32; // scope 0 at $DIR/aggregate_exprs.rs:+6:13: +6:20 + _2 = Foo { a: const 1_i32, b: const 2_i32 }; // scope 0 at $DIR/aggregate_exprs.rs:+7:13: +10:14 + _3 = Bar::Foo(move _2, _1); // scope 0 at $DIR/aggregate_exprs.rs:+11:13: +11:39 + _0 = Onion { neon: ((_3 as variant#0).1: i32) }; // scope 0 at $DIR/aggregate_exprs.rs:+12:13: +12:58 + return; // scope 0 at $DIR/aggregate_exprs.rs:+13:13: +13:21 + } +} diff --git a/tests/mir-opt/building/custom/aggregate_exprs.array.built.after.mir b/tests/mir-opt/building/custom/aggregate_exprs.array.built.after.mir new file mode 100644 index 0000000000000..30d12897331ce --- /dev/null +++ b/tests/mir-opt/building/custom/aggregate_exprs.array.built.after.mir @@ -0,0 +1,15 @@ +// MIR for `array` after built + +fn array() -> [i32; 2] { + let mut _0: [i32; 2]; // return place in scope 0 at $DIR/aggregate_exprs.rs:+0:15: +0:23 + let mut _1: [i32; 2]; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + let mut _2: i32; // in scope 0 at $SRC_DIR/core/src/intrinsics/mir.rs:LL:COL + + bb0: { + _1 = [const 42_i32, const 43_i32]; // scope 0 at $DIR/aggregate_exprs.rs:+5:13: +5:25 + _2 = const 1_i32; // scope 0 at $DIR/aggregate_exprs.rs:+6:13: +6:20 + _1 = [_2, const 2_i32]; // scope 0 at $DIR/aggregate_exprs.rs:+7:13: +7:25 + _0 = move _1; // scope 0 at $DIR/aggregate_exprs.rs:+8:13: +8:26 + return; // scope 0 at $DIR/aggregate_exprs.rs:+9:13: +9:21 + } +} diff --git a/tests/mir-opt/building/custom/aggregate_exprs.rs b/tests/mir-opt/building/custom/aggregate_exprs.rs new file mode 100644 index 0000000000000..554c9c03ba4a0 --- /dev/null +++ b/tests/mir-opt/building/custom/aggregate_exprs.rs @@ -0,0 +1,71 @@ +#![feature(custom_mir, core_intrinsics)] + +extern crate core; +use core::intrinsics::mir::*; + +// EMIT_MIR aggregate_exprs.tuple.built.after.mir +#[custom_mir(dialect = "built")] +fn tuple() -> (i32, bool) { + mir!( + { + RET = (1, true); + Return() + } + ) +} + +// EMIT_MIR aggregate_exprs.array.built.after.mir +#[custom_mir(dialect = "built")] +fn array() -> [i32; 2] { + mir!( + let x: [i32; 2]; + let one: i32; + { + x = [42, 43]; + one = 1; + x = [one, 2]; + RET = Move(x); + Return() + } + ) +} + +struct Foo { + a: i32, + b: i32, +} + +enum Bar { + Foo(Foo, i32), +} + +union Onion { + neon: i32, + noun: f32, +} + +// EMIT_MIR aggregate_exprs.adt.built.after.mir +#[custom_mir(dialect = "built")] +fn adt() -> Onion { + mir!( + let one: i32; + let x: Foo; + let y: Bar; + { + one = 1; + x = Foo { + a: 1, + b: 2, + }; + y = Bar::Foo(Move(x), one); + RET = Onion { neon: Field(Variant(y, 0), 1) }; + Return() + } + ) +} + +fn main() { + assert_eq!(tuple(), (1, true)); + assert_eq!(array(), [1, 2]); + assert_eq!(unsafe { adt().neon }, 1); +} diff --git a/tests/mir-opt/building/custom/aggregate_exprs.tuple.built.after.mir b/tests/mir-opt/building/custom/aggregate_exprs.tuple.built.after.mir new file mode 100644 index 0000000000000..5fe45ccc90ca6 --- /dev/null +++ b/tests/mir-opt/building/custom/aggregate_exprs.tuple.built.after.mir @@ -0,0 +1,10 @@ +// MIR for `tuple` after built + +fn tuple() -> (i32, bool) { + let mut _0: (i32, bool); // return place in scope 0 at $DIR/aggregate_exprs.rs:+0:15: +0:26 + + bb0: { + _0 = (const 1_i32, const true); // scope 0 at $DIR/aggregate_exprs.rs:+3:13: +3:28 + return; // scope 0 at $DIR/aggregate_exprs.rs:+4:13: +4:21 + } +} From e24f5ac56b001d205618590b0519137abbc65580 Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Mon, 20 Mar 2023 15:27:40 +0100 Subject: [PATCH 22/26] Fix off-by-one in mir syntax doc --- compiler/rustc_middle/src/mir/syntax.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index b16b6616415aa..a702a6b9ee1b6 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -1165,7 +1165,7 @@ pub enum AggregateKind<'tcx> { Tuple, /// The second field is the variant index. It's equal to 0 for struct - /// and union expressions. The fourth field is + /// and union expressions. The last field is the /// active field number and is present only for union expressions /// -- e.g., for a union expression `SomeUnion { c: .. }`, the /// active field index would identity the field `c` From f404f33c2197c534cf2468ea7b929e73c9b5d4b7 Mon Sep 17 00:00:00 2001 From: Andy Wang Date: Mon, 20 Mar 2023 16:21:43 +0100 Subject: [PATCH 23/26] Use builtin_index instead of match Co-authored-by: Oli Scherer --- .../rustc_mir_build/src/build/custom/parse/instruction.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs index e60e1a4c81507..adbd37a7cd950 100644 --- a/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs +++ b/compiler/rustc_mir_build/src/build/custom/parse/instruction.rs @@ -173,12 +173,9 @@ impl<'tcx, 'body> ParseCtxt<'tcx, 'body> { ) ), ExprKind::Array { fields } => { - let elem_ty = match expr.ty.kind() { - ty::Array(ty, ..) => ty, - _ => unreachable!("ty is array"), - }; + let elem_ty = expr.ty.builtin_index().expect("ty must be an array"); Ok(Rvalue::Aggregate( - Box::new(AggregateKind::Array(*elem_ty)), + Box::new(AggregateKind::Array(elem_ty)), fields.iter().map(|e| self.parse_operand(*e)).collect::>()? )) }, From e4a4064480c34df9776bcb7c739b17b6b04dc6e7 Mon Sep 17 00:00:00 2001 From: Krasimir Georgiev Date: Mon, 20 Mar 2023 14:39:35 +0000 Subject: [PATCH 24/26] adapt tests/codegen/vec-shrink-panik for LLVM 17 After https://github.com/llvm/llvm-project/commit/0d4a709bb876824a0afa5f86e138e8ffdcaf7661 LLVM now doesn't generate references to panic_cannot_unwind: @nikic: https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/a.20couple.20codegen.20test.20failures.20after.20llvm.200d4a709bb876824a/near/342664944 >Okay, so LLVM now realizes that double panic is not possible, so that's fine. --- tests/codegen/vec-shrink-panik.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/tests/codegen/vec-shrink-panik.rs b/tests/codegen/vec-shrink-panik.rs index aa6589dc35bb1..b3c3483fea982 100644 --- a/tests/codegen/vec-shrink-panik.rs +++ b/tests/codegen/vec-shrink-panik.rs @@ -1,3 +1,8 @@ +// revisions: old new +// LLVM 17 realizes double panic is not possible and doesn't generate calls +// to panic_cannot_unwind. +// [old]ignore-llvm-version: 17 - 99 +// [new]min-llvm-version: 17 // compile-flags: -O // ignore-debug: the debug assertions get in the way #![crate_type = "lib"] @@ -18,11 +23,11 @@ pub fn shrink_to_fit(vec: &mut Vec) { pub fn issue71861(vec: Vec) -> Box<[u32]> { // CHECK-NOT: panic - // Call to panic_cannot_unwind in case of double-panic is expected, - // but other panics are not. + // Call to panic_cannot_unwind in case of double-panic is expected + // on LLVM 16 and older, but other panics are not. // CHECK: cleanup - // CHECK-NEXT: ; call core::panicking::panic_cannot_unwind - // CHECK-NEXT: panic_cannot_unwind + // old-NEXT: ; call core::panicking::panic_cannot_unwind + // old-NEXT: panic_cannot_unwind // CHECK-NOT: panic vec.into_boxed_slice() @@ -34,14 +39,14 @@ pub fn issue75636<'a>(iter: &[&'a str]) -> Box<[&'a str]> { // CHECK-NOT: panic // Call to panic_cannot_unwind in case of double-panic is expected, - // but other panics are not. + // on LLVM 16 and older, but other panics are not. // CHECK: cleanup - // CHECK-NEXT: ; call core::panicking::panic_cannot_unwind - // CHECK-NEXT: panic_cannot_unwind + // old-NEXT: ; call core::panicking::panic_cannot_unwind + // old-NEXT: panic_cannot_unwind // CHECK-NOT: panic iter.iter().copied().collect() } -// CHECK: ; core::panicking::panic_cannot_unwind -// CHECK: declare void @{{.*}}panic_cannot_unwind +// old: ; core::panicking::panic_cannot_unwind +// old: declare void @{{.*}}panic_cannot_unwind From 5058cc8e62f1557f684d90ff0dde7cedc6c5d529 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 20 Mar 2023 19:36:01 +0100 Subject: [PATCH 25/26] not *all* retags might be explicit in Runtime MIR --- compiler/rustc_middle/src/mir/syntax.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs index b16b6616415aa..f25577de6a452 100644 --- a/compiler/rustc_middle/src/mir/syntax.rs +++ b/compiler/rustc_middle/src/mir/syntax.rs @@ -78,7 +78,8 @@ pub enum MirPhase { /// MIR, this is UB. /// - Retags: If `-Zmir-emit-retag` is enabled, analysis MIR has "implicit" retags in the same way /// that Rust itself has them. Where exactly these are is generally subject to change, and so we - /// don't document this here. Runtime MIR has all retags explicit. + /// don't document this here. Runtime MIR has most retags explicit (though implicit retags + /// can still occur at `Rvalue::{Ref,AddrOf}`). /// - Generator bodies: In analysis MIR, locals may actually be behind a pointer that user code has /// access to. This occurs in generator bodies. Such locals do not behave like other locals, /// because they eg may be aliased in surprising ways. Runtime MIR has no such special locals - From 93eeb127241928a2c64b0bd8c81d5ed859b864aa Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 1 Feb 2023 16:47:04 +1100 Subject: [PATCH 26/26] Refactor `handle_missing_lit`. --- compiler/rustc_parse/src/parser/expr.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_parse/src/parser/expr.rs b/compiler/rustc_parse/src/parser/expr.rs index 296eb4d653cdd..8b69b3cb03683 100644 --- a/compiler/rustc_parse/src/parser/expr.rs +++ b/compiler/rustc_parse/src/parser/expr.rs @@ -1843,20 +1843,14 @@ impl<'a> Parser<'a> { &mut self, mk_lit_char: impl FnOnce(Symbol, Span) -> L, ) -> PResult<'a, L> { - if let token::Interpolated(inner) = &self.token.kind { - let expr = match inner.as_ref() { - token::NtExpr(expr) => Some(expr), - token::NtLiteral(expr) => Some(expr), - _ => None, - }; - if let Some(expr) = expr { - if matches!(expr.kind, ExprKind::Err) { - let mut err = errors::InvalidInterpolatedExpression { span: self.token.span } - .into_diagnostic(&self.sess.span_diagnostic); - err.downgrade_to_delayed_bug(); - return Err(err); - } - } + if let token::Interpolated(nt) = &self.token.kind + && let token::NtExpr(e) | token::NtLiteral(e) = &**nt + && matches!(e.kind, ExprKind::Err) + { + let mut err = errors::InvalidInterpolatedExpression { span: self.token.span } + .into_diagnostic(&self.sess.span_diagnostic); + err.downgrade_to_delayed_bug(); + return Err(err); } let token = self.token.clone(); let err = |self_: &Self| {