From 83bbb551e85153b779d5b759c9b4e6bcdbaeee04 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 29 Feb 2024 12:53:33 +0300 Subject: [PATCH 1/4] run change tracker even when config parse fails Please note that we are currently validating the build configuration on two entry points (e.g., profile validation is handled on the python side), and change-tracker system is handled on the rust side. Once #94829 is completed (scheduled for 2024), we will be able to handle this more effectively. Signed-off-by: onur-ozkan --- src/bootstrap/src/bin/main.rs | 12 +++------ src/bootstrap/src/core/config/config.rs | 31 ++++++++++++++++++++--- src/bootstrap/src/lib.rs | 4 ++- src/bootstrap/src/utils/change_tracker.rs | 14 ++++++++++ 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/bootstrap/src/bin/main.rs b/src/bootstrap/src/bin/main.rs index 070d951dba99a..d9b8953280099 100644 --- a/src/bootstrap/src/bin/main.rs +++ b/src/bootstrap/src/bin/main.rs @@ -14,7 +14,8 @@ use std::{ }; use bootstrap::{ - find_recent_config_change_ids, t, Build, Config, Subcommand, CONFIG_CHANGE_HISTORY, + find_recent_config_change_ids, human_readable_changes, t, Build, Config, Subcommand, + CONFIG_CHANGE_HISTORY, }; fn main() { @@ -154,14 +155,7 @@ fn check_version(config: &Config) -> Option { } msg.push_str("There have been changes to x.py since you last updated:\n"); - - for change in changes { - msg.push_str(&format!(" [{}] {}\n", change.severity, change.summary)); - msg.push_str(&format!( - " - PR Link https://github.com/rust-lang/rust/pull/{}\n", - change.change_id - )); - } + msg.push_str(&human_readable_changes(&changes)); msg.push_str("NOTE: to silence this warning, "); msg.push_str(&format!( diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 875a4efae02fc..24a085ab40087 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -600,7 +600,8 @@ impl Target { #[serde(deny_unknown_fields, rename_all = "kebab-case")] pub(crate) struct TomlConfig { changelog_seen: Option, // FIXME: Deprecated field. Remove it at 2024. - change_id: Option, + #[serde(flatten)] + change_id: ChangeIdWrapper, build: Option, install: Option, llvm: Option, @@ -610,6 +611,16 @@ pub(crate) struct TomlConfig { profile: Option, } +/// Since we use `#[serde(deny_unknown_fields)]` on `TomlConfig`, we need a wrapper type +/// for the "change-id" field to parse it even if other fields are invalid. This ensures +/// that if deserialization fails due to other fields, we can still provide the changelogs +/// to allow developers to potentially find the reason for the failure in the logs.. +#[derive(Deserialize, Default)] +pub(crate) struct ChangeIdWrapper { + #[serde(alias = "change-id")] + inner: Option, +} + /// Describes how to handle conflicts in merging two [`TomlConfig`] #[derive(Copy, Clone, Debug)] enum ReplaceOpt { @@ -651,7 +662,7 @@ impl Merge for TomlConfig { } } self.changelog_seen.merge(changelog_seen, replace); - self.change_id.merge(change_id, replace); + self.change_id.inner.merge(change_id.inner, replace); do_merge(&mut self.build, build, replace); do_merge(&mut self.install, install, replace); do_merge(&mut self.llvm, llvm, replace); @@ -1200,6 +1211,20 @@ impl Config { toml::from_str(&contents) .and_then(|table: toml::Value| TomlConfig::deserialize(table)) .unwrap_or_else(|err| { + if let Ok(Some(changes)) = toml::from_str(&contents) + .and_then(|table: toml::Value| ChangeIdWrapper::deserialize(table)) + .and_then(|change_id| { + Ok(change_id.inner.map(|id| crate::find_recent_config_change_ids(id))) + }) + { + if !changes.is_empty() { + println!( + "WARNING: There have been changes to x.py since you last updated:\n{}", + crate::human_readable_changes(&changes) + ); + } + } + eprintln!("failed to parse TOML configuration '{}': {err}", file.display()); exit!(2); }) @@ -1366,7 +1391,7 @@ impl Config { toml.merge(override_toml, ReplaceOpt::Override); config.changelog_seen = toml.changelog_seen; - config.change_id = toml.change_id; + config.change_id = toml.change_id.inner; let Build { build, diff --git a/src/bootstrap/src/lib.rs b/src/bootstrap/src/lib.rs index 965d788bb8371..8a452feeece01 100644 --- a/src/bootstrap/src/lib.rs +++ b/src/bootstrap/src/lib.rs @@ -51,7 +51,9 @@ mod utils; pub use core::builder::PathSet; pub use core::config::flags::Subcommand; pub use core::config::Config; -pub use utils::change_tracker::{find_recent_config_change_ids, CONFIG_CHANGE_HISTORY}; +pub use utils::change_tracker::{ + find_recent_config_change_ids, human_readable_changes, CONFIG_CHANGE_HISTORY, +}; const LLVM_TOOLS: &[&str] = &[ "llvm-cov", // used to generate coverage report diff --git a/src/bootstrap/src/utils/change_tracker.rs b/src/bootstrap/src/utils/change_tracker.rs index 9a50ad4437e73..50cb7d7ef5a51 100644 --- a/src/bootstrap/src/utils/change_tracker.rs +++ b/src/bootstrap/src/utils/change_tracker.rs @@ -60,6 +60,20 @@ pub fn find_recent_config_change_ids(current_id: usize) -> Vec { .collect() } +pub fn human_readable_changes(changes: &[ChangeInfo]) -> String { + let mut message = String::new(); + + for change in changes { + message.push_str(&format!(" [{}] {}\n", change.severity, change.summary)); + message.push_str(&format!( + " - PR Link https://github.com/rust-lang/rust/pull/{}\n", + change.change_id + )); + } + + message +} + /// Keeps track of major changes made to the bootstrap configuration. /// /// If you make any major changes (such as adding new values or changing default values), From c36f4934e03d4a844d0066bb7645760f8636d449 Mon Sep 17 00:00:00 2001 From: onur-ozkan Date: Thu, 29 Feb 2024 18:18:38 +0300 Subject: [PATCH 2/4] add unit tests on unknown fields Signed-off-by: onur-ozkan --- src/bootstrap/src/core/config/config.rs | 2 +- src/bootstrap/src/core/config/tests.rs | 19 ++++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/src/core/config/config.rs b/src/bootstrap/src/core/config/config.rs index 24a085ab40087..f04857ac2b3ec 100644 --- a/src/bootstrap/src/core/config/config.rs +++ b/src/bootstrap/src/core/config/config.rs @@ -618,7 +618,7 @@ pub(crate) struct TomlConfig { #[derive(Deserialize, Default)] pub(crate) struct ChangeIdWrapper { #[serde(alias = "change-id")] - inner: Option, + pub(crate) inner: Option, } /// Describes how to handle conflicts in merging two [`TomlConfig`] diff --git a/src/bootstrap/src/core/config/tests.rs b/src/bootstrap/src/core/config/tests.rs index 6ac573c68df18..93ba5f4120ac1 100644 --- a/src/bootstrap/src/core/config/tests.rs +++ b/src/bootstrap/src/core/config/tests.rs @@ -1,4 +1,4 @@ -use super::{flags::Flags, Config}; +use super::{flags::Flags, ChangeIdWrapper, Config}; use crate::core::config::{LldMode, TomlConfig}; use clap::CommandFactory; @@ -237,3 +237,20 @@ fn rust_lld() { assert!(matches!(parse("rust.use-lld = true").lld_mode, LldMode::External)); assert!(matches!(parse("rust.use-lld = false").lld_mode, LldMode::Unused)); } + +#[test] +#[should_panic] +fn parse_config_with_unknown_field() { + parse("unknown-key = 1"); +} + +#[test] +fn parse_change_id_with_unknown_field() { + let config = r#" + change-id = 3461 + unknown-key = 1 + "#; + + let change_id_wrapper: ChangeIdWrapper = toml::from_str(config).unwrap(); + assert_eq!(change_id_wrapper.inner, Some(3461)); +} From 78e94cba7733d52f0601d853be15622361d94e44 Mon Sep 17 00:00:00 2001 From: long-long-float Date: Wed, 14 Feb 2024 01:29:39 +0900 Subject: [PATCH 3/4] Don't show suggestion if slice pattern is enclosed by any patterns --- compiler/rustc_hir_typeck/src/pat.rs | 40 +++++++++++++------ .../suppress-consider-slicing-issue-120605.rs | 20 ++++++++++ ...press-consider-slicing-issue-120605.stderr | 23 +++++++++++ 3 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 tests/ui/suggestions/suppress-consider-slicing-issue-120605.rs create mode 100644 tests/ui/suggestions/suppress-consider-slicing-issue-120605.stderr diff --git a/compiler/rustc_hir_typeck/src/pat.rs b/compiler/rustc_hir_typeck/src/pat.rs index bb963ad7a3972..553729503688c 100644 --- a/compiler/rustc_hir_typeck/src/pat.rs +++ b/compiler/rustc_hir_typeck/src/pat.rs @@ -83,6 +83,9 @@ struct PatInfo<'tcx, 'a> { binding_mode: BindingMode, top_info: TopInfo<'tcx>, decl_origin: Option>, + + /// The depth of current pattern + current_depth: u32, } impl<'tcx> FnCtxt<'_, 'tcx> { @@ -152,7 +155,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { decl_origin: Option>, ) { let info = TopInfo { expected, origin_expr, span }; - let pat_info = PatInfo { binding_mode: INITIAL_BM, top_info: info, decl_origin }; + let pat_info = + PatInfo { binding_mode: INITIAL_BM, top_info: info, decl_origin, current_depth: 0 }; self.check_pat(pat, expected, pat_info); } @@ -163,7 +167,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { /// Conversely, inside this module, `check_pat_top` should never be used. #[instrument(level = "debug", skip(self, pat_info))] fn check_pat(&self, pat: &'tcx Pat<'tcx>, expected: Ty<'tcx>, pat_info: PatInfo<'tcx, '_>) { - let PatInfo { binding_mode: def_bm, top_info: ti, .. } = pat_info; + let PatInfo { binding_mode: def_bm, top_info: ti, current_depth, .. } = pat_info; + let path_res = match &pat.kind { PatKind::Path(qpath) => Some( self.resolve_ty_and_res_fully_qualified_call(qpath, pat.hir_id, pat.span, None), @@ -172,8 +177,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }; let adjust_mode = self.calc_adjust_mode(pat, path_res.map(|(res, ..)| res)); let (expected, def_bm) = self.calc_default_binding_mode(pat, expected, def_bm, adjust_mode); - let pat_info = - PatInfo { binding_mode: def_bm, top_info: ti, decl_origin: pat_info.decl_origin }; + let pat_info = PatInfo { + binding_mode: def_bm, + top_info: ti, + decl_origin: pat_info.decl_origin, + current_depth: current_depth + 1, + }; let ty = match pat.kind { PatKind::Wild | PatKind::Err(_) => expected, @@ -1046,14 +1055,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expected: Ty<'tcx>, pat_info: PatInfo<'tcx, '_>, ) -> Ty<'tcx> { - let PatInfo { binding_mode: def_bm, top_info: ti, decl_origin } = pat_info; + let PatInfo { binding_mode: def_bm, top_info: ti, decl_origin, current_depth } = pat_info; let tcx = self.tcx; let on_error = |e| { for pat in subpats { self.check_pat( pat, Ty::new_error(tcx, e), - PatInfo { binding_mode: def_bm, top_info: ti, decl_origin }, + PatInfo { binding_mode: def_bm, top_info: ti, decl_origin, current_depth }, ); } }; @@ -1120,7 +1129,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.check_pat( subpat, field_ty, - PatInfo { binding_mode: def_bm, top_info: ti, decl_origin }, + PatInfo { binding_mode: def_bm, top_info: ti, decl_origin, current_depth }, ); self.tcx.check_stability( @@ -2134,7 +2143,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // The expected type must be an array or slice, but was neither, so error. _ => { let guar = expected.error_reported().err().unwrap_or_else(|| { - self.error_expected_array_or_slice(span, expected, pat_info.top_info) + self.error_expected_array_or_slice(span, expected, pat_info) }); let err = Ty::new_error(self.tcx, guar); (err, Some(err), err) @@ -2273,8 +2282,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &self, span: Span, expected_ty: Ty<'tcx>, - ti: TopInfo<'tcx>, + pat_info: PatInfo<'tcx, '_>, ) -> ErrorGuaranteed { + let PatInfo { top_info: ti, current_depth, .. } = pat_info; + let mut err = struct_span_code_err!( self.dcx(), span, @@ -2292,9 +2303,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { && let Some(_) = ti.origin_expr && let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { - let ty = self.resolve_vars_if_possible(ti.expected); - let is_slice_or_array_or_vector = self.is_slice_or_array_or_vector(ty); - match is_slice_or_array_or_vector.1.kind() { + let resolved_ty = self.resolve_vars_if_possible(ti.expected); + let (is_slice_or_array_or_vector, resolved_ty) = + self.is_slice_or_array_or_vector(resolved_ty); + match resolved_ty.kind() { ty::Adt(adt_def, _) if self.tcx.is_diagnostic_item(sym::Option, adt_def.did()) || self.tcx.is_diagnostic_item(sym::Result, adt_def.did()) => @@ -2309,7 +2321,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } _ => (), } - if is_slice_or_array_or_vector.0 { + + let is_top_level = current_depth <= 1; + if is_slice_or_array_or_vector && is_top_level { err.span_suggestion( span, "consider slicing here", diff --git a/tests/ui/suggestions/suppress-consider-slicing-issue-120605.rs b/tests/ui/suggestions/suppress-consider-slicing-issue-120605.rs new file mode 100644 index 0000000000000..135535cd00ad1 --- /dev/null +++ b/tests/ui/suggestions/suppress-consider-slicing-issue-120605.rs @@ -0,0 +1,20 @@ +pub struct Struct { + a: Vec, +} + +impl Struct { + pub fn test(&self) { + if let [Struct { a: [] }] = &self.a { + //~^ ERROR expected an array or slice + //~| ERROR expected an array or slice + println!("matches!") + } + + if let [Struct { a: [] }] = &self.a[..] { + //~^ ERROR expected an array or slice + println!("matches!") + } + } +} + +fn main() {} diff --git a/tests/ui/suggestions/suppress-consider-slicing-issue-120605.stderr b/tests/ui/suggestions/suppress-consider-slicing-issue-120605.stderr new file mode 100644 index 0000000000000..c28d67604da9f --- /dev/null +++ b/tests/ui/suggestions/suppress-consider-slicing-issue-120605.stderr @@ -0,0 +1,23 @@ +error[E0529]: expected an array or slice, found `Vec` + --> $DIR/suppress-consider-slicing-issue-120605.rs:7:16 + | +LL | if let [Struct { a: [] }] = &self.a { + | ^^^^^^^^^^^^^^^^^^ ------- help: consider slicing here: `&self.a[..]` + | | + | pattern cannot match with input type `Vec` + +error[E0529]: expected an array or slice, found `Vec` + --> $DIR/suppress-consider-slicing-issue-120605.rs:7:29 + | +LL | if let [Struct { a: [] }] = &self.a { + | ^^ pattern cannot match with input type `Vec` + +error[E0529]: expected an array or slice, found `Vec` + --> $DIR/suppress-consider-slicing-issue-120605.rs:13:29 + | +LL | if let [Struct { a: [] }] = &self.a[..] { + | ^^ pattern cannot match with input type `Vec` + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0529`. From b8db431f37e097a545f364335fdcd5f65c056cb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Sun, 17 Mar 2024 12:19:46 +0100 Subject: [PATCH 4/4] avoid unnecessary collect() --- compiler/rustc_hir_analysis/src/astconv/errors.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/astconv/errors.rs b/compiler/rustc_hir_analysis/src/astconv/errors.rs index 37fbf45235a86..68896768e8d8a 100644 --- a/compiler/rustc_hir_analysis/src/astconv/errors.rs +++ b/compiler/rustc_hir_analysis/src/astconv/errors.rs @@ -408,10 +408,7 @@ impl<'tcx> dyn AstConv<'tcx> + '_ { traits with associated type `{name}`, you could use the \ fully-qualified path", ), - traits - .iter() - .map(|trait_str| format!("::{name}")) - .collect::>(), + traits.iter().map(|trait_str| format!("::{name}")), Applicability::HasPlaceholders, ); }