From 01307cf03f5b95759a8e84bce659f6326e247049 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 24 Dec 2024 23:41:50 +0000 Subject: [PATCH 1/7] Implement `default_overrides_default_fields` lint Detect when a manual `Default` implementation isn't using the existing default field values and suggest using `..` instead: ``` error: `Default` impl doesn't use the declared default field values --> $DIR/manual-default-impl-could-be-derived.rs:14:1 | LL | / impl Default for A { LL | | fn default() -> Self { LL | | A { LL | | y: 0, | | - this field has a default value ... | LL | | } | |_^ | = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time note: the lint level is defined here --> $DIR/manual-default-impl-could-be-derived.rs:5:9 | LL | #![deny(default_overrides_default_fields)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` --- .../src/default_could_be_derived.rs | 185 +++++++++++++++++ compiler/rustc_lint/src/lib.rs | 3 + .../manual-default-impl-could-be-derived.rs | 194 ++++++++++++++++++ ...anual-default-impl-could-be-derived.stderr | 144 +++++++++++++ 4 files changed, 526 insertions(+) create mode 100644 compiler/rustc_lint/src/default_could_be_derived.rs create mode 100644 tests/ui/structs/manual-default-impl-could-be-derived.rs create mode 100644 tests/ui/structs/manual-default-impl-could-be-derived.stderr diff --git a/compiler/rustc_lint/src/default_could_be_derived.rs b/compiler/rustc_lint/src/default_could_be_derived.rs new file mode 100644 index 0000000000000..d95cbb051580b --- /dev/null +++ b/compiler/rustc_lint/src/default_could_be_derived.rs @@ -0,0 +1,185 @@ +use rustc_data_structures::fx::FxHashMap; +use rustc_errors::Diag; +use rustc_hir as hir; +use rustc_middle::ty; +use rustc_session::{declare_lint, impl_lint_pass}; +use rustc_span::Symbol; +use rustc_span::symbol::sym; + +use crate::{LateContext, LateLintPass}; + +declare_lint! { + /// The `default_overrides_default_fields` lint checks for manual `impl` blocks of the + /// `Default` trait of types with default field values. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// #![feature(default_field_values)] + /// struct Foo { + /// x: i32 = 101, + /// y: NonDefault, + /// } + /// + /// struct NonDefault; + /// + /// #[deny(default_overrides_default_fields)] + /// impl Default for Foo { + /// fn default() -> Foo { + /// Foo { x: 100, y: NonDefault } + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// Manually writing a `Default` implementation for a type that has + /// default field values runs the risk of diverging behavior between + /// `Type { .. }` and `::default()`, which would be a + /// foot-gun for users of that type that would expect these to be + /// equivalent. If `Default` can't be derived due to some fields not + /// having a `Default` implementation, we encourage the use of `..` for + /// the fields that do have a default field value. + pub DEFAULT_OVERRIDES_DEFAULT_FIELDS, + Deny, + "detect `Default` impl that should use the type's default field values", + @feature_gate = default_field_values; +} + +#[derive(Default)] +pub(crate) struct DefaultCouldBeDerived; + +impl_lint_pass!(DefaultCouldBeDerived => [DEFAULT_OVERRIDES_DEFAULT_FIELDS]); + +impl<'tcx> LateLintPass<'tcx> for DefaultCouldBeDerived { + fn check_impl_item(&mut self, cx: &LateContext<'_>, impl_item: &hir::ImplItem<'_>) { + // Look for manual implementations of `Default`. + let Some(default_def_id) = cx.tcx.get_diagnostic_item(sym::Default) else { return }; + let hir::ImplItemKind::Fn(_sig, body_id) = impl_item.kind else { return }; + let assoc = cx.tcx.associated_item(impl_item.owner_id); + let parent = assoc.container_id(cx.tcx); + if cx.tcx.has_attr(parent, sym::automatically_derived) { + // We don't care about what `#[derive(Default)]` produces in this lint. + return; + } + let Some(trait_ref) = cx.tcx.impl_trait_ref(parent) else { return }; + let trait_ref = trait_ref.instantiate_identity(); + if trait_ref.def_id != default_def_id { + return; + } + let ty = trait_ref.self_ty(); + let ty::Adt(def, _) = ty.kind() else { return }; + + // We now know we have a manually written definition of a `::default()`. + + let hir = cx.tcx.hir(); + + let type_def_id = def.did(); + let body = hir.body(body_id); + + // FIXME: evaluate bodies with statements and evaluate bindings to see if they would be + // derivable. + let hir::ExprKind::Block(hir::Block { stmts: _, expr: Some(expr), .. }, None) = + body.value.kind + else { + return; + }; + + // Keep a mapping of field name to `hir::FieldDef` for every field in the type. We'll use + // these to check for things like checking whether it has a default or using its span for + // suggestions. + let orig_fields = match hir.get_if_local(type_def_id) { + Some(hir::Node::Item(hir::Item { + kind: + hir::ItemKind::Struct(hir::VariantData::Struct { fields, recovered: _ }, _generics), + .. + })) => fields.iter().map(|f| (f.ident.name, f)).collect::>(), + _ => return, + }; + + // We check `fn default()` body is a single ADT literal and get all the fields that are + // being set. + let hir::ExprKind::Struct(_qpath, fields, tail) = expr.kind else { return }; + + // We have a struct literal + // + // struct Foo { + // field: Type, + // } + // + // impl Default for Foo { + // fn default() -> Foo { + // Foo { + // field: val, + // } + // } + // } + // + // We would suggest `#[derive(Default)]` if `field` has a default value, regardless of what + // it is; we don't want to encourage divergent behavior between `Default::default()` and + // `..`. + + if let hir::StructTailExpr::Base(_) = tail { + // This is *very* niche. We'd only get here if someone wrote + // impl Default for Ty { + // fn default() -> Ty { + // Ty { ..something() } + // } + // } + // where `something()` would have to be a call or path. + // We have nothing meaninful to do with this. + return; + } + + // At least one of the fields with a default value have been overriden in + // the `Default` implementation. We suggest removing it and relying on `..` + // instead. + let any_default_field_given = + fields.iter().any(|f| orig_fields.get(&f.ident.name).and_then(|f| f.default).is_some()); + + if !any_default_field_given { + // None of the default fields were actually provided explicitly, so the manual impl + // doesn't override them (the user used `..`), so there's no risk of divergent behavior. + return; + } + + let Some(local) = parent.as_local() else { return }; + let hir_id = cx.tcx.local_def_id_to_hir_id(local); + let hir::Node::Item(item) = cx.tcx.hir_node(hir_id) else { return }; + cx.tcx.node_span_lint(DEFAULT_OVERRIDES_DEFAULT_FIELDS, hir_id, item.span, |diag| { + mk_lint(diag, orig_fields, fields); + }); + } +} + +fn mk_lint( + diag: &mut Diag<'_, ()>, + orig_fields: FxHashMap>, + fields: &[hir::ExprField<'_>], +) { + diag.primary_message("`Default` impl doesn't use the declared default field values"); + + // For each field in the struct expression + // - if the field in the type has a default value, it should be removed + // - elif the field is an expression that could be a default value, it should be used as the + // field's default value (FIXME: not done). + // - else, we wouldn't touch this field, it would remain in the manual impl + let mut removed_all_fields = true; + for field in fields { + if orig_fields.get(&field.ident.name).and_then(|f| f.default).is_some() { + diag.span_label(field.expr.span, "this field has a default value"); + } else { + removed_all_fields = false; + } + } + + diag.help(if removed_all_fields { + "to avoid divergence in behavior between `Struct { .. }` and \ + `::default()`, derive the `Default`" + } else { + "use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them \ + diverging over time" + }); +} diff --git a/compiler/rustc_lint/src/lib.rs b/compiler/rustc_lint/src/lib.rs index d7f0d2a6941fb..1465c2cff7bc1 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -41,6 +41,7 @@ mod async_fn_in_trait; pub mod builtin; mod context; mod dangling; +mod default_could_be_derived; mod deref_into_dyn_supertrait; mod drop_forget_useless; mod early; @@ -85,6 +86,7 @@ use async_closures::AsyncClosureUsage; use async_fn_in_trait::AsyncFnInTrait; use builtin::*; use dangling::*; +use default_could_be_derived::DefaultCouldBeDerived; use deref_into_dyn_supertrait::*; use drop_forget_useless::*; use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums; @@ -189,6 +191,7 @@ late_lint_methods!( BuiltinCombinedModuleLateLintPass, [ ForLoopsOverFallibles: ForLoopsOverFallibles, + DefaultCouldBeDerived: DefaultCouldBeDerived::default(), DerefIntoDynSupertrait: DerefIntoDynSupertrait, DropForgetUseless: DropForgetUseless, ImproperCTypesDeclarations: ImproperCTypesDeclarations, diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.rs b/tests/ui/structs/manual-default-impl-could-be-derived.rs new file mode 100644 index 0000000000000..773b738998862 --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.rs @@ -0,0 +1,194 @@ +// Warn when we encounter a manual `Default` impl that could be derived. +// Restricted only to types using `default_field_values`. +#![feature(default_field_values)] +#![allow(dead_code)] +#![deny(default_overrides_default_fields)] +struct S(i32); +fn s() -> S { S(1) } + +struct A { + x: S, + y: i32 = 1, +} + +impl Default for A { //~ ERROR default_overrides_default_fields + fn default() -> Self { + A { + y: 0, + x: s(), + } + } +} + +struct B { + x: S = S(3), + y: i32 = 1, +} + +impl Default for B { //~ ERROR default_overrides_default_fields + fn default() -> Self { + B { + x: s(), + y: 0, + } + } +} + +struct C { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for C { //~ ERROR default_overrides_default_fields + fn default() -> Self { + C { + x: s(), + y: 0, + .. + } + } +} + +struct D { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for D { //~ ERROR default_overrides_default_fields + fn default() -> Self { + D { + y: 0, + x: s(), + .. + } + } +} + +struct E { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E { + y: 0, + z: 0, + x: s(), + } + } +} + +// Let's ensure that the span for `x` and the span for `y` don't overlap when suggesting their +// removal in favor of their default field values. +struct E2 { + x: S, + y: i32 = 1, + z: i32 = 1, +} + +impl Default for E2 { //~ ERROR default_overrides_default_fields + fn default() -> Self { + E2 { + x: s(), + y: i(), + z: 0, + } + } +} + +fn i() -> i32 { + 1 +} + +// Account for a `const fn` being the `Default::default()` of a field's type. +struct F { + x: G, + y: i32 = 1, +} + +impl Default for F { //~ ERROR default_overrides_default_fields + fn default() -> Self { + F { + x: g_const(), + y: 0, + } + } +} + +struct G; + +impl Default for G { // ok + fn default() -> Self { + g_const() + } +} + +const fn g_const() -> G { + G +} + +// Account for a `const fn` being used in `Default::default()`, even if the type doesn't use it as +// its own `Default`. We suggest setting the default field value in that case. +struct H { + x: I, + y: i32 = 1, +} + +impl Default for H { //~ ERROR default_overrides_default_fields + fn default() -> Self { + H { + x: i_const(), + y: 0, + } + } +} + +struct I; + +const fn i_const() -> I { + I +} + +// Account for a `const` and struct literal being the `Default::default()` of a field's type. +struct M { + x: N, + y: i32 = 1, + z: A, +} + +impl Default for M { // ok, `y` is not specified + fn default() -> Self { + M { + x: N_CONST, + z: A { + x: S(0), + y: 0, + }, + .. + } + } +} + +struct N; + +const N_CONST: N = N; + +struct O { + x: Option, + y: i32 = 1, +} + +impl Default for O { //~ ERROR default_overrides_default_fields + fn default() -> Self { + O { + x: None, + y: 1, + } + } +} + +fn main() {} diff --git a/tests/ui/structs/manual-default-impl-could-be-derived.stderr b/tests/ui/structs/manual-default-impl-could-be-derived.stderr new file mode 100644 index 0000000000000..e8f607fac7edd --- /dev/null +++ b/tests/ui/structs/manual-default-impl-could-be-derived.stderr @@ -0,0 +1,144 @@ +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:14:1 + | +LL | / impl Default for A { +LL | | fn default() -> Self { +LL | | A { +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time +note: the lint level is defined here + --> $DIR/manual-default-impl-could-be-derived.rs:5:9 + | +LL | #![deny(default_overrides_default_fields)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:28:1 + | +LL | / impl Default for B { +LL | | fn default() -> Self { +LL | | B { +LL | | x: s(), + | | --- this field has a default value +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: to avoid divergence in behavior between `Struct { .. }` and `::default()`, derive the `Default` + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:43:1 + | +LL | / impl Default for C { +LL | | fn default() -> Self { +LL | | C { +LL | | x: s(), +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:59:1 + | +LL | / impl Default for D { +LL | | fn default() -> Self { +LL | | D { +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:75:1 + | +LL | / impl Default for E { +LL | | fn default() -> Self { +LL | | E { +LL | | y: 0, + | | - this field has a default value +LL | | z: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:93:1 + | +LL | / impl Default for E2 { +LL | | fn default() -> Self { +LL | | E2 { +LL | | x: s(), +LL | | y: i(), + | | --- this field has a default value +LL | | z: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:113:1 + | +LL | / impl Default for F { +LL | | fn default() -> Self { +LL | | F { +LL | | x: g_const(), +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:141:1 + | +LL | / impl Default for H { +LL | | fn default() -> Self { +LL | | H { +LL | | x: i_const(), +LL | | y: 0, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: `Default` impl doesn't use the declared default field values + --> $DIR/manual-default-impl-could-be-derived.rs:185:1 + | +LL | / impl Default for O { +LL | | fn default() -> Self { +LL | | O { +LL | | x: None, +LL | | y: 1, + | | - this field has a default value +... | +LL | | } + | |_^ + | + = help: use the default values in the `impl` with `Struct { mandatory_field, .. }` to avoid them diverging over time + +error: aborting due to 9 previous errors + From 68bd853bb6b391135565e42577baa2be09dee762 Mon Sep 17 00:00:00 2001 From: Trevor Gross Date: Fri, 27 Dec 2024 21:57:09 +0000 Subject: [PATCH 2/7] Update `compiler-builtins` to 0.1.140 Nothing significant here, just syncing the following small changes: - https://github.com/rust-lang/compiler-builtins/pull/727 - https://github.com/rust-lang/compiler-builtins/pull/730 - https://github.com/rust-lang/compiler-builtins/pull/736 - https://github.com/rust-lang/compiler-builtins/pull/737 --- ...029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch | 4 ++-- library/Cargo.lock | 4 ++-- library/alloc/Cargo.toml | 2 +- library/std/Cargo.toml | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_codegen_cranelift/patches/0029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch b/compiler/rustc_codegen_cranelift/patches/0029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch index 6012af6a8dd2a..bf07e455a75f5 100644 --- a/compiler/rustc_codegen_cranelift/patches/0029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch +++ b/compiler/rustc_codegen_cranelift/patches/0029-stdlib-Disable-f16-and-f128-in-compiler-builtins.patch @@ -16,8 +16,8 @@ index 7165c3e48af..968552ad435 100644 [dependencies] core = { path = "../core" } --compiler_builtins = { version = "=0.1.138", features = ['rustc-dep-of-std'] } -+compiler_builtins = { version = "=0.1.138", features = ['rustc-dep-of-std', 'no-f16-f128'] } +-compiler_builtins = { version = "=0.1.140", features = ['rustc-dep-of-std'] } ++compiler_builtins = { version = "=0.1.140", features = ['rustc-dep-of-std', 'no-f16-f128'] } [dev-dependencies] rand = { version = "0.8.5", default-features = false, features = ["alloc"] } diff --git a/library/Cargo.lock b/library/Cargo.lock index 15ca4cbff7b79..40edd2c211cd3 100644 --- a/library/Cargo.lock +++ b/library/Cargo.lock @@ -61,9 +61,9 @@ dependencies = [ [[package]] name = "compiler_builtins" -version = "0.1.138" +version = "0.1.140" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53f0ea7fff95b51f84371588f06062557e96bbe363d2b36218ddb806f3ca8611" +checksum = "df14d41c5d172a886df3753d54238eefb0f61c96cbd8b363c33ccc92c457bee3" dependencies = [ "cc", "rustc-std-workspace-core", diff --git a/library/alloc/Cargo.toml b/library/alloc/Cargo.toml index 3464047d4ee9e..07596fa16f982 100644 --- a/library/alloc/Cargo.toml +++ b/library/alloc/Cargo.toml @@ -10,7 +10,7 @@ edition = "2021" [dependencies] core = { path = "../core" } -compiler_builtins = { version = "=0.1.138", features = ['rustc-dep-of-std'] } +compiler_builtins = { version = "=0.1.140", features = ['rustc-dep-of-std'] } [dev-dependencies] rand = { version = "0.8.5", default-features = false, features = ["alloc"] } diff --git a/library/std/Cargo.toml b/library/std/Cargo.toml index f43dcc1630c6a..6380c941e6ab5 100644 --- a/library/std/Cargo.toml +++ b/library/std/Cargo.toml @@ -17,7 +17,7 @@ cfg-if = { version = "1.0", features = ['rustc-dep-of-std'] } panic_unwind = { path = "../panic_unwind", optional = true } panic_abort = { path = "../panic_abort" } core = { path = "../core", public = true } -compiler_builtins = { version = "=0.1.138" } +compiler_builtins = { version = "=0.1.140" } unwind = { path = "../unwind" } hashbrown = { version = "0.15", default-features = false, features = [ 'rustc-dep-of-std', From ef19017f7ca29450efff332db297851933f21844 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 28 Dec 2024 13:12:36 +1100 Subject: [PATCH 3/7] compiletest: Self-test for `normalize-*` with revisions --- .../normalize-with-revision.a.run.stderr | 2 ++ .../normalize-with-revision.b.run.stderr | 2 ++ .../normalize-with-revision.rs | 20 +++++++++++++++++++ 3 files changed, 24 insertions(+) create mode 100644 tests/ui/compiletest-self-test/normalize-with-revision.a.run.stderr create mode 100644 tests/ui/compiletest-self-test/normalize-with-revision.b.run.stderr create mode 100644 tests/ui/compiletest-self-test/normalize-with-revision.rs diff --git a/tests/ui/compiletest-self-test/normalize-with-revision.a.run.stderr b/tests/ui/compiletest-self-test/normalize-with-revision.a.run.stderr new file mode 100644 index 0000000000000..3eb3f6b4e5712 --- /dev/null +++ b/tests/ui/compiletest-self-test/normalize-with-revision.a.run.stderr @@ -0,0 +1,2 @@ +1st emitted line +second emitted line diff --git a/tests/ui/compiletest-self-test/normalize-with-revision.b.run.stderr b/tests/ui/compiletest-self-test/normalize-with-revision.b.run.stderr new file mode 100644 index 0000000000000..8d9156480abff --- /dev/null +++ b/tests/ui/compiletest-self-test/normalize-with-revision.b.run.stderr @@ -0,0 +1,2 @@ +first emitted line +2nd emitted line diff --git a/tests/ui/compiletest-self-test/normalize-with-revision.rs b/tests/ui/compiletest-self-test/normalize-with-revision.rs new file mode 100644 index 0000000000000..e1bbbb3eabb70 --- /dev/null +++ b/tests/ui/compiletest-self-test/normalize-with-revision.rs @@ -0,0 +1,20 @@ +//! Checks that `[rev] normalize-*` directives affect the specified revision, +//! and don't affect other revisions. +//! +//! This currently relies on the fact that `normalize-*` directives are +//! applied to run output, not just compiler output. If that ever changes, +//! this test might need to be adjusted. + +//@ edition: 2021 +//@ revisions: a b +//@ run-pass +//@ check-run-results + +//@ normalize-stderr: "output" -> "emitted" +//@[a] normalize-stderr: "first" -> "1st" +//@[b] normalize-stderr: "second" -> "2nd" + +fn main() { + eprintln!("first output line"); + eprintln!("second output line"); +} From 3a4e82195e08a77d542078cadcc881105e9d0838 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 28 Dec 2024 13:21:21 +1100 Subject: [PATCH 4/7] compiletest: Only pass the post-colon value to `parse_normalize_rule` --- src/tools/compiletest/src/header.rs | 18 ++++++++-------- src/tools/compiletest/src/header/tests.rs | 25 +++++++++++++---------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 67ecdaf9e5e13..73d00fbf7b85f 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -978,10 +978,10 @@ impl Config { } } - fn parse_custom_normalization(&self, line: &str) -> Option { + fn parse_custom_normalization(&self, raw_directive: &str) -> Option { // FIXME(Zalathar): Integrate name/value splitting into `DirectiveLine` // instead of doing it here. - let (directive_name, _value) = line.split_once(':')?; + let (directive_name, raw_value) = raw_directive.split_once(':')?; let kind = match directive_name { "normalize-stdout" => NormalizeKind::Stdout, @@ -991,11 +991,9 @@ impl Config { _ => return None, }; - // FIXME(Zalathar): The normalize rule parser should only care about - // the value part, not the "line" (which isn't even the whole line). - let Some((regex, replacement)) = parse_normalize_rule(line) else { + let Some((regex, replacement)) = parse_normalize_rule(raw_value) else { panic!( - "couldn't parse custom normalization rule: `{line}`\n\ + "couldn't parse custom normalization rule: `{raw_directive}`\n\ help: expected syntax is: `{directive_name}: \"REGEX\" -> \"REPLACEMENT\"`" ); }; @@ -1141,21 +1139,21 @@ enum NormalizeKind { /// Parses the regex and replacement values of a `//@ normalize-*` header, /// in the format: /// ```text -/// normalize-*: "REGEX" -> "REPLACEMENT" +/// "REGEX" -> "REPLACEMENT" /// ``` -fn parse_normalize_rule(header: &str) -> Option<(String, String)> { +fn parse_normalize_rule(raw_value: &str) -> Option<(String, String)> { // FIXME: Support escaped double-quotes in strings. let captures = static_regex!( r#"(?x) # (verbose mode regex) ^ - [^:\s]+:\s* # (header name followed by colon) + \s* # (leading whitespace) "(?[^"]*)" # "REGEX" \s+->\s+ # -> "(?[^"]*)" # "REPLACEMENT" $ "# ) - .captures(header)?; + .captures(raw_value)?; let regex = captures["regex"].to_owned(); let replacement = captures["replacement"].to_owned(); // FIXME: Support escaped new-line in strings. diff --git a/src/tools/compiletest/src/header/tests.rs b/src/tools/compiletest/src/header/tests.rs index cd7c6f8361ed3..618b66dfd4cb6 100644 --- a/src/tools/compiletest/src/header/tests.rs +++ b/src/tools/compiletest/src/header/tests.rs @@ -35,11 +35,14 @@ fn make_test_description( #[test] fn test_parse_normalize_rule() { - let good_data = &[( - r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)""#, - "something (32 bits)", - "something ($WORD bits)", - )]; + let good_data = &[ + ( + r#""something (32 bits)" -> "something ($WORD bits)""#, + "something (32 bits)", + "something ($WORD bits)", + ), + (r#" " with whitespace" -> " replacement""#, " with whitespace", " replacement"), + ]; for &(input, expected_regex, expected_replacement) in good_data { let parsed = parse_normalize_rule(input); @@ -49,15 +52,15 @@ fn test_parse_normalize_rule() { } let bad_data = &[ - r#"normalize-stderr-32bit "something (32 bits)" -> "something ($WORD bits)""#, - r#"normalize-stderr-16bit: something (16 bits) -> something ($WORD bits)"#, - r#"normalize-stderr-32bit: something (32 bits) -> something ($WORD bits)"#, - r#"normalize-stderr-32bit: "something (32 bits) -> something ($WORD bits)"#, - r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"#, - r#"normalize-stderr-32bit: "something (32 bits)" -> "something ($WORD bits)"."#, + r#"something (11 bits) -> something ($WORD bits)"#, + r#"something (12 bits) -> something ($WORD bits)"#, + r#""something (13 bits) -> something ($WORD bits)"#, + r#""something (14 bits)" -> "something ($WORD bits)"#, + r#""something (15 bits)" -> "something ($WORD bits)"."#, ]; for &input in bad_data { + println!("- {input:?}"); let parsed = parse_normalize_rule(input); assert_eq!(parsed, None); } From f55736365a886d6fbb90cf97e3941c76d77536f2 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 28 Dec 2024 14:23:12 +1100 Subject: [PATCH 5/7] compiletest: Make a FIXME for escaped newlines less confusing The old FIXME implies that we don't support escaped newlines, but in fact it was added in the same patch that added support for escaped newlines. The new FIXME makes it clear that we do currently support this, and that the FIXME is for doing so in a less ad-hoc way. --- src/tools/compiletest/src/header.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/tools/compiletest/src/header.rs b/src/tools/compiletest/src/header.rs index 73d00fbf7b85f..48149e3b897e1 100644 --- a/src/tools/compiletest/src/header.rs +++ b/src/tools/compiletest/src/header.rs @@ -1156,7 +1156,9 @@ fn parse_normalize_rule(raw_value: &str) -> Option<(String, String)> { .captures(raw_value)?; let regex = captures["regex"].to_owned(); let replacement = captures["replacement"].to_owned(); - // FIXME: Support escaped new-line in strings. + // A `\n` sequence in the replacement becomes an actual newline. + // FIXME: Do unescaping in a less ad-hoc way, and perhaps support escaped + // backslashes and double-quotes. let replacement = replacement.replace("\\n", "\n"); Some((regex, replacement)) } From b77ab2dd90b8636a38cb34a3e6fe73dbb1e7b0f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Sat, 28 Dec 2024 02:20:26 +0800 Subject: [PATCH 6/7] tests: migrate `libs-through-symlink` to rmake.rs - Document test intent, backlink to #13890 and fix PR #13903. - Fix the test logic: the `Makefile` version seems to not actually be exercising the "library search traverses symlink" logic, because the actual symlinked-to-library is present under the directory tree when `bar.rs` is compiled, because the `$(RUSTC)` invocation has an implicit `-L $(TMPDIR)`. The symlink itself was actually broken, i.e. it should've been `ln -nsf $(TMPDIR)/outdir/$(NAME) $(TMPDIR)` but it used `ln -nsf outdir/$(NAME) $(TMPDIR)`. Co-authored-by: Oneirical --- .../tidy/src/allowed_run_make_makefiles.txt | 1 - tests/run-make/libs-through-symlinks/Makefile | 22 --------- tests/run-make/libs-through-symlinks/rmake.rs | 48 +++++++++++++++++++ 3 files changed, 48 insertions(+), 23 deletions(-) delete mode 100644 tests/run-make/libs-through-symlinks/Makefile create mode 100644 tests/run-make/libs-through-symlinks/rmake.rs diff --git a/src/tools/tidy/src/allowed_run_make_makefiles.txt b/src/tools/tidy/src/allowed_run_make_makefiles.txt index f7ecb4851529c..7ab27667e28fc 100644 --- a/src/tools/tidy/src/allowed_run_make_makefiles.txt +++ b/src/tools/tidy/src/allowed_run_make_makefiles.txt @@ -2,7 +2,6 @@ run-make/branch-protection-check-IBT/Makefile run-make/cat-and-grep-sanity-check/Makefile run-make/extern-fn-reachable/Makefile run-make/jobserver-error/Makefile -run-make/libs-through-symlinks/Makefile run-make/split-debuginfo/Makefile run-make/symbol-mangling-hashed/Makefile run-make/translation/Makefile diff --git a/tests/run-make/libs-through-symlinks/Makefile b/tests/run-make/libs-through-symlinks/Makefile deleted file mode 100644 index c6ff566a0e86e..0000000000000 --- a/tests/run-make/libs-through-symlinks/Makefile +++ /dev/null @@ -1,22 +0,0 @@ -# ignore-cross-compile -include ../tools.mk - -# ignore-windows - -# The option -n for the AIX ln command has a different purpose than it does -# on Linux. On Linux, the -n option is used to treat the destination path as -# normal file if it is a symbolic link to a directory, which is the default -# behavior of the AIX ln command. -ifeq ($(UNAME),AIX) -LN_FLAGS := -sf -else -LN_FLAGS := -nsf -endif - -NAME := $(shell $(RUSTC) --print file-names foo.rs) - -all: - mkdir -p $(TMPDIR)/outdir - $(RUSTC) foo.rs -o $(TMPDIR)/outdir/$(NAME) - ln $(LN_FLAGS) outdir/$(NAME) $(TMPDIR) - RUSTC_LOG=rustc_metadata::loader $(RUSTC) bar.rs diff --git a/tests/run-make/libs-through-symlinks/rmake.rs b/tests/run-make/libs-through-symlinks/rmake.rs new file mode 100644 index 0000000000000..4bb3d05abb7a3 --- /dev/null +++ b/tests/run-make/libs-through-symlinks/rmake.rs @@ -0,0 +1,48 @@ +//! Regression test for [rustc doesn't handle relative symlinks to libraries +//! #13890](https://github.com/rust-lang/rust/issues/13890). +//! +//! This smoke test checks that for a given library search path `P`: +//! +//! - `rustc` is able to locate a library available via a symlink, where: +//! - the symlink is under the directory subtree of `P`, +//! - but the actual library is not (it's in a different directory subtree). +//! +//! For example: +//! +//! ```text +//! actual_dir/ +//! libfoo.rlib +//! symlink_dir/ # $CWD set; rustc -L . bar.rs that depends on foo +//! libfoo.rlib --> ../actual_dir/libfoo.rlib +//! ``` +//! +//! Previously, if `rustc` was invoked with CWD set to `symlink_dir/`, it would fail to traverse the +//! symlink to locate `actual_dir/libfoo.rlib`. This was originally fixed in +//! . + +//@ ignore-cross-compile + +use run_make_support::{bare_rustc, cwd, path, rfs, rust_lib_name}; + +fn main() { + let actual_lib_dir = path("actual_lib_dir"); + let symlink_lib_dir = path("symlink_lib_dir"); + rfs::create_dir_all(&actual_lib_dir); + rfs::create_dir_all(&symlink_lib_dir); + + // NOTE: `bare_rustc` is used because it does not introduce an implicit `-L .` library search + // flag. + bare_rustc().input("foo.rs").output(actual_lib_dir.join(rust_lib_name("foo"))).run(); + + rfs::symlink_file( + actual_lib_dir.join(rust_lib_name("foo")), + symlink_lib_dir.join(rust_lib_name("foo")), + ); + + // Make rustc's $CWD be in the directory containing the symlink-to-lib. + bare_rustc() + .current_dir(&symlink_lib_dir) + .library_search_path(".") + .input(cwd().join("bar.rs")) + .run(); +} From b32591e5808ce7c59b58bd807dc1d26670cedb68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Wed, 25 Dec 2024 20:15:49 +0800 Subject: [PATCH 7/7] tests: migrate `branch-protection-check-IBT` to rmake.rs - The Makefile version *never* ran because of Makefile syntax confusion. - The test would've always failed because precompiled std is not built with `-Z cf-protection=branch`, but linkers require all input object files to indicate IBT support in order to enable IBT for the executable, which is not the case for std. - Thus, the test input file is instead changed to a `no_std` + `no_core` program. Co-authored-by: Jerry Wang Co-authored-by: Oneirical --- .../tidy/src/allowed_run_make_makefiles.txt | 1 - .../branch-protection-check-IBT/Makefile | 21 -------- .../branch-protection-check-IBT/_rmake.rs | 29 ---------- .../branch-protection-check-IBT/main.rs | 8 +-- .../branch-protection-check-IBT/rmake.rs | 53 +++++++++++++++++++ 5 files changed, 58 insertions(+), 54 deletions(-) delete mode 100644 tests/run-make/branch-protection-check-IBT/Makefile delete mode 100644 tests/run-make/branch-protection-check-IBT/_rmake.rs create mode 100644 tests/run-make/branch-protection-check-IBT/rmake.rs diff --git a/src/tools/tidy/src/allowed_run_make_makefiles.txt b/src/tools/tidy/src/allowed_run_make_makefiles.txt index f7ecb4851529c..2f8981870229d 100644 --- a/src/tools/tidy/src/allowed_run_make_makefiles.txt +++ b/src/tools/tidy/src/allowed_run_make_makefiles.txt @@ -1,4 +1,3 @@ -run-make/branch-protection-check-IBT/Makefile run-make/cat-and-grep-sanity-check/Makefile run-make/extern-fn-reachable/Makefile run-make/jobserver-error/Makefile diff --git a/tests/run-make/branch-protection-check-IBT/Makefile b/tests/run-make/branch-protection-check-IBT/Makefile deleted file mode 100644 index ee0e034627f6f..0000000000000 --- a/tests/run-make/branch-protection-check-IBT/Makefile +++ /dev/null @@ -1,21 +0,0 @@ -# Check for GNU Property Note - -include ../tools.mk - -# How to run this -# python3 x.py test --target x86_64-unknown-linux-gnu tests/run-make/branch-protection-check-IBT/ - -# only-x86_64 - -# ignore-test -# FIXME(jieyouxu): This test never runs because the `ifeq` check on line 17 -# compares `x86` to `x86_64`, which always evaluates to false. -# When the test does run, the compilation does not include `.note.gnu.property`. -# See https://github.com/rust-lang/rust/pull/126720 for more information. - -all: -ifeq ($(filter x86,$(LLVM_COMPONENTS)),x86_64) - $(RUSTC) --target x86_64-unknown-linux-gnu -Z cf-protection=branch -L$(TMPDIR) -C link-args='-nostartfiles' -C save-temps ./main.rs -o $(TMPDIR)/rsmain - readelf -nW $(TMPDIR)/rsmain | $(CGREP) -e ".note.gnu.property" -endif - diff --git a/tests/run-make/branch-protection-check-IBT/_rmake.rs b/tests/run-make/branch-protection-check-IBT/_rmake.rs deleted file mode 100644 index 911514087856c..0000000000000 --- a/tests/run-make/branch-protection-check-IBT/_rmake.rs +++ /dev/null @@ -1,29 +0,0 @@ -// Check for GNU Property Note - -// How to run this -// python3 x.py test --target x86_64-unknown-linux-gnu tests/run-make/branch-protection-check-IBT/ - -//@ only-x86_64 - -//@ ignore-test -// FIXME(jieyouxu): see the FIXME in the Makefile - -use run_make_support::{cwd, env_var, llvm_readobj, rustc}; - -fn main() { - let llvm_components = env_var("LLVM_COMPONENTS"); - if !format!(" {llvm_components} ").contains(" x86 ") { - return; - } - - rustc() - .input("main.rs") - .target("x86_64-unknown-linux-gnu") - .arg("-Zcf-protection=branch") - .arg(format!("-L{}", cwd().display())) - .arg("-Clink-args=-nostartfiles") - .arg("-Csave-temps") - .run(); - - llvm_readobj().arg("-nW").input("main").run().assert_stdout_contains(".note.gnu.property"); -} diff --git a/tests/run-make/branch-protection-check-IBT/main.rs b/tests/run-make/branch-protection-check-IBT/main.rs index ad379d6ea4373..445b8795134c5 100644 --- a/tests/run-make/branch-protection-check-IBT/main.rs +++ b/tests/run-make/branch-protection-check-IBT/main.rs @@ -1,3 +1,5 @@ -fn main() { - println!("hello world"); -} +#![feature(no_core)] +#![allow(internal_features)] +#![no_core] +#![no_std] +#![no_main] diff --git a/tests/run-make/branch-protection-check-IBT/rmake.rs b/tests/run-make/branch-protection-check-IBT/rmake.rs new file mode 100644 index 0000000000000..73109df12ae9d --- /dev/null +++ b/tests/run-make/branch-protection-check-IBT/rmake.rs @@ -0,0 +1,53 @@ +// ignore-tidy-linelength +//! A basic smoke test to check for GNU Property Note to see that for `x86_64` targets when [`-Z +//! cf-protection=branch`][intel-cet-tracking-issue] is requested, that the +//! +//! ```text +//! NT_GNU_PROPERTY_TYPE_0 Properties: x86 feature: IBT +//! ``` +//! +//! Intel Indirect Branch Tracking (IBT) property is emitted. This was generated in +//! in order to address +//! . +//! +//! Note that the precompiled std currently is not compiled with `-Z cf-protection=branch`! +//! +//! In particular, it is expected that: +//! +//! > IBT to only be enabled for the process if `.note.gnu.property` indicates that the executable +//! > was compiled with IBT support and the linker to only tell that IBT is supported if all input +//! > object files indicate that they support IBT, which in turn requires the standard library to be +//! > compiled with IBT enabled. +//! +//! Note that Intel IBT (Indirect Branch Tracking) is not to be confused with Arm's BTI (Branch +//! Target Identification). See below for link to Intel IBT docs. +//! +//! ## Related links +//! +//! - [Tracking Issue for Intel Control Enforcement Technology (CET)][intel-cet-tracking-issue] +//! - Zulip question about this test: +//! +//! - Intel IBT docs: +//! +//! +//! [intel-cet-tracking-issue]: https://github.com/rust-lang/rust/issues/93754 + +//@ needs-llvm-components: x86 + +// FIXME(#93754): increase the test coverage of this test. +//@ only-x86_64-unknown-linux-gnu +//@ ignore-cross-compile + +use run_make_support::{bare_rustc, llvm_readobj}; + +fn main() { + // `main.rs` is `#![no_std]` to not pull in the currently not-compiled-with-IBT precompiled std. + bare_rustc() + .input("main.rs") + .target("x86_64-unknown-linux-gnu") + .arg("-Zcf-protection=branch") + .arg("-Clink-args=-nostartfiles") + .run(); + + llvm_readobj().arg("-nW").input("main").run().assert_stdout_contains(".note.gnu.property"); +}