From 5c0e62cd3e3df41802b73465e03aeca541aa4d00 Mon Sep 17 00:00:00 2001 From: Jules Bertholet Date: Tue, 19 Dec 2023 22:52:04 -0500 Subject: [PATCH 01/14] Hide foreign `#[doc(hidden)]` paths in import suggestions --- compiler/rustc_middle/src/ty/util.rs | 2 +- compiler/rustc_resolve/src/diagnostics.rs | 54 ++++++++++++++----- .../rustc_resolve/src/late/diagnostics.rs | 18 +++++-- .../clippy/tests/ui/crashes/ice-6252.stderr | 2 - .../ui/suggestions/auxiliary/hidden-struct.rs | 17 ++++++ .../dont-suggest-foreign-doc-hidden.rs | 15 ++++++ .../dont-suggest-foreign-doc-hidden.stderr | 25 +++++++++ .../nested-impl-trait-in-tait.rs | 4 +- .../nested-impl-trait-in-tait.stderr | 24 ++++----- 9 files changed, 126 insertions(+), 35 deletions(-) create mode 100644 tests/ui/suggestions/auxiliary/hidden-struct.rs create mode 100644 tests/ui/suggestions/dont-suggest-foreign-doc-hidden.rs create mode 100644 tests/ui/suggestions/dont-suggest-foreign-doc-hidden.stderr diff --git a/compiler/rustc_middle/src/ty/util.rs b/compiler/rustc_middle/src/ty/util.rs index 8b2b76764e646..6845de38ba3fd 100644 --- a/compiler/rustc_middle/src/ty/util.rs +++ b/compiler/rustc_middle/src/ty/util.rs @@ -1476,7 +1476,7 @@ pub fn reveal_opaque_types_in_bounds<'tcx>( val.fold_with(&mut visitor) } -/// Determines whether an item is annotated with `doc(hidden)`. +/// Determines whether an item is directly annotated with `doc(hidden)`. fn is_doc_hidden(tcx: TyCtxt<'_>, def_id: LocalDefId) -> bool { tcx.get_attrs(def_id, sym::doc) .filter_map(|attr| attr.meta_item_list()) diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 542aff69e3459..0c8e70366b95a 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -98,6 +98,8 @@ pub(crate) struct ImportSuggestion { pub descr: &'static str, pub path: Path, pub accessible: bool, + // false if the path traverses a foreign `#[doc(hidden)]` item. + pub doc_visible: bool, pub via_import: bool, /// An extra note that should be issued if this item is suggested pub note: Option, @@ -1153,10 +1155,16 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { { let mut candidates = Vec::new(); let mut seen_modules = FxHashSet::default(); - let mut worklist = vec![(start_module, ThinVec::::new(), true)]; + let start_did = start_module.def_id(); + let mut worklist = vec![( + start_module, + ThinVec::::new(), + true, + start_did.is_local() || !self.tcx.is_doc_hidden(start_did), + )]; let mut worklist_via_import = vec![]; - while let Some((in_module, path_segments, accessible)) = match worklist.pop() { + while let Some((in_module, path_segments, accessible, doc_visible)) = match worklist.pop() { None => worklist_via_import.pop(), Some(x) => Some(x), } { @@ -1199,6 +1207,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } } + let res = name_binding.res(); + let did = match res { + Res::Def(DefKind::Ctor(..), did) => this.tcx.opt_parent(did), + _ => res.opt_def_id(), + }; + let child_doc_visible = doc_visible + && (did.map_or(true, |did| did.is_local() || !this.tcx.is_doc_hidden(did))); + // collect results based on the filter function // avoid suggesting anything from the same module in which we are resolving // avoid suggesting anything with a hygienic name @@ -1207,7 +1223,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { && in_module != parent_scope.module && !ident.span.normalize_to_macros_2_0().from_expansion() { - let res = name_binding.res(); if filter_fn(res) { // create the path let mut segms = if lookup_ident.span.at_least_rust_2018() { @@ -1221,10 +1236,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { segms.push(ast::PathSegment::from_ident(ident)); let path = Path { span: name_binding.span, segments: segms, tokens: None }; - let did = match res { - Res::Def(DefKind::Ctor(..), did) => this.tcx.opt_parent(did), - _ => res.opt_def_id(), - }; if child_accessible { // Remove invisible match if exists @@ -1264,6 +1275,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { descr: res.descr(), path, accessible: child_accessible, + doc_visible: child_doc_visible, note, via_import, }); @@ -1284,7 +1296,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { // add the module to the lookup if seen_modules.insert(module.def_id()) { if via_import { &mut worklist_via_import } else { &mut worklist } - .push((module, path_segments, child_accessible)); + .push((module, path_segments, child_accessible, child_doc_visible)); } } } @@ -2694,8 +2706,26 @@ fn show_candidates( Vec::new(); candidates.iter().for_each(|c| { - (if c.accessible { &mut accessible_path_strings } else { &mut inaccessible_path_strings }) - .push((pprust::path_to_string(&c.path), c.descr, c.did, &c.note, c.via_import)) + if c.accessible { + // Don't suggest `#[doc(hidden)]` items from other crates + if c.doc_visible { + accessible_path_strings.push(( + pprust::path_to_string(&c.path), + c.descr, + c.did, + &c.note, + c.via_import, + )) + } + } else { + inaccessible_path_strings.push(( + pprust::path_to_string(&c.path), + c.descr, + c.did, + &c.note, + c.via_import, + )) + } }); // we want consistent results across executions, but candidates are produced @@ -2794,9 +2824,7 @@ fn show_candidates( err.help(msg); } true - } else if !matches!(mode, DiagnosticMode::Import) { - assert!(!inaccessible_path_strings.is_empty()); - + } else if !(inaccessible_path_strings.is_empty() || matches!(mode, DiagnosticMode::Import)) { let prefix = if let DiagnosticMode::Pattern = mode { "you might have meant to match on " } else { diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs index d767ed74139ba..54d78cfd47d2b 100644 --- a/compiler/rustc_resolve/src/late/diagnostics.rs +++ b/compiler/rustc_resolve/src/late/diagnostics.rs @@ -2194,15 +2194,20 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> { fn find_module(&mut self, def_id: DefId) -> Option<(Module<'a>, ImportSuggestion)> { let mut result = None; let mut seen_modules = FxHashSet::default(); - let mut worklist = vec![(self.r.graph_root, ThinVec::new())]; - - while let Some((in_module, path_segments)) = worklist.pop() { + let root_did = self.r.graph_root.def_id(); + let mut worklist = vec![( + self.r.graph_root, + ThinVec::new(), + root_did.is_local() || !self.r.tcx.is_doc_hidden(root_did), + )]; + + while let Some((in_module, path_segments, doc_visible)) = worklist.pop() { // abort if the module is already found if result.is_some() { break; } - in_module.for_each_child(self.r, |_, ident, _, name_binding| { + in_module.for_each_child(self.r, |r, ident, _, name_binding| { // abort if the module is already found or if name_binding is private external if result.is_some() || !name_binding.vis.is_visible_locally() { return; @@ -2212,6 +2217,8 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> { let mut path_segments = path_segments.clone(); path_segments.push(ast::PathSegment::from_ident(ident)); let module_def_id = module.def_id(); + let doc_visible = doc_visible + && (module_def_id.is_local() || !r.tcx.is_doc_hidden(module_def_id)); if module_def_id == def_id { let path = Path { span: name_binding.span, segments: path_segments, tokens: None }; @@ -2222,6 +2229,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> { descr: "module", path, accessible: true, + doc_visible, note: None, via_import: false, }, @@ -2229,7 +2237,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> { } else { // add the module to the lookup if seen_modules.insert(module_def_id) { - worklist.push((module, path_segments)); + worklist.push((module, path_segments, doc_visible)); } } } diff --git a/src/tools/clippy/tests/ui/crashes/ice-6252.stderr b/src/tools/clippy/tests/ui/crashes/ice-6252.stderr index f929bec9583c5..f6d0976091c53 100644 --- a/src/tools/clippy/tests/ui/crashes/ice-6252.stderr +++ b/src/tools/clippy/tests/ui/crashes/ice-6252.stderr @@ -8,8 +8,6 @@ help: consider importing one of these items | LL + use core::marker::PhantomData; | -LL + use serde::__private::PhantomData; - | LL + use std::marker::PhantomData; | diff --git a/tests/ui/suggestions/auxiliary/hidden-struct.rs b/tests/ui/suggestions/auxiliary/hidden-struct.rs new file mode 100644 index 0000000000000..30d69acac2097 --- /dev/null +++ b/tests/ui/suggestions/auxiliary/hidden-struct.rs @@ -0,0 +1,17 @@ +#[doc(hidden)] +pub mod hidden { + pub struct Foo; +} + +pub mod hidden1 { + #[doc(hidden)] + pub struct Foo; +} + + +#[doc(hidden)] +pub(crate) mod hidden2 { + pub struct Bar; +} + +pub use hidden2::Bar; diff --git a/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.rs b/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.rs new file mode 100644 index 0000000000000..779a0c43c02b2 --- /dev/null +++ b/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.rs @@ -0,0 +1,15 @@ +// aux-build:hidden-struct.rs +// compile-flags: --crate-type lib + +extern crate hidden_struct; + +#[doc(hidden)] +mod local { + pub struct Foo; +} + +pub fn test(_: Foo) {} +//~^ ERROR cannot find type `Foo` in this scope + +pub fn test2(_: Bar) {} +//~^ ERROR cannot find type `Bar` in this scope diff --git a/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.stderr b/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.stderr new file mode 100644 index 0000000000000..7fb4d95ff9bf5 --- /dev/null +++ b/tests/ui/suggestions/dont-suggest-foreign-doc-hidden.stderr @@ -0,0 +1,25 @@ +error[E0412]: cannot find type `Foo` in this scope + --> $DIR/dont-suggest-foreign-doc-hidden.rs:11:16 + | +LL | pub fn test(_: Foo) {} + | ^^^ not found in this scope + | +help: consider importing this struct + | +LL + use local::Foo; + | + +error[E0412]: cannot find type `Bar` in this scope + --> $DIR/dont-suggest-foreign-doc-hidden.rs:14:17 + | +LL | pub fn test2(_: Bar) {} + | ^^^ not found in this scope + | +help: consider importing this struct + | +LL + use hidden_struct::Bar; + | + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0412`. diff --git a/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.rs b/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.rs index fec0fdc46fbb6..6a74d1dc4ef38 100644 --- a/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.rs +++ b/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.rs @@ -1,8 +1,8 @@ #![feature(type_alias_impl_trait)] -pub type Tait = impl Iterator; +pub type Tait = impl Iterator; //~^ ERROR use of undeclared lifetime name `'db` -//~| ERROR cannot find type `Key` in this scope +//~| ERROR cannot find type `LocalKey` in this scope //~| ERROR unconstrained opaque type //~| ERROR unconstrained opaque type diff --git a/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.stderr b/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.stderr index d4aeace4ae707..ca15b134a9947 100644 --- a/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.stderr +++ b/tests/ui/type-alias-impl-trait/nested-impl-trait-in-tait.stderr @@ -1,43 +1,43 @@ error[E0261]: use of undeclared lifetime name `'db` --> $DIR/nested-impl-trait-in-tait.rs:3:40 | -LL | pub type Tait = impl Iterator; +LL | pub type Tait = impl Iterator; | ^^^ undeclared lifetime | = note: for more information on higher-ranked polymorphism, visit https://doc.rust-lang.org/nomicon/hrtb.html help: consider making the bound lifetime-generic with a new `'db` lifetime | -LL | pub type Tait = impl for<'db> Iterator; +LL | pub type Tait = impl for<'db> Iterator; | ++++++++ help: consider introducing lifetime `'db` here | -LL | pub type Tait<'db> = impl Iterator; +LL | pub type Tait<'db> = impl Iterator; | +++++ -error[E0412]: cannot find type `Key` in this scope +error[E0412]: cannot find type `LocalKey` in this scope --> $DIR/nested-impl-trait-in-tait.rs:3:44 | -LL | pub type Tait = impl Iterator; - | ^^^ not found in this scope +LL | pub type Tait = impl Iterator; + | ^^^^^^^^ not found in this scope | help: consider importing this struct | -LL + use std::thread::local_impl::Key; +LL + use std::thread::LocalKey; | error: unconstrained opaque type --> $DIR/nested-impl-trait-in-tait.rs:3:17 | -LL | pub type Tait = impl Iterator; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | pub type Tait = impl Iterator; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `Tait` must be used in combination with a concrete type within the same module error: unconstrained opaque type - --> $DIR/nested-impl-trait-in-tait.rs:3:49 + --> $DIR/nested-impl-trait-in-tait.rs:3:54 | -LL | pub type Tait = impl Iterator; - | ^^^^^^^^^^^^^ +LL | pub type Tait = impl Iterator; + | ^^^^^^^^^^^^^ | = note: `Tait` must be used in combination with a concrete type within the same module From 5f5646562185e5eb3e3b4a5d45d4bd37585a0040 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 27 Dec 2023 18:53:06 +0100 Subject: [PATCH 02/14] Make feature `negative_bounds` internal --- compiler/rustc_feature/src/unstable.rs | 2 +- .../negative-bounds/associated-constraints.rs | 1 - .../associated-constraints.stderr | 19 ++++---------- tests/ui/traits/negative-bounds/simple.rs | 1 - tests/ui/traits/negative-bounds/simple.stderr | 26 +++++++------------ tests/ui/traits/negative-bounds/supertrait.rs | 1 - .../traits/negative-bounds/supertrait.stderr | 10 ------- 7 files changed, 15 insertions(+), 45 deletions(-) delete mode 100644 tests/ui/traits/negative-bounds/supertrait.stderr diff --git a/compiler/rustc_feature/src/unstable.rs b/compiler/rustc_feature/src/unstable.rs index 763bd4fc3916b..904847d09290b 100644 --- a/compiler/rustc_feature/src/unstable.rs +++ b/compiler/rustc_feature/src/unstable.rs @@ -210,7 +210,7 @@ declare_features! ( /// Allows the `multiple_supertrait_upcastable` lint. (unstable, multiple_supertrait_upcastable, "1.69.0", None), /// Allow negative trait bounds. This is an internal-only feature for testing the trait solver! - (incomplete, negative_bounds, "1.71.0", None), + (internal, negative_bounds, "1.71.0", None), /// Allows using `#[omit_gdb_pretty_printer_section]`. (internal, omit_gdb_pretty_printer_section, "1.5.0", None), /// Allows using `#[prelude_import]` on glob `use` items. diff --git a/tests/ui/traits/negative-bounds/associated-constraints.rs b/tests/ui/traits/negative-bounds/associated-constraints.rs index bc1a0ef170837..23ca2e41f3443 100644 --- a/tests/ui/traits/negative-bounds/associated-constraints.rs +++ b/tests/ui/traits/negative-bounds/associated-constraints.rs @@ -1,5 +1,4 @@ #![feature(negative_bounds, associated_type_bounds)] -//~^ WARN the feature `negative_bounds` is incomplete and may not be safe to use and/or cause compiler crashes trait Trait { type Assoc; diff --git a/tests/ui/traits/negative-bounds/associated-constraints.stderr b/tests/ui/traits/negative-bounds/associated-constraints.stderr index 335ac7e5ad903..eae0ba4e6e411 100644 --- a/tests/ui/traits/negative-bounds/associated-constraints.stderr +++ b/tests/ui/traits/negative-bounds/associated-constraints.stderr @@ -1,34 +1,25 @@ error: associated type constraints not allowed on negative bounds - --> $DIR/associated-constraints.rs:8:19 + --> $DIR/associated-constraints.rs:7:19 | LL | fn test>() {} | ^^^^^^^^^^^ error: associated type constraints not allowed on negative bounds - --> $DIR/associated-constraints.rs:11:31 + --> $DIR/associated-constraints.rs:10:31 | LL | fn test2() where T: !Trait {} | ^^^^^^^^^^^ error: associated type constraints not allowed on negative bounds - --> $DIR/associated-constraints.rs:14:20 + --> $DIR/associated-constraints.rs:13:20 | LL | fn test3>() {} | ^^^^^^^^^^^ error: associated type constraints not allowed on negative bounds - --> $DIR/associated-constraints.rs:17:31 + --> $DIR/associated-constraints.rs:16:31 | LL | fn test4() where T: !Trait {} | ^^^^^^^^^^^ -warning: the feature `negative_bounds` is incomplete and may not be safe to use and/or cause compiler crashes - --> $DIR/associated-constraints.rs:1:12 - | -LL | #![feature(negative_bounds, associated_type_bounds)] - | ^^^^^^^^^^^^^^^ - | - = note: `#[warn(incomplete_features)]` on by default - -error: aborting due to 4 previous errors; 1 warning emitted - +error: aborting due to 4 previous errors diff --git a/tests/ui/traits/negative-bounds/simple.rs b/tests/ui/traits/negative-bounds/simple.rs index f6d1d5169c4fc..a2febf353f6d3 100644 --- a/tests/ui/traits/negative-bounds/simple.rs +++ b/tests/ui/traits/negative-bounds/simple.rs @@ -1,5 +1,4 @@ #![feature(negative_bounds, negative_impls)] -//~^ WARN the feature `negative_bounds` is incomplete and may not be safe to use and/or cause compiler crashes fn not_copy() {} diff --git a/tests/ui/traits/negative-bounds/simple.stderr b/tests/ui/traits/negative-bounds/simple.stderr index a3cab41a2ce0f..6d750739e197c 100644 --- a/tests/ui/traits/negative-bounds/simple.stderr +++ b/tests/ui/traits/negative-bounds/simple.stderr @@ -1,44 +1,36 @@ -warning: the feature `negative_bounds` is incomplete and may not be safe to use and/or cause compiler crashes - --> $DIR/simple.rs:1:12 - | -LL | #![feature(negative_bounds, negative_impls)] - | ^^^^^^^^^^^^^^^ - | - = note: `#[warn(incomplete_features)]` on by default - error[E0277]: the trait bound `T: !Copy` is not satisfied - --> $DIR/simple.rs:11:16 + --> $DIR/simple.rs:10:16 | LL | not_copy::(); | ^ the trait `!Copy` is not implemented for `T` | note: required by a bound in `not_copy` - --> $DIR/simple.rs:4:16 + --> $DIR/simple.rs:3:16 | LL | fn not_copy() {} | ^^^^^ required by this bound in `not_copy` error[E0277]: the trait bound `T: !Copy` is not satisfied - --> $DIR/simple.rs:16:16 + --> $DIR/simple.rs:15:16 | LL | not_copy::(); | ^ the trait `!Copy` is not implemented for `T` | note: required by a bound in `not_copy` - --> $DIR/simple.rs:4:16 + --> $DIR/simple.rs:3:16 | LL | fn not_copy() {} | ^^^^^ required by this bound in `not_copy` error[E0277]: the trait bound `Copyable: !Copy` is not satisfied - --> $DIR/simple.rs:31:16 + --> $DIR/simple.rs:30:16 | LL | not_copy::(); | ^^^^^^^^ the trait `!Copy` is not implemented for `Copyable` | = help: the trait `Copy` is implemented for `Copyable` note: required by a bound in `not_copy` - --> $DIR/simple.rs:4:16 + --> $DIR/simple.rs:3:16 | LL | fn not_copy() {} | ^^^^^ required by this bound in `not_copy` @@ -49,13 +41,13 @@ LL | struct Copyable; | error[E0277]: the trait bound `NotNecessarilyCopyable: !Copy` is not satisfied - --> $DIR/simple.rs:38:16 + --> $DIR/simple.rs:37:16 | LL | not_copy::(); | ^^^^^^^^^^^^^^^^^^^^^^ the trait `!Copy` is not implemented for `NotNecessarilyCopyable` | note: required by a bound in `not_copy` - --> $DIR/simple.rs:4:16 + --> $DIR/simple.rs:3:16 | LL | fn not_copy() {} | ^^^^^ required by this bound in `not_copy` @@ -65,6 +57,6 @@ LL + #[derive(Copy)] LL | struct NotNecessarilyCopyable; | -error: aborting due to 4 previous errors; 1 warning emitted +error: aborting due to 4 previous errors For more information about this error, try `rustc --explain E0277`. diff --git a/tests/ui/traits/negative-bounds/supertrait.rs b/tests/ui/traits/negative-bounds/supertrait.rs index df0884b8b9f16..a66bc4a60a08e 100644 --- a/tests/ui/traits/negative-bounds/supertrait.rs +++ b/tests/ui/traits/negative-bounds/supertrait.rs @@ -1,7 +1,6 @@ // check-pass #![feature(negative_bounds)] -//~^ WARN the feature `negative_bounds` is incomplete trait A: !B {} trait B: !A {} diff --git a/tests/ui/traits/negative-bounds/supertrait.stderr b/tests/ui/traits/negative-bounds/supertrait.stderr deleted file mode 100644 index f44753b624e3f..0000000000000 --- a/tests/ui/traits/negative-bounds/supertrait.stderr +++ /dev/null @@ -1,10 +0,0 @@ -warning: the feature `negative_bounds` is incomplete and may not be safe to use and/or cause compiler crashes - --> $DIR/supertrait.rs:3:12 - | -LL | #![feature(negative_bounds)] - | ^^^^^^^^^^^^^^^ - | - = note: `#[warn(incomplete_features)]` on by default - -warning: 1 warning emitted - From a25197401545551ca4f15ac75993ab7cad1b0e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 27 Dec 2023 17:57:19 +0100 Subject: [PATCH 03/14] Deny parenthetical notation for negative bounds --- compiler/rustc_ast_passes/messages.ftl | 3 +++ .../rustc_ast_passes/src/ast_validation.rs | 21 ++++++++++++++----- compiler/rustc_ast_passes/src/errors.rs | 7 +++++++ .../negative-bounds/associated-constraints.rs | 3 +++ .../associated-constraints.stderr | 9 +++++++- 5 files changed, 37 insertions(+), 6 deletions(-) diff --git a/compiler/rustc_ast_passes/messages.ftl b/compiler/rustc_ast_passes/messages.ftl index b5612c1820d0c..228f346305891 100644 --- a/compiler/rustc_ast_passes/messages.ftl +++ b/compiler/rustc_ast_passes/messages.ftl @@ -188,6 +188,9 @@ ast_passes_module_nonascii = trying to load file for module `{$name}` with non-a ast_passes_negative_bound_not_supported = negative bounds are not supported +ast_passes_negative_bound_with_parenthetical_notation = + parenthetical notation may not be used for negative bounds + ast_passes_nested_impl_trait = nested `impl Trait` is not allowed .outer = outer `impl Trait` .inner = nested `impl Trait` here diff --git a/compiler/rustc_ast_passes/src/ast_validation.rs b/compiler/rustc_ast_passes/src/ast_validation.rs index bc5cf463f1204..cd41e6a99b1e1 100644 --- a/compiler/rustc_ast_passes/src/ast_validation.rs +++ b/compiler/rustc_ast_passes/src/ast_validation.rs @@ -1257,13 +1257,24 @@ impl<'a> Visitor<'a> for AstValidator<'a> { if let GenericBound::Trait(trait_ref, modifiers) = bound && let BoundPolarity::Negative(_) = modifiers.polarity && let Some(segment) = trait_ref.trait_ref.path.segments.last() - && let Some(ast::GenericArgs::AngleBracketed(args)) = segment.args.as_deref() { - for arg in &args.args { - if let ast::AngleBracketedArg::Constraint(constraint) = arg { - self.dcx() - .emit_err(errors::ConstraintOnNegativeBound { span: constraint.span }); + match segment.args.as_deref() { + Some(ast::GenericArgs::AngleBracketed(args)) => { + for arg in &args.args { + if let ast::AngleBracketedArg::Constraint(constraint) = arg { + self.dcx().emit_err(errors::ConstraintOnNegativeBound { + span: constraint.span, + }); + } + } + } + // The lowered form of parenthesized generic args contains a type binding. + Some(ast::GenericArgs::Parenthesized(args)) => { + self.dcx().emit_err(errors::NegativeBoundWithParentheticalNotation { + span: args.span, + }); } + None => {} } } diff --git a/compiler/rustc_ast_passes/src/errors.rs b/compiler/rustc_ast_passes/src/errors.rs index 0cec4374be2ee..c5c6d45a63d90 100644 --- a/compiler/rustc_ast_passes/src/errors.rs +++ b/compiler/rustc_ast_passes/src/errors.rs @@ -745,6 +745,13 @@ pub struct ConstraintOnNegativeBound { pub span: Span, } +#[derive(Diagnostic)] +#[diag(ast_passes_negative_bound_with_parenthetical_notation)] +pub struct NegativeBoundWithParentheticalNotation { + #[primary_span] + pub span: Span, +} + #[derive(Diagnostic)] #[diag(ast_passes_invalid_unnamed_field_ty)] pub struct InvalidUnnamedFieldTy { diff --git a/tests/ui/traits/negative-bounds/associated-constraints.rs b/tests/ui/traits/negative-bounds/associated-constraints.rs index 23ca2e41f3443..4a7132ccde917 100644 --- a/tests/ui/traits/negative-bounds/associated-constraints.rs +++ b/tests/ui/traits/negative-bounds/associated-constraints.rs @@ -16,4 +16,7 @@ fn test3>() {} fn test4() where T: !Trait {} //~^ ERROR associated type constraints not allowed on negative bounds +fn test5() where T: !Fn() -> i32 {} +//~^ ERROR parenthetical notation may not be used for negative bounds + fn main() {} diff --git a/tests/ui/traits/negative-bounds/associated-constraints.stderr b/tests/ui/traits/negative-bounds/associated-constraints.stderr index eae0ba4e6e411..c1a6d2ca6a2e2 100644 --- a/tests/ui/traits/negative-bounds/associated-constraints.stderr +++ b/tests/ui/traits/negative-bounds/associated-constraints.stderr @@ -22,4 +22,11 @@ error: associated type constraints not allowed on negative bounds LL | fn test4() where T: !Trait {} | ^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: parenthetical notation may not be used for negative bounds + --> $DIR/associated-constraints.rs:19:25 + | +LL | fn test5() where T: !Fn() -> i32 {} + | ^^^^^^^^^^^ + +error: aborting due to 5 previous errors + From 32cea61c86d35a6536669f011a35dec2687ef5ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 27 Dec 2023 23:04:12 +0100 Subject: [PATCH 04/14] Don't elaborate `!Sized` to `!Sized + Sized` --- .../rustc_hir_analysis/src/astconv/bounds.rs | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/compiler/rustc_hir_analysis/src/astconv/bounds.rs b/compiler/rustc_hir_analysis/src/astconv/bounds.rs index 91b3807d74462..d403f1a850d9c 100644 --- a/compiler/rustc_hir_analysis/src/astconv/bounds.rs +++ b/compiler/rustc_hir_analysis/src/astconv/bounds.rs @@ -26,23 +26,36 @@ impl<'tcx> dyn AstConv<'tcx> + '_ { span: Span, ) { let tcx = self.tcx(); + let sized_def_id = tcx.lang_items().sized_trait(); + let mut seen_negative_sized_bound = false; // Try to find an unbound in bounds. let mut unbounds: SmallVec<[_; 1]> = SmallVec::new(); let mut search_bounds = |ast_bounds: &'tcx [hir::GenericBound<'tcx>]| { for ab in ast_bounds { - if let hir::GenericBound::Trait(ptr, hir::TraitBoundModifier::Maybe) = ab { - unbounds.push(ptr) + let hir::GenericBound::Trait(ptr, modifier) = ab else { + continue; + }; + match modifier { + hir::TraitBoundModifier::Maybe => unbounds.push(ptr), + hir::TraitBoundModifier::Negative => { + if let Some(sized_def_id) = sized_def_id + && ptr.trait_ref.path.res == Res::Def(DefKind::Trait, sized_def_id) + { + seen_negative_sized_bound = true; + } + } + _ => {} } } }; search_bounds(ast_bounds); if let Some((self_ty, where_clause)) = self_ty_where_predicates { for clause in where_clause { - if let hir::WherePredicate::BoundPredicate(pred) = clause { - if pred.is_param_bound(self_ty.to_def_id()) { - search_bounds(pred.bounds); - } + if let hir::WherePredicate::BoundPredicate(pred) = clause + && pred.is_param_bound(self_ty.to_def_id()) + { + search_bounds(pred.bounds); } } } @@ -53,15 +66,13 @@ impl<'tcx> dyn AstConv<'tcx> + '_ { }); } - let sized_def_id = tcx.lang_items().sized_trait(); - let mut seen_sized_unbound = false; for unbound in unbounds { - if let Some(sized_def_id) = sized_def_id { - if unbound.trait_ref.path.res == Res::Def(DefKind::Trait, sized_def_id) { - seen_sized_unbound = true; - continue; - } + if let Some(sized_def_id) = sized_def_id + && unbound.trait_ref.path.res == Res::Def(DefKind::Trait, sized_def_id) + { + seen_sized_unbound = true; + continue; } // There was a `?Trait` bound, but it was not `?Sized`; warn. tcx.dcx().span_warn( @@ -71,15 +82,12 @@ impl<'tcx> dyn AstConv<'tcx> + '_ { ); } - // If the above loop finished there was no `?Sized` bound; add implicitly sized if `Sized` is available. - if sized_def_id.is_none() { - // No lang item for `Sized`, so we can't add it as a bound. - return; - } - if seen_sized_unbound { - // There was in fact a `?Sized` bound, return without doing anything - } else { - // There was no `?Sized` bound; add implicitly sized if `Sized` is available. + if seen_sized_unbound || seen_negative_sized_bound { + // There was in fact a `?Sized` or `!Sized` bound; + // we don't need to do anything. + } else if sized_def_id.is_some() { + // There was no `?Sized` or `!Sized` bound; + // add `Sized` if it's available. bounds.push_sized(tcx, self_ty, span); } } From 977546d3fc9f48702046214749521da4c459f614 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 27 Dec 2023 17:57:55 +0100 Subject: [PATCH 05/14] rustc_middle: Pretty-print negative bounds correctly --- compiler/rustc_middle/src/ty/print/pretty.rs | 72 +++++++++++++------ .../opaque-type-unsatisfied-bound.rs | 23 ++++++ .../opaque-type-unsatisfied-bound.stderr | 69 ++++++++++++++++++ .../opaque-type-unsatisfied-fn-bound.rs | 9 +++ .../opaque-type-unsatisfied-fn-bound.stderr | 21 ++++++ 5 files changed, 173 insertions(+), 21 deletions(-) create mode 100644 tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.rs create mode 100644 tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.stderr create mode 100644 tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.rs create mode 100644 tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.stderr diff --git a/compiler/rustc_middle/src/ty/print/pretty.rs b/compiler/rustc_middle/src/ty/print/pretty.rs index f7900d883ad37..6841685b0c457 100644 --- a/compiler/rustc_middle/src/ty/print/pretty.rs +++ b/compiler/rustc_middle/src/ty/print/pretty.rs @@ -912,7 +912,8 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { let mut traits = FxIndexMap::default(); let mut fn_traits = FxIndexMap::default(); - let mut is_sized = false; + let mut has_sized_bound = false; + let mut has_negative_sized_bound = false; let mut lifetimes = SmallVec::<[ty::Region<'tcx>; 1]>::new(); for (predicate, _) in bounds.iter_instantiated_copied(tcx, args) { @@ -922,13 +923,24 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { ty::ClauseKind::Trait(pred) => { let trait_ref = bound_predicate.rebind(pred.trait_ref); - // Don't print + Sized, but rather + ?Sized if absent. + // Don't print `+ Sized`, but rather `+ ?Sized` if absent. if Some(trait_ref.def_id()) == tcx.lang_items().sized_trait() { - is_sized = true; - continue; + match pred.polarity { + ty::ImplPolarity::Positive | ty::ImplPolarity::Reservation => { + has_sized_bound = true; + continue; + } + ty::ImplPolarity::Negative => has_negative_sized_bound = true, + } } - self.insert_trait_and_projection(trait_ref, None, &mut traits, &mut fn_traits); + self.insert_trait_and_projection( + trait_ref, + pred.polarity, + None, + &mut traits, + &mut fn_traits, + ); } ty::ClauseKind::Projection(pred) => { let proj_ref = bound_predicate.rebind(pred); @@ -939,6 +951,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { self.insert_trait_and_projection( trait_ref, + ty::ImplPolarity::Positive, Some(proj_ty), &mut traits, &mut fn_traits, @@ -955,7 +968,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { let mut first = true; // Insert parenthesis around (Fn(A, B) -> C) if the opaque ty has more than one other trait - let paren_needed = fn_traits.len() > 1 || traits.len() > 0 || !is_sized; + let paren_needed = fn_traits.len() > 1 || traits.len() > 0 || !has_sized_bound; for (fn_once_trait_ref, entry) in fn_traits { write!(self, "{}", if first { "" } else { " + " })?; @@ -1002,18 +1015,21 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { // trait_refs we collected in the OpaqueFnEntry as normal trait refs. _ => { if entry.has_fn_once { - traits.entry(fn_once_trait_ref).or_default().extend( - // Group the return ty with its def id, if we had one. - entry - .return_ty - .map(|ty| (tcx.require_lang_item(LangItem::FnOnce, None), ty)), - ); + traits + .entry((fn_once_trait_ref, ty::ImplPolarity::Positive)) + .or_default() + .extend( + // Group the return ty with its def id, if we had one. + entry.return_ty.map(|ty| { + (tcx.require_lang_item(LangItem::FnOnce, None), ty) + }), + ); } if let Some(trait_ref) = entry.fn_mut_trait_ref { - traits.entry(trait_ref).or_default(); + traits.entry((trait_ref, ty::ImplPolarity::Positive)).or_default(); } if let Some(trait_ref) = entry.fn_trait_ref { - traits.entry(trait_ref).or_default(); + traits.entry((trait_ref, ty::ImplPolarity::Positive)).or_default(); } } } @@ -1023,11 +1039,15 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { } // Print the rest of the trait types (that aren't Fn* family of traits) - for (trait_ref, assoc_items) in traits { + for ((trait_ref, polarity), assoc_items) in traits { write!(self, "{}", if first { "" } else { " + " })?; self.wrap_binder(&trait_ref, |trait_ref, cx| { define_scoped_cx!(cx); + + if polarity == ty::ImplPolarity::Negative { + p!("!"); + } p!(print(trait_ref.print_only_trait_name())); let generics = tcx.generics_of(trait_ref.def_id); @@ -1094,9 +1114,15 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { })?; } - if !is_sized { - write!(self, "{}?Sized", if first { "" } else { " + " })?; - } else if first { + let add_sized = has_sized_bound && (first || has_negative_sized_bound); + let add_maybe_sized = !has_sized_bound && !has_negative_sized_bound; + if add_sized || add_maybe_sized { + if !first { + write!(self, " + ")?; + } + if add_maybe_sized { + write!(self, "?")?; + } write!(self, "Sized")?; } @@ -1128,9 +1154,10 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { fn insert_trait_and_projection( &mut self, trait_ref: ty::PolyTraitRef<'tcx>, + polarity: ty::ImplPolarity, proj_ty: Option<(DefId, ty::Binder<'tcx, Term<'tcx>>)>, traits: &mut FxIndexMap< - ty::PolyTraitRef<'tcx>, + (ty::PolyTraitRef<'tcx>, ty::ImplPolarity), FxIndexMap>>, >, fn_traits: &mut FxIndexMap, OpaqueFnEntry<'tcx>>, @@ -1139,7 +1166,10 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { // If our trait_ref is FnOnce or any of its children, project it onto the parent FnOnce // super-trait ref and record it there. - if let Some(fn_once_trait) = self.tcx().lang_items().fn_once_trait() { + // We skip negative Fn* bounds since they can't use parenthetical notation anyway. + if polarity == ty::ImplPolarity::Positive + && let Some(fn_once_trait) = self.tcx().lang_items().fn_once_trait() + { // If we have a FnOnce, then insert it into if trait_def_id == fn_once_trait { let entry = fn_traits.entry(trait_ref).or_default(); @@ -1167,7 +1197,7 @@ pub trait PrettyPrinter<'tcx>: Printer<'tcx> + fmt::Write { } // Otherwise, just group our traits and projection types. - traits.entry(trait_ref).or_default().extend(proj_ty); + traits.entry((trait_ref, polarity)).or_default().extend(proj_ty); } fn pretty_print_inherent_projection( diff --git a/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.rs b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.rs new file mode 100644 index 0000000000000..e1e93f7992065 --- /dev/null +++ b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.rs @@ -0,0 +1,23 @@ +// compile-flags: -Znext-solver + +#![feature(negative_bounds, negative_impls)] + +trait Trait {} +impl !Trait for () {} + +fn produce() -> impl !Trait {} +fn consume(_: impl Trait) {} + +fn main() { + consume(produce()); //~ ERROR the trait bound `impl !Trait: Trait` is not satisfied +} + +fn weird0() -> impl Sized + !Sized {} +//~^ ERROR mismatched types +//~| ERROR type mismatch resolving `() == impl !Sized + Sized` +fn weird1() -> impl !Sized + Sized {} +//~^ ERROR mismatched types +//~| ERROR type mismatch resolving `() == impl !Sized + Sized` +fn weird2() -> impl !Sized {} +//~^ ERROR mismatched types +//~| ERROR type mismatch resolving `() == impl !Sized` diff --git a/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.stderr b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.stderr new file mode 100644 index 0000000000000..627927618707f --- /dev/null +++ b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-bound.stderr @@ -0,0 +1,69 @@ +error[E0308]: mismatched types + --> $DIR/opaque-type-unsatisfied-bound.rs:15:36 + | +LL | fn weird0() -> impl Sized + !Sized {} + | ------------------- ^^ types differ + | | + | the expected opaque type + | + = note: expected opaque type `impl !Sized + Sized` + found unit type `()` + +error[E0271]: type mismatch resolving `() == impl !Sized + Sized` + --> $DIR/opaque-type-unsatisfied-bound.rs:15:16 + | +LL | fn weird0() -> impl Sized + !Sized {} + | ^^^^^^^^^^^^^^^^^^^ types differ + +error[E0308]: mismatched types + --> $DIR/opaque-type-unsatisfied-bound.rs:18:36 + | +LL | fn weird1() -> impl !Sized + Sized {} + | ------------------- ^^ types differ + | | + | the expected opaque type + | + = note: expected opaque type `impl !Sized + Sized` + found unit type `()` + +error[E0271]: type mismatch resolving `() == impl !Sized + Sized` + --> $DIR/opaque-type-unsatisfied-bound.rs:18:16 + | +LL | fn weird1() -> impl !Sized + Sized {} + | ^^^^^^^^^^^^^^^^^^^ types differ + +error[E0308]: mismatched types + --> $DIR/opaque-type-unsatisfied-bound.rs:21:28 + | +LL | fn weird2() -> impl !Sized {} + | ----------- ^^ types differ + | | + | the expected opaque type + | + = note: expected opaque type `impl !Sized` + found unit type `()` + +error[E0271]: type mismatch resolving `() == impl !Sized` + --> $DIR/opaque-type-unsatisfied-bound.rs:21:16 + | +LL | fn weird2() -> impl !Sized {} + | ^^^^^^^^^^^ types differ + +error[E0277]: the trait bound `impl !Trait: Trait` is not satisfied + --> $DIR/opaque-type-unsatisfied-bound.rs:12:13 + | +LL | consume(produce()); + | ------- ^^^^^^^^^ the trait `Trait` is not implemented for `impl !Trait` + | | + | required by a bound introduced by this call + | +note: required by a bound in `consume` + --> $DIR/opaque-type-unsatisfied-bound.rs:9:20 + | +LL | fn consume(_: impl Trait) {} + | ^^^^^ required by this bound in `consume` + +error: aborting due to 7 previous errors + +Some errors have detailed explanations: E0271, E0277, E0308. +For more information about an error, try `rustc --explain E0271`. diff --git a/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.rs b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.rs new file mode 100644 index 0000000000000..72bca1a8910b5 --- /dev/null +++ b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.rs @@ -0,0 +1,9 @@ +// compile-flags: -Znext-solver + +#![feature(negative_bounds, unboxed_closures)] + +fn produce() -> impl !Fn<(u32,)> {} +//~^ ERROR mismatched types +//~| ERROR type mismatch resolving `() == impl !Fn<(u32,)>` + +fn main() {} diff --git a/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.stderr b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.stderr new file mode 100644 index 0000000000000..a4fb4b2b5c47f --- /dev/null +++ b/tests/ui/traits/negative-bounds/opaque-type-unsatisfied-fn-bound.stderr @@ -0,0 +1,21 @@ +error[E0308]: mismatched types + --> $DIR/opaque-type-unsatisfied-fn-bound.rs:5:34 + | +LL | fn produce() -> impl !Fn<(u32,)> {} + | ---------------- ^^ types differ + | | + | the expected opaque type + | + = note: expected opaque type `impl !Fn<(u32,)>` + found unit type `()` + +error[E0271]: type mismatch resolving `() == impl !Fn<(u32,)>` + --> $DIR/opaque-type-unsatisfied-fn-bound.rs:5:17 + | +LL | fn produce() -> impl !Fn<(u32,)> {} + | ^^^^^^^^^^^^^^^^ types differ + +error: aborting due to 2 previous errors + +Some errors have detailed explanations: E0271, E0308. +For more information about an error, try `rustc --explain E0271`. From 90d6fe2cba2b621b4fc331a1f6dd67c63a7c1033 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Wed, 27 Dec 2023 15:29:52 +0100 Subject: [PATCH 06/14] Imply outlives-bounds on lazy type aliases --- .../src/outlives/implicit_infer.rs | 179 +++++++++++------- .../rustc_hir_analysis/src/outlives/mod.rs | 8 +- .../associated-type-bounds/duplicate.stderr | 48 ++--- .../implied-outlives-bounds.neg.stderr | 34 ++++ .../implied-outlives-bounds.rs | 39 ++++ 5 files changed, 211 insertions(+), 97 deletions(-) create mode 100644 tests/ui/lazy-type-alias/implied-outlives-bounds.neg.stderr create mode 100644 tests/ui/lazy-type-alias/implied-outlives-bounds.rs diff --git a/compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs b/compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs index c17925471d983..0cb38094ceca5 100644 --- a/compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs +++ b/compiler/rustc_hir_analysis/src/outlives/implicit_infer.rs @@ -59,6 +59,17 @@ pub(super) fn infer_predicates( } } + DefKind::TyAlias if tcx.type_alias_is_lazy(item_did) => { + insert_required_predicates_to_be_wf( + tcx, + tcx.type_of(item_did).instantiate_identity(), + tcx.def_span(item_did), + &global_inferred_outlives, + &mut item_required_predicates, + &mut explicit_map, + ); + } + _ => {} }; @@ -88,14 +99,14 @@ pub(super) fn infer_predicates( fn insert_required_predicates_to_be_wf<'tcx>( tcx: TyCtxt<'tcx>, - field_ty: Ty<'tcx>, - field_span: Span, + ty: Ty<'tcx>, + span: Span, global_inferred_outlives: &FxHashMap>>, required_predicates: &mut RequiredPredicates<'tcx>, explicit_map: &mut ExplicitPredicatesMap<'tcx>, ) { - for arg in field_ty.walk() { - let ty = match arg.unpack() { + for arg in ty.walk() { + let leaf_ty = match arg.unpack() { GenericArgKind::Type(ty) => ty, // No predicates from lifetimes or constants, except potentially @@ -103,63 +114,26 @@ fn insert_required_predicates_to_be_wf<'tcx>( GenericArgKind::Lifetime(_) | GenericArgKind::Const(_) => continue, }; - match *ty.kind() { - // The field is of type &'a T which means that we will have - // a predicate requirement of T: 'a (T outlives 'a). - // - // We also want to calculate potential predicates for the T + match *leaf_ty.kind() { ty::Ref(region, rty, _) => { + // The type is `&'a T` which means that we will have + // a predicate requirement of `T: 'a` (`T` outlives `'a`). + // + // We also want to calculate potential predicates for the `T`. debug!("Ref"); - insert_outlives_predicate(tcx, rty.into(), region, field_span, required_predicates); + insert_outlives_predicate(tcx, rty.into(), region, span, required_predicates); } - // For each Adt (struct/enum/union) type `Foo<'a, T>`, we - // can load the current set of inferred and explicit - // predicates from `global_inferred_outlives` and filter the - // ones that are TypeOutlives. ty::Adt(def, args) => { - // First check the inferred predicates - // - // Example 1: - // - // struct Foo<'a, T> { - // field1: Bar<'a, T> - // } - // - // struct Bar<'b, U> { - // field2: &'b U - // } - // - // Here, when processing the type of `field1`, we would - // request the set of implicit predicates computed for `Bar` - // thus far. This will initially come back empty, but in next - // round we will get `U: 'b`. We then apply the substitution - // `['b => 'a, U => T]` and thus get the requirement that `T: - // 'a` holds for `Foo`. + // For ADTs (structs/enums/unions), we check inferred and explicit predicates. debug!("Adt"); - if let Some(unsubstituted_predicates) = global_inferred_outlives.get(&def.did()) { - for (unsubstituted_predicate, &span) in - unsubstituted_predicates.as_ref().skip_binder() - { - // `unsubstituted_predicate` is `U: 'b` in the - // example above. So apply the substitution to - // get `T: 'a` (or `predicate`): - let predicate = unsubstituted_predicates - .rebind(*unsubstituted_predicate) - .instantiate(tcx, args); - insert_outlives_predicate( - tcx, - predicate.0, - predicate.1, - span, - required_predicates, - ); - } - } - - // Check if the type has any explicit predicates that need - // to be added to `required_predicates` - // let _: () = args.region_at(0); + check_inferred_predicates( + tcx, + def.did(), + args, + global_inferred_outlives, + required_predicates, + ); check_explicit_predicates( tcx, def.did(), @@ -170,13 +144,31 @@ fn insert_required_predicates_to_be_wf<'tcx>( ); } + ty::Alias(ty::Weak, alias) => { + // This corresponds to a type like `Type<'a, T>`. + // We check inferred and explicit predicates. + debug!("Weak"); + check_inferred_predicates( + tcx, + alias.def_id, + alias.args, + global_inferred_outlives, + required_predicates, + ); + check_explicit_predicates( + tcx, + alias.def_id, + alias.args, + required_predicates, + explicit_map, + None, + ); + } + ty::Dynamic(obj, ..) => { // This corresponds to `dyn Trait<..>`. In this case, we should // use the explicit predicates as well. - debug!("Dynamic"); - debug!("field_ty = {}", &field_ty); - debug!("ty in field = {}", &ty); if let Some(ex_trait_ref) = obj.principal() { // Here, we are passing the type `usize` as a // placeholder value with the function @@ -198,21 +190,22 @@ fn insert_required_predicates_to_be_wf<'tcx>( } } - ty::Alias(ty::Projection, obj) => { - // This corresponds to `>::Bar`. In this case, we should use the - // explicit predicates as well. + ty::Alias(ty::Projection, alias) => { + // This corresponds to a type like `<() as Trait<'a, T>>::Type`. + // We only use the explicit predicates of the trait but + // not the ones of the associated type itself. debug!("Projection"); check_explicit_predicates( tcx, - tcx.parent(obj.def_id), - obj.args, + tcx.parent(alias.def_id), + alias.args, required_predicates, explicit_map, None, ); } - // FIXME(inherent_associated_types): Handle this case properly. + // FIXME(inherent_associated_types): Use the explicit predicates from the parent impl. ty::Alias(ty::Inherent, _) => {} _ => {} @@ -220,19 +213,21 @@ fn insert_required_predicates_to_be_wf<'tcx>( } } -/// We also have to check the explicit predicates -/// declared on the type. +/// Check the explicit predicates declared on the type. +/// +/// ### Example +/// /// ```ignore (illustrative) -/// struct Foo<'a, T> { -/// field1: Bar +/// struct Outer<'a, T> { +/// field: Inner, /// } /// -/// struct Bar where U: 'static, U: Foo { -/// ... +/// struct Inner where U: 'static, U: Outer { +/// // ... /// } /// ``` /// Here, we should fetch the explicit predicates, which -/// will give us `U: 'static` and `U: Foo`. The latter we +/// will give us `U: 'static` and `U: Outer`. The latter we /// can ignore, but we will want to process `U: 'static`, /// applying the substitution as above. fn check_explicit_predicates<'tcx>( @@ -303,3 +298,45 @@ fn check_explicit_predicates<'tcx>( insert_outlives_predicate(tcx, predicate.0, predicate.1, span, required_predicates); } } + +/// Check the inferred predicates declared on the type. +/// +/// ### Example +/// +/// ```ignore (illustrative) +/// struct Outer<'a, T> { +/// outer: Inner<'a, T>, +/// } +/// +/// struct Inner<'b, U> { +/// inner: &'b U, +/// } +/// ``` +/// +/// Here, when processing the type of field `outer`, we would request the +/// set of implicit predicates computed for `Inner` thus far. This will +/// initially come back empty, but in next round we will get `U: 'b`. +/// We then apply the substitution `['b => 'a, U => T]` and thus get the +/// requirement that `T: 'a` holds for `Outer`. +fn check_inferred_predicates<'tcx>( + tcx: TyCtxt<'tcx>, + def_id: DefId, + args: ty::GenericArgsRef<'tcx>, + global_inferred_outlives: &FxHashMap>>, + required_predicates: &mut RequiredPredicates<'tcx>, +) { + // Load the current set of inferred and explicit predicates from `global_inferred_outlives` + // and filter the ones that are `TypeOutlives`. + + let Some(predicates) = global_inferred_outlives.get(&def_id) else { + return; + }; + + for (&predicate, &span) in predicates.as_ref().skip_binder() { + // `predicate` is `U: 'b` in the example above. + // So apply the substitution to get `T: 'a`. + let ty::OutlivesPredicate(arg, region) = + predicates.rebind(predicate).instantiate(tcx, args); + insert_outlives_predicate(tcx, arg, region, span, required_predicates); + } +} diff --git a/compiler/rustc_hir_analysis/src/outlives/mod.rs b/compiler/rustc_hir_analysis/src/outlives/mod.rs index 72511bfa01f14..a87112dcc1230 100644 --- a/compiler/rustc_hir_analysis/src/outlives/mod.rs +++ b/compiler/rustc_hir_analysis/src/outlives/mod.rs @@ -21,6 +21,10 @@ fn inferred_outlives_of(tcx: TyCtxt<'_>, item_def_id: LocalDefId) -> &[(ty::Clau let crate_map = tcx.inferred_outlives_crate(()); crate_map.predicates.get(&item_def_id.to_def_id()).copied().unwrap_or(&[]) } + DefKind::TyAlias if tcx.type_alias_is_lazy(item_def_id) => { + let crate_map = tcx.inferred_outlives_crate(()); + crate_map.predicates.get(&item_def_id.to_def_id()).copied().unwrap_or(&[]) + } DefKind::AnonConst if tcx.features().generic_const_exprs => { let id = tcx.local_def_id_to_hir_id(item_def_id); if tcx.hir().opt_const_param_default_param_def_id(id).is_some() { @@ -47,8 +51,8 @@ fn inferred_outlives_of(tcx: TyCtxt<'_>, item_def_id: LocalDefId) -> &[(ty::Clau } fn inferred_outlives_crate(tcx: TyCtxt<'_>, (): ()) -> CratePredicatesMap<'_> { - // Compute a map from each struct/enum/union S to the **explicit** - // outlives predicates (`T: 'a`, `'a: 'b`) that the user wrote. + // Compute a map from each ADT (struct/enum/union) and lazy type alias to + // the **explicit** outlives predicates (`T: 'a`, `'a: 'b`) that the user wrote. // Typically there won't be many of these, except in older code where // they were mandatory. Nonetheless, we have to ensure that every such // predicate is satisfied, so they form a kind of base set of requirements diff --git a/tests/ui/associated-type-bounds/duplicate.stderr b/tests/ui/associated-type-bounds/duplicate.stderr index 3888e62230f14..38b812dfdd442 100644 --- a/tests/ui/associated-type-bounds/duplicate.stderr +++ b/tests/ui/associated-type-bounds/duplicate.stderr @@ -6,6 +6,30 @@ LL | struct SI1> { | | | `Item` bound here first +error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified + --> $DIR/duplicate.rs:255:40 + | +LL | type TADyn1 = dyn Iterator; + | ---------- ^^^^^^^^^^ re-bound here + | | + | `Item` bound here first + +error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified + --> $DIR/duplicate.rs:257:44 + | +LL | type TADyn2 = Box>; + | ---------- ^^^^^^^^^^ re-bound here + | | + | `Item` bound here first + +error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified + --> $DIR/duplicate.rs:259:43 + | +LL | type TADyn3 = dyn Iterator; + | ------------- ^^^^^^^^^^^^^ re-bound here + | | + | `Item` bound here first + error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified --> $DIR/duplicate.rs:11:36 | @@ -490,30 +514,6 @@ LL | Self: Iterator, | = note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no` -error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified - --> $DIR/duplicate.rs:255:40 - | -LL | type TADyn1 = dyn Iterator; - | ---------- ^^^^^^^^^^ re-bound here - | | - | `Item` bound here first - -error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified - --> $DIR/duplicate.rs:257:44 - | -LL | type TADyn2 = Box>; - | ---------- ^^^^^^^^^^ re-bound here - | | - | `Item` bound here first - -error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified - --> $DIR/duplicate.rs:259:43 - | -LL | type TADyn3 = dyn Iterator; - | ------------- ^^^^^^^^^^^^^ re-bound here - | | - | `Item` bound here first - error[E0719]: the value of the associated type `Item` in trait `Iterator` is already specified --> $DIR/duplicate.rs:243:34 | diff --git a/tests/ui/lazy-type-alias/implied-outlives-bounds.neg.stderr b/tests/ui/lazy-type-alias/implied-outlives-bounds.neg.stderr new file mode 100644 index 0000000000000..a2dbb58ecd03d --- /dev/null +++ b/tests/ui/lazy-type-alias/implied-outlives-bounds.neg.stderr @@ -0,0 +1,34 @@ +error: lifetime may not live long enough + --> $DIR/implied-outlives-bounds.rs:21:12 + | +LL | fn env0<'any>() { + | ---- lifetime `'any` defined here +LL | let _: TypeOutlives<'static, &'any ()>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'any` must outlive `'static` + +error: lifetime may not live long enough + --> $DIR/implied-outlives-bounds.rs:26:12 + | +LL | fn env1<'any>() { + | ---- lifetime `'any` defined here +LL | let _: RegionOutlives<'static, 'any>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'any` must outlive `'static` + +error: lifetime may not live long enough + --> $DIR/implied-outlives-bounds.rs:31:12 + | +LL | fn env2<'any>() { + | ---- lifetime `'any` defined here +LL | let _: Outer0<'static, &'any ()>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'any` must outlive `'static` + +error: lifetime may not live long enough + --> $DIR/implied-outlives-bounds.rs:36:12 + | +LL | fn env3<'any>() { + | ---- lifetime `'any` defined here +LL | let _: Outer1<'static, &'any ()>; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'any` must outlive `'static` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/lazy-type-alias/implied-outlives-bounds.rs b/tests/ui/lazy-type-alias/implied-outlives-bounds.rs new file mode 100644 index 0000000000000..c08e45975de10 --- /dev/null +++ b/tests/ui/lazy-type-alias/implied-outlives-bounds.rs @@ -0,0 +1,39 @@ +// Check that we imply outlives-bounds on lazy type aliases. + +// revisions: pos neg +//[pos] check-pass + +#![feature(lazy_type_alias)] +#![allow(incomplete_features)] + +type TypeOutlives<'a, T> = &'a T; +type RegionOutlives<'a, 'b> = &'a &'b (); + +// Ensure that we imply bounds from the explicit bounds of weak aliases. +struct Outer0<'a, T>(ExplicitTypeOutlives<'a, T>); +type ExplicitTypeOutlives<'a, T: 'a> = (&'a (), T); + +// Ensure that we imply bounds from the implied bounds of weak aliases. +type Outer1<'b, U> = TypeOutlives<'b, U>; + +#[cfg(neg)] +fn env0<'any>() { + let _: TypeOutlives<'static, &'any ()>; //[neg]~ ERROR lifetime may not live long enough +} + +#[cfg(neg)] +fn env1<'any>() { + let _: RegionOutlives<'static, 'any>; //[neg]~ ERROR lifetime may not live long enough +} + +#[cfg(neg)] +fn env2<'any>() { + let _: Outer0<'static, &'any ()>; //[neg]~ ERROR lifetime may not live long enough +} + +#[cfg(neg)] +fn env3<'any>() { + let _: Outer1<'static, &'any ()>; //[neg]~ ERROR lifetime may not live long enough +} + +fn main() {} From 1d48f69d65af74201314304623f37f5bcefa9a24 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 4 Jan 2024 01:46:43 +0000 Subject: [PATCH 07/14] Check yield terminator's resume type in borrowck --- .../src/type_check/input_output.rs | 33 +++++++---------- .../src/type_check/liveness/mod.rs | 1 + compiler/rustc_borrowck/src/type_check/mod.rs | 26 ++++++++++++-- .../rustc_borrowck/src/universal_regions.rs | 12 +++++-- compiler/rustc_middle/src/mir/mod.rs | 9 +++++ compiler/rustc_middle/src/mir/visit.rs | 8 +++++ compiler/rustc_mir_build/src/build/mod.rs | 27 ++++++++------ compiler/rustc_mir_transform/src/coroutine.rs | 1 + .../coroutine/check-resume-ty-lifetimes-2.rs | 35 ++++++++++++++++++ .../check-resume-ty-lifetimes-2.stderr | 36 +++++++++++++++++++ .../ui/coroutine/check-resume-ty-lifetimes.rs | 27 ++++++++++++++ .../check-resume-ty-lifetimes.stderr | 11 ++++++ 12 files changed, 190 insertions(+), 36 deletions(-) create mode 100644 tests/ui/coroutine/check-resume-ty-lifetimes-2.rs create mode 100644 tests/ui/coroutine/check-resume-ty-lifetimes-2.stderr create mode 100644 tests/ui/coroutine/check-resume-ty-lifetimes.rs create mode 100644 tests/ui/coroutine/check-resume-ty-lifetimes.stderr diff --git a/compiler/rustc_borrowck/src/type_check/input_output.rs b/compiler/rustc_borrowck/src/type_check/input_output.rs index 5bd7cc9514ca2..61b6bef3b87b9 100644 --- a/compiler/rustc_borrowck/src/type_check/input_output.rs +++ b/compiler/rustc_borrowck/src/type_check/input_output.rs @@ -94,31 +94,22 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { ); } - debug!( - "equate_inputs_and_outputs: body.yield_ty {:?}, universal_regions.yield_ty {:?}", - body.yield_ty(), - universal_regions.yield_ty - ); - - // We will not have a universal_regions.yield_ty if we yield (by accident) - // outside of a coroutine and return an `impl Trait`, so emit a span_delayed_bug - // because we don't want to panic in an assert here if we've already got errors. - if body.yield_ty().is_some() != universal_regions.yield_ty.is_some() { - self.tcx().dcx().span_delayed_bug( - body.span, - format!( - "Expected body to have yield_ty ({:?}) iff we have a UR yield_ty ({:?})", - body.yield_ty(), - universal_regions.yield_ty, - ), + if let Some(mir_yield_ty) = body.yield_ty() { + let yield_span = body.local_decls[RETURN_PLACE].source_info.span; + self.equate_normalized_input_or_output( + universal_regions.yield_ty.unwrap(), + mir_yield_ty, + yield_span, ); } - if let (Some(mir_yield_ty), Some(ur_yield_ty)) = - (body.yield_ty(), universal_regions.yield_ty) - { + if let Some(mir_resume_ty) = body.resume_ty() { let yield_span = body.local_decls[RETURN_PLACE].source_info.span; - self.equate_normalized_input_or_output(ur_yield_ty, mir_yield_ty, yield_span); + self.equate_normalized_input_or_output( + universal_regions.resume_ty.unwrap(), + mir_resume_ty, + yield_span, + ); } // Return types are a bit more complex. They may contain opaque `impl Trait` types. diff --git a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs index dc4695fd2b058..e137bc1be0aeb 100644 --- a/compiler/rustc_borrowck/src/type_check/liveness/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/liveness/mod.rs @@ -183,6 +183,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for LiveVariablesVisitor<'cx, 'tcx> { match ty_context { TyContext::ReturnTy(SourceInfo { span, .. }) | TyContext::YieldTy(SourceInfo { span, .. }) + | TyContext::ResumeTy(SourceInfo { span, .. }) | TyContext::UserTy(span) | TyContext::LocalDecl { source_info: SourceInfo { span, .. }, .. } => { span_bug!(span, "should not be visiting outside of the CFG: {:?}", ty_context); diff --git a/compiler/rustc_borrowck/src/type_check/mod.rs b/compiler/rustc_borrowck/src/type_check/mod.rs index 80575e30a8d23..9c0f53ddb86fa 100644 --- a/compiler/rustc_borrowck/src/type_check/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/mod.rs @@ -1450,13 +1450,13 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { } } } - TerminatorKind::Yield { value, .. } => { + TerminatorKind::Yield { value, resume_arg, .. } => { self.check_operand(value, term_location); - let value_ty = value.ty(body, tcx); match body.yield_ty() { None => span_mirbug!(self, term, "yield in non-coroutine"), Some(ty) => { + let value_ty = value.ty(body, tcx); if let Err(terr) = self.sub_types( value_ty, ty, @@ -1474,6 +1474,28 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { } } } + + match body.resume_ty() { + None => span_mirbug!(self, term, "yield in non-coroutine"), + Some(ty) => { + let resume_ty = resume_arg.ty(body, tcx); + if let Err(terr) = self.sub_types( + ty, + resume_ty.ty, + term_location.to_locations(), + ConstraintCategory::Yield, + ) { + span_mirbug!( + self, + term, + "type of resume place is {:?}, but the resume type is {:?}: {:?}", + resume_ty, + ty, + terr + ); + } + } + } } } } diff --git a/compiler/rustc_borrowck/src/universal_regions.rs b/compiler/rustc_borrowck/src/universal_regions.rs index a02304a2f8b30..addb41ff5fc8f 100644 --- a/compiler/rustc_borrowck/src/universal_regions.rs +++ b/compiler/rustc_borrowck/src/universal_regions.rs @@ -76,6 +76,8 @@ pub struct UniversalRegions<'tcx> { pub unnormalized_input_tys: &'tcx [Ty<'tcx>], pub yield_ty: Option>, + + pub resume_ty: Option>, } /// The "defining type" for this MIR. The key feature of the "defining @@ -525,9 +527,12 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> { debug!("build: extern regions = {}..{}", first_extern_index, first_local_index); debug!("build: local regions = {}..{}", first_local_index, num_universals); - let yield_ty = match defining_ty { - DefiningTy::Coroutine(_, args) => Some(args.as_coroutine().yield_ty()), - _ => None, + let (resume_ty, yield_ty) = match defining_ty { + DefiningTy::Coroutine(_, args) => { + let tys = args.as_coroutine(); + (Some(tys.resume_ty()), Some(tys.yield_ty())) + } + _ => (None, None), }; UniversalRegions { @@ -541,6 +546,7 @@ impl<'cx, 'tcx> UniversalRegionsBuilder<'cx, 'tcx> { unnormalized_output_ty: *unnormalized_output_ty, unnormalized_input_tys, yield_ty, + resume_ty, } } diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 5c425fef27ebc..01ad3aefffa3b 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -250,6 +250,9 @@ pub struct CoroutineInfo<'tcx> { /// The yield type of the function, if it is a coroutine. pub yield_ty: Option>, + /// The resume type of the function, if it is a coroutine. + pub resume_ty: Option>, + /// Coroutine drop glue. pub coroutine_drop: Option>, @@ -385,6 +388,7 @@ impl<'tcx> Body<'tcx> { coroutine: coroutine_kind.map(|coroutine_kind| { Box::new(CoroutineInfo { yield_ty: None, + resume_ty: None, coroutine_drop: None, coroutine_layout: None, coroutine_kind, @@ -551,6 +555,11 @@ impl<'tcx> Body<'tcx> { self.coroutine.as_ref().and_then(|coroutine| coroutine.yield_ty) } + #[inline] + pub fn resume_ty(&self) -> Option> { + self.coroutine.as_ref().and_then(|coroutine| coroutine.resume_ty) + } + #[inline] pub fn coroutine_layout(&self) -> Option<&CoroutineLayout<'tcx>> { self.coroutine.as_ref().and_then(|coroutine| coroutine.coroutine_layout.as_ref()) diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs index 132ecf91af187..2ccf5a9f6f7ad 100644 --- a/compiler/rustc_middle/src/mir/visit.rs +++ b/compiler/rustc_middle/src/mir/visit.rs @@ -996,6 +996,12 @@ macro_rules! super_body { TyContext::YieldTy(SourceInfo::outermost(span)) ); } + if let Some(resume_ty) = $(& $mutability)? gen.resume_ty { + $self.visit_ty( + resume_ty, + TyContext::ResumeTy(SourceInfo::outermost(span)) + ); + } } for (bb, data) in basic_blocks_iter!($body, $($mutability, $invalidate)?) { @@ -1244,6 +1250,8 @@ pub enum TyContext { YieldTy(SourceInfo), + ResumeTy(SourceInfo), + /// A type found at some location. Location(Location), } diff --git a/compiler/rustc_mir_build/src/build/mod.rs b/compiler/rustc_mir_build/src/build/mod.rs index e0199fb876717..c4cade839478c 100644 --- a/compiler/rustc_mir_build/src/build/mod.rs +++ b/compiler/rustc_mir_build/src/build/mod.rs @@ -488,7 +488,7 @@ fn construct_fn<'tcx>( let arguments = &thir.params; - let (yield_ty, return_ty) = if coroutine_kind.is_some() { + let (resume_ty, yield_ty, return_ty) = if coroutine_kind.is_some() { let coroutine_ty = arguments[thir::UPVAR_ENV_PARAM].ty; let coroutine_sig = match coroutine_ty.kind() { ty::Coroutine(_, gen_args, ..) => gen_args.as_coroutine().sig(), @@ -496,9 +496,9 @@ fn construct_fn<'tcx>( span_bug!(span, "coroutine w/o coroutine type: {:?}", coroutine_ty) } }; - (Some(coroutine_sig.yield_ty), coroutine_sig.return_ty) + (Some(coroutine_sig.resume_ty), Some(coroutine_sig.yield_ty), coroutine_sig.return_ty) } else { - (None, fn_sig.output()) + (None, None, fn_sig.output()) }; if let Some(custom_mir_attr) = @@ -562,9 +562,12 @@ fn construct_fn<'tcx>( } else { None }; - if yield_ty.is_some() { + + if coroutine_kind.is_some() { body.coroutine.as_mut().unwrap().yield_ty = yield_ty; + body.coroutine.as_mut().unwrap().resume_ty = resume_ty; } + body } @@ -631,18 +634,18 @@ fn construct_error(tcx: TyCtxt<'_>, def_id: LocalDefId, guar: ErrorGuaranteed) - let hir_id = tcx.local_def_id_to_hir_id(def_id); let coroutine_kind = tcx.coroutine_kind(def_id); - let (inputs, output, yield_ty) = match tcx.def_kind(def_id) { + let (inputs, output, resume_ty, yield_ty) = match tcx.def_kind(def_id) { DefKind::Const | DefKind::AssocConst | DefKind::AnonConst | DefKind::InlineConst - | DefKind::Static(_) => (vec![], tcx.type_of(def_id).instantiate_identity(), None), + | DefKind::Static(_) => (vec![], tcx.type_of(def_id).instantiate_identity(), None, None), DefKind::Ctor(..) | DefKind::Fn | DefKind::AssocFn => { let sig = tcx.liberate_late_bound_regions( def_id.to_def_id(), tcx.fn_sig(def_id).instantiate_identity(), ); - (sig.inputs().to_vec(), sig.output(), None) + (sig.inputs().to_vec(), sig.output(), None, None) } DefKind::Closure if coroutine_kind.is_some() => { let coroutine_ty = tcx.type_of(def_id).instantiate_identity(); @@ -650,9 +653,10 @@ fn construct_error(tcx: TyCtxt<'_>, def_id: LocalDefId, guar: ErrorGuaranteed) - bug!("expected type of coroutine-like closure to be a coroutine") }; let args = args.as_coroutine(); + let resume_ty = args.resume_ty(); let yield_ty = args.yield_ty(); let return_ty = args.return_ty(); - (vec![coroutine_ty, args.resume_ty()], return_ty, Some(yield_ty)) + (vec![coroutine_ty, args.resume_ty()], return_ty, Some(resume_ty), Some(yield_ty)) } DefKind::Closure => { let closure_ty = tcx.type_of(def_id).instantiate_identity(); @@ -666,7 +670,7 @@ fn construct_error(tcx: TyCtxt<'_>, def_id: LocalDefId, guar: ErrorGuaranteed) - ty::ClosureKind::FnMut => Ty::new_mut_ref(tcx, tcx.lifetimes.re_erased, closure_ty), ty::ClosureKind::FnOnce => closure_ty, }; - ([self_ty].into_iter().chain(sig.inputs().to_vec()).collect(), sig.output(), None) + ([self_ty].into_iter().chain(sig.inputs().to_vec()).collect(), sig.output(), None, None) } dk => bug!("{:?} is not a body: {:?}", def_id, dk), }; @@ -705,7 +709,10 @@ fn construct_error(tcx: TyCtxt<'_>, def_id: LocalDefId, guar: ErrorGuaranteed) - Some(guar), ); - body.coroutine.as_mut().map(|gen| gen.yield_ty = yield_ty); + body.coroutine.as_mut().map(|gen| { + gen.yield_ty = yield_ty; + gen.resume_ty = resume_ty; + }); body } diff --git a/compiler/rustc_mir_transform/src/coroutine.rs b/compiler/rustc_mir_transform/src/coroutine.rs index ce1a36cf67021..33e305497b505 100644 --- a/compiler/rustc_mir_transform/src/coroutine.rs +++ b/compiler/rustc_mir_transform/src/coroutine.rs @@ -1733,6 +1733,7 @@ impl<'tcx> MirPass<'tcx> for StateTransform { } body.coroutine.as_mut().unwrap().yield_ty = None; + body.coroutine.as_mut().unwrap().resume_ty = None; body.coroutine.as_mut().unwrap().coroutine_layout = Some(layout); // Insert `drop(coroutine_struct)` which is used to drop upvars for coroutines in diff --git a/tests/ui/coroutine/check-resume-ty-lifetimes-2.rs b/tests/ui/coroutine/check-resume-ty-lifetimes-2.rs new file mode 100644 index 0000000000000..a316c50e86732 --- /dev/null +++ b/tests/ui/coroutine/check-resume-ty-lifetimes-2.rs @@ -0,0 +1,35 @@ +#![feature(coroutine_trait)] +#![feature(coroutines)] + +use std::ops::Coroutine; + +struct Contravariant<'a>(fn(&'a ())); +struct Covariant<'a>(fn() -> &'a ()); + +fn bad1<'short, 'long: 'short>() -> impl Coroutine> { + |_: Covariant<'short>| { + let a: Covariant<'long> = yield (); + //~^ ERROR lifetime may not live long enough + } +} + +fn bad2<'short, 'long: 'short>() -> impl Coroutine> { + |_: Contravariant<'long>| { + let a: Contravariant<'short> = yield (); + //~^ ERROR lifetime may not live long enough + } +} + +fn good1<'short, 'long: 'short>() -> impl Coroutine> { + |_: Covariant<'long>| { + let a: Covariant<'short> = yield (); + } +} + +fn good2<'short, 'long: 'short>() -> impl Coroutine> { + |_: Contravariant<'short>| { + let a: Contravariant<'long> = yield (); + } +} + +fn main() {} diff --git a/tests/ui/coroutine/check-resume-ty-lifetimes-2.stderr b/tests/ui/coroutine/check-resume-ty-lifetimes-2.stderr new file mode 100644 index 0000000000000..e0cbca2dd5267 --- /dev/null +++ b/tests/ui/coroutine/check-resume-ty-lifetimes-2.stderr @@ -0,0 +1,36 @@ +error: lifetime may not live long enough + --> $DIR/check-resume-ty-lifetimes-2.rs:11:16 + | +LL | fn bad1<'short, 'long: 'short>() -> impl Coroutine> { + | ------ ----- lifetime `'long` defined here + | | + | lifetime `'short` defined here +LL | |_: Covariant<'short>| { +LL | let a: Covariant<'long> = yield (); + | ^^^^^^^^^^^^^^^^ type annotation requires that `'short` must outlive `'long` + | + = help: consider adding the following bound: `'short: 'long` +help: consider adding 'move' keyword before the nested closure + | +LL | move |_: Covariant<'short>| { + | ++++ + +error: lifetime may not live long enough + --> $DIR/check-resume-ty-lifetimes-2.rs:18:40 + | +LL | fn bad2<'short, 'long: 'short>() -> impl Coroutine> { + | ------ ----- lifetime `'long` defined here + | | + | lifetime `'short` defined here +LL | |_: Contravariant<'long>| { +LL | let a: Contravariant<'short> = yield (); + | ^^^^^^^^ yielding this value requires that `'short` must outlive `'long` + | + = help: consider adding the following bound: `'short: 'long` +help: consider adding 'move' keyword before the nested closure + | +LL | move |_: Contravariant<'long>| { + | ++++ + +error: aborting due to 2 previous errors + diff --git a/tests/ui/coroutine/check-resume-ty-lifetimes.rs b/tests/ui/coroutine/check-resume-ty-lifetimes.rs new file mode 100644 index 0000000000000..add0b5080a8a8 --- /dev/null +++ b/tests/ui/coroutine/check-resume-ty-lifetimes.rs @@ -0,0 +1,27 @@ +#![feature(coroutine_trait)] +#![feature(coroutines)] +#![allow(unused)] + +use std::ops::Coroutine; +use std::ops::CoroutineState; +use std::pin::pin; + +fn mk_static(s: &str) -> &'static str { + let mut storage: Option<&'static str> = None; + + let mut coroutine = pin!(|_: &str| { + let x: &'static str = yield (); + //~^ ERROR lifetime may not live long enough + storage = Some(x); + }); + + coroutine.as_mut().resume(s); + coroutine.as_mut().resume(s); + + storage.unwrap() +} + +fn main() { + let s = mk_static(&String::from("hello, world")); + println!("{s}"); +} diff --git a/tests/ui/coroutine/check-resume-ty-lifetimes.stderr b/tests/ui/coroutine/check-resume-ty-lifetimes.stderr new file mode 100644 index 0000000000000..f373aa778a82c --- /dev/null +++ b/tests/ui/coroutine/check-resume-ty-lifetimes.stderr @@ -0,0 +1,11 @@ +error: lifetime may not live long enough + --> $DIR/check-resume-ty-lifetimes.rs:13:16 + | +LL | fn mk_static(s: &str) -> &'static str { + | - let's call the lifetime of this reference `'1` +... +LL | let x: &'static str = yield (); + | ^^^^^^^^^^^^ type annotation requires that `'1` must outlive `'static` + +error: aborting due to 1 previous error + From 4bc3552cd871afe32cb45f7c9685492af430ed77 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Thu, 4 Jan 2024 21:09:13 +0300 Subject: [PATCH 08/14] cstore: Remove unnecessary locking from `CrateMetadata` --- compiler/rustc_metadata/src/creader.rs | 41 ++++++++++--------- compiler/rustc_metadata/src/rmeta/decoder.rs | 39 +++++++++--------- .../src/rmeta/decoder/cstore_impl.rs | 36 ++++++++-------- 3 files changed, 58 insertions(+), 58 deletions(-) diff --git a/compiler/rustc_metadata/src/creader.rs b/compiler/rustc_metadata/src/creader.rs index a3da8c14f63a7..bb02a8a1e4743 100644 --- a/compiler/rustc_metadata/src/creader.rs +++ b/compiler/rustc_metadata/src/creader.rs @@ -196,6 +196,10 @@ impl CStore { CrateMetadataRef { cdata, cstore: self } } + pub(crate) fn get_crate_data_mut(&mut self, cnum: CrateNum) -> &mut CrateMetadata { + self.metas[cnum].as_mut().unwrap_or_else(|| panic!("Failed to get crate data for {cnum:?}")) + } + fn set_crate_data(&mut self, cnum: CrateNum, data: CrateMetadata) { assert!(self.metas[cnum].is_none(), "Overwriting crate metadata entry"); self.metas[cnum] = Some(Box::new(data)); @@ -207,6 +211,12 @@ impl CStore { .filter_map(|(cnum, data)| data.as_deref().map(|data| (cnum, data))) } + fn iter_crate_data_mut(&mut self) -> impl Iterator { + self.metas + .iter_enumerated_mut() + .filter_map(|(cnum, data)| data.as_deref_mut().map(|data| (cnum, data))) + } + fn push_dependencies_in_postorder(&self, deps: &mut Vec, cnum: CrateNum) { if !deps.contains(&cnum) { let data = self.get_crate_data(cnum); @@ -586,11 +596,11 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { match result { (LoadResult::Previous(cnum), None) => { - let data = self.cstore.get_crate_data(cnum); + let data = self.cstore.get_crate_data_mut(cnum); if data.is_proc_macro_crate() { dep_kind = CrateDepKind::MacrosOnly; } - data.update_dep_kind(|data_dep_kind| cmp::max(data_dep_kind, dep_kind)); + data.set_dep_kind(cmp::max(data.dep_kind(), dep_kind)); if let Some(private_dep) = private_dep { data.update_and_private_dep(private_dep); } @@ -637,17 +647,6 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { })) } - fn update_extern_crate(&self, cnum: CrateNum, extern_crate: ExternCrate) { - let cmeta = self.cstore.get_crate_data(cnum); - if cmeta.update_extern_crate(extern_crate) { - // Propagate the extern crate info to dependencies if it was updated. - let extern_crate = ExternCrate { dependency_of: cnum, ..extern_crate }; - for dep_cnum in cmeta.dependencies() { - self.update_extern_crate(dep_cnum, extern_crate); - } - } - } - // Go through the crate metadata and load any crates that it references fn resolve_crate_deps( &mut self, @@ -726,17 +725,19 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { let mut runtime_found = false; let mut needs_panic_runtime = attr::contains_name(&krate.attrs, sym::needs_panic_runtime); + let mut panic_runtimes = Vec::new(); for (cnum, data) in self.cstore.iter_crate_data() { needs_panic_runtime = needs_panic_runtime || data.needs_panic_runtime(); if data.is_panic_runtime() { // Inject a dependency from all #![needs_panic_runtime] to this // #![panic_runtime] crate. - self.inject_dependency_if(cnum, "a panic runtime", &|data| { - data.needs_panic_runtime() - }); + panic_runtimes.push(cnum); runtime_found = runtime_found || data.dep_kind() == CrateDepKind::Explicit; } } + for cnum in panic_runtimes { + self.inject_dependency_if(cnum, "a panic runtime", &|data| data.needs_panic_runtime()); + } // If an explicitly linked and matching panic runtime was found, or if // we just don't need one at all, then we're done here and there's @@ -917,7 +918,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { } fn inject_dependency_if( - &self, + &mut self, krate: CrateNum, what: &str, needs_dep: &dyn Fn(&CrateMetadata) -> bool, @@ -947,7 +948,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { // crate provided for this compile, but in order for this compilation to // be successfully linked we need to inject a dependency (to order the // crates on the command line correctly). - for (cnum, data) in self.cstore.iter_crate_data() { + for (cnum, data) in self.cstore.iter_crate_data_mut() { if needs_dep(data) { info!("injecting a dep from {} to {}", cnum, krate); data.add_dependency(krate); @@ -1031,7 +1032,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { let cnum = self.resolve_crate(name, item.span, dep_kind)?; let path_len = definitions.def_path(def_id).data.len(); - self.update_extern_crate( + self.cstore.update_extern_crate( cnum, ExternCrate { src: ExternCrateSource::Extern(def_id.to_def_id()), @@ -1049,7 +1050,7 @@ impl<'a, 'tcx> CrateLoader<'a, 'tcx> { pub fn process_path_extern(&mut self, name: Symbol, span: Span) -> Option { let cnum = self.resolve_crate(name, span, CrateDepKind::Explicit)?; - self.update_extern_crate( + self.cstore.update_extern_crate( cnum, ExternCrate { src: ExternCrateSource::Path, diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 2de29db9e5c84..d13a1664adea4 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -8,7 +8,7 @@ use rustc_ast as ast; use rustc_data_structures::captures::Captures; use rustc_data_structures::fingerprint::Fingerprint; use rustc_data_structures::owned_slice::OwnedSlice; -use rustc_data_structures::sync::{AppendOnlyVec, AtomicBool, Lock, Lrc, OnceLock}; +use rustc_data_structures::sync::{Lock, Lrc, OnceLock}; use rustc_data_structures::unhash::UnhashMap; use rustc_expand::base::{SyntaxExtension, SyntaxExtensionKind}; use rustc_expand::proc_macro::{AttrProcMacro, BangProcMacro, DeriveProcMacro}; @@ -31,7 +31,6 @@ use rustc_span::{BytePos, Pos, SpanData, SyntaxContext, DUMMY_SP}; use proc_macro::bridge::client::ProcMacro; use std::iter::TrustedLen; use std::path::Path; -use std::sync::atomic::Ordering; use std::{io, iter, mem}; pub(super) use cstore_impl::provide; @@ -96,15 +95,15 @@ pub(crate) struct CrateMetadata { /// IDs as they are seen from the current compilation session. cnum_map: CrateNumMap, /// Same ID set as `cnum_map` plus maybe some injected crates like panic runtime. - dependencies: AppendOnlyVec, + dependencies: Vec, /// How to link (or not link) this crate to the currently compiled crate. - dep_kind: Lock, + dep_kind: CrateDepKind, /// Filesystem location of this crate. source: Lrc, /// Whether or not this crate should be consider a private dependency. /// Used by the 'exported_private_dependencies' lint, and for determining /// whether to emit suggestions that reference this crate. - private_dep: AtomicBool, + private_dep: bool, /// The hash for the host proc macro. Used to support `-Z dual-proc-macro`. host_hash: Option, @@ -118,7 +117,7 @@ pub(crate) struct CrateMetadata { // --- Data used only for improving diagnostics --- /// Information about the `extern crate` item or path that caused this crate to be loaded. /// If this is `None`, then the crate was injected (e.g., by the allocator). - extern_crate: Lock>, + extern_crate: Option, } /// Holds information about a rustc_span::SourceFile imported from another crate. @@ -1818,11 +1817,11 @@ impl CrateMetadata { cnum, cnum_map, dependencies, - dep_kind: Lock::new(dep_kind), + dep_kind, source: Lrc::new(source), - private_dep: AtomicBool::new(private_dep), + private_dep, host_hash, - extern_crate: Lock::new(None), + extern_crate: None, hygiene_context: Default::default(), def_key_cache: Default::default(), }; @@ -1839,18 +1838,18 @@ impl CrateMetadata { } pub(crate) fn dependencies(&self) -> impl Iterator + '_ { - self.dependencies.iter() + self.dependencies.iter().copied() } - pub(crate) fn add_dependency(&self, cnum: CrateNum) { + pub(crate) fn add_dependency(&mut self, cnum: CrateNum) { self.dependencies.push(cnum); } - pub(crate) fn update_extern_crate(&self, new_extern_crate: ExternCrate) -> bool { - let mut extern_crate = self.extern_crate.borrow_mut(); - let update = Some(new_extern_crate.rank()) > extern_crate.as_ref().map(ExternCrate::rank); + pub(crate) fn update_extern_crate(&mut self, new_extern_crate: ExternCrate) -> bool { + let update = + Some(new_extern_crate.rank()) > self.extern_crate.as_ref().map(ExternCrate::rank); if update { - *extern_crate = Some(new_extern_crate); + self.extern_crate = Some(new_extern_crate); } update } @@ -1860,15 +1859,15 @@ impl CrateMetadata { } pub(crate) fn dep_kind(&self) -> CrateDepKind { - *self.dep_kind.lock() + self.dep_kind } - pub(crate) fn update_dep_kind(&self, f: impl FnOnce(CrateDepKind) -> CrateDepKind) { - self.dep_kind.with_lock(|dep_kind| *dep_kind = f(*dep_kind)) + pub(crate) fn set_dep_kind(&mut self, dep_kind: CrateDepKind) { + self.dep_kind = dep_kind; } - pub(crate) fn update_and_private_dep(&self, private_dep: bool) { - self.private_dep.fetch_and(private_dep, Ordering::SeqCst); + pub(crate) fn update_and_private_dep(&mut self, private_dep: bool) { + self.private_dep &= private_dep; } pub(crate) fn required_panic_strategy(&self) -> Option { diff --git a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs index bb8f4af8e97ae..912c2f36eb3c8 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs @@ -19,7 +19,7 @@ use rustc_middle::query::LocalCrate; use rustc_middle::ty::fast_reject::SimplifiedType; use rustc_middle::ty::{self, TyCtxt}; use rustc_middle::util::Providers; -use rustc_session::cstore::CrateStore; +use rustc_session::cstore::{CrateStore, ExternCrate}; use rustc_session::{Session, StableCrateId}; use rustc_span::hygiene::{ExpnHash, ExpnId}; use rustc_span::symbol::{kw, Symbol}; @@ -290,13 +290,7 @@ provide! { tcx, def_id, other, cdata, cross_crate_inlinable => { cdata.cross_crate_inlinable(def_id.index) } dylib_dependency_formats => { cdata.get_dylib_dependency_formats(tcx) } - is_private_dep => { - // Parallel compiler needs to synchronize type checking and linting (which use this flag) - // so that they happen strictly crate loading. Otherwise, the full list of available - // impls aren't loaded yet. - use std::sync::atomic::Ordering; - cdata.private_dep.load(Ordering::Acquire) - } + is_private_dep => { cdata.private_dep } is_panic_runtime => { cdata.root.panic_runtime } is_compiler_builtins => { cdata.root.compiler_builtins } has_global_allocator => { cdata.root.has_global_allocator } @@ -305,10 +299,7 @@ provide! { tcx, def_id, other, cdata, is_profiler_runtime => { cdata.root.profiler_runtime } required_panic_strategy => { cdata.root.required_panic_strategy } panic_in_drop_strategy => { cdata.root.panic_in_drop_strategy } - extern_crate => { - let r = *cdata.extern_crate.lock(); - r.map(|c| &*tcx.arena.alloc(c)) - } + extern_crate => { cdata.extern_crate.map(|c| &*tcx.arena.alloc(c)) } is_no_builtins => { cdata.root.no_builtins } symbol_mangling_version => { cdata.root.symbol_mangling_version } reachable_non_generics => { @@ -339,10 +330,7 @@ provide! { tcx, def_id, other, cdata, implementations_of_trait => { cdata.get_implementations_of_trait(tcx, other) } crate_incoherent_impls => { cdata.get_incoherent_impls(tcx, other) } - dep_kind => { - let r = *cdata.dep_kind.lock(); - r - } + dep_kind => { cdata.dep_kind } module_children => { tcx.arena.alloc_from_iter(cdata.get_module_children(def_id.index, tcx.sess)) } @@ -357,8 +345,7 @@ provide! { tcx, def_id, other, cdata, missing_lang_items => { cdata.get_missing_lang_items(tcx) } missing_extern_crate_item => { - let r = matches!(*cdata.extern_crate.borrow(), Some(extern_crate) if !extern_crate.is_direct()); - r + matches!(cdata.extern_crate, Some(extern_crate) if !extern_crate.is_direct()) } used_crate_source => { Lrc::clone(&cdata.source) } @@ -581,6 +568,19 @@ impl CStore { ) -> Span { self.get_crate_data(cnum).get_proc_macro_quoted_span(id, sess) } + + pub(crate) fn update_extern_crate(&mut self, cnum: CrateNum, extern_crate: ExternCrate) { + let cmeta = self.get_crate_data_mut(cnum); + if cmeta.update_extern_crate(extern_crate) { + // Propagate the extern crate info to dependencies if it was updated. + let extern_crate = ExternCrate { dependency_of: cnum, ..extern_crate }; + let dependencies = std::mem::take(&mut cmeta.dependencies); + for &dep_cnum in &dependencies { + self.update_extern_crate(dep_cnum, extern_crate); + } + self.get_crate_data_mut(cnum).dependencies = dependencies; + } + } } impl CrateStore for CStore { From 407cb241422cd13ab01b0017fcf1ef6e49c80b1b Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 21 Sep 2023 11:26:50 +0000 Subject: [PATCH 09/14] Remove `hir::Guard` Use Expr instead. Use `ExprKind::Let` to represent if let guards. --- compiler/rustc_ast_lowering/src/expr.rs | 15 +------ .../src/diagnostics/conflict_errors.rs | 2 +- compiler/rustc_hir/src/hir.rs | 22 +--------- compiler/rustc_hir/src/intravisit.rs | 9 +--- .../rustc_hir_analysis/src/check/region.rs | 3 +- compiler/rustc_hir_pretty/src/lib.rs | 14 ++---- compiler/rustc_hir_typeck/src/_match.rs | 11 +---- .../rustc_hir_typeck/src/expr_use_visitor.rs | 8 +--- .../rustc_mir_build/src/build/expr/into.rs | 4 +- .../rustc_mir_build/src/build/matches/mod.rs | 44 +++++++++++++++++-- compiler/rustc_mir_build/src/thir/cx/expr.rs | 9 +--- compiler/rustc_passes/src/liveness.rs | 21 +++------ .../issue-88118-2.stderr | 4 +- tests/ui/lint/lint-match-arms-2.stderr | 4 +- .../deny-irrefutable-let-patterns.stderr | 4 +- .../rfcs/rfc-2294-if-let-guard/warns.stderr | 4 +- tests/ui/stats/hir-stats.stderr | 8 ++-- 17 files changed, 77 insertions(+), 109 deletions(-) diff --git a/compiler/rustc_ast_lowering/src/expr.rs b/compiler/rustc_ast_lowering/src/expr.rs index ba858d49acf6e..69704de105cbe 100644 --- a/compiler/rustc_ast_lowering/src/expr.rs +++ b/compiler/rustc_ast_lowering/src/expr.rs @@ -546,20 +546,7 @@ impl<'hir> LoweringContext<'_, 'hir> { fn lower_arm(&mut self, arm: &Arm) -> hir::Arm<'hir> { let pat = self.lower_pat(&arm.pat); - let guard = arm.guard.as_ref().map(|cond| { - if let ExprKind::Let(pat, scrutinee, span, is_recovered) = &cond.kind { - hir::Guard::IfLet(self.arena.alloc(hir::Let { - hir_id: self.next_id(), - span: self.lower_span(*span), - pat: self.lower_pat(pat), - ty: None, - init: self.lower_expr(scrutinee), - is_recovered: *is_recovered, - })) - } else { - hir::Guard::If(self.lower_expr(cond)) - } - }); + let guard = arm.guard.as_ref().map(|cond| self.lower_expr(cond)); let hir_id = self.next_id(); let span = self.lower_span(arm.span); self.lower_attrs(hir_id, &arm.attrs); diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index d824260f47c18..9ce753134fbcd 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -3590,7 +3590,7 @@ impl<'b, 'v> Visitor<'v> for ConditionVisitor<'b> { )); } else if let Some(guard) = &arm.guard { self.errors.push(( - arm.pat.span.to(guard.body().span), + arm.pat.span.to(guard.span), format!( "if this pattern and condition are matched, {} is not \ initialized", diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs index 2c34fc13919bc..e88b876534e50 100644 --- a/compiler/rustc_hir/src/hir.rs +++ b/compiler/rustc_hir/src/hir.rs @@ -1258,7 +1258,7 @@ pub struct Arm<'hir> { /// If this pattern and the optional guard matches, then `body` is evaluated. pub pat: &'hir Pat<'hir>, /// Optional guard clause. - pub guard: Option>, + pub guard: Option<&'hir Expr<'hir>>, /// The expression the arm evaluates to if this arm matches. pub body: &'hir Expr<'hir>, } @@ -1280,26 +1280,6 @@ pub struct Let<'hir> { pub is_recovered: Option, } -#[derive(Debug, Clone, Copy, HashStable_Generic)] -pub enum Guard<'hir> { - If(&'hir Expr<'hir>), - IfLet(&'hir Let<'hir>), -} - -impl<'hir> Guard<'hir> { - /// Returns the body of the guard - /// - /// In other words, returns the e in either of the following: - /// - /// - `if e` - /// - `if let x = e` - pub fn body(&self) -> &'hir Expr<'hir> { - match self { - Guard::If(e) | Guard::IfLet(Let { init: e, .. }) => e, - } - } -} - #[derive(Debug, Clone, Copy, HashStable_Generic)] pub struct ExprField<'hir> { #[stable_hasher(ignore)] diff --git a/compiler/rustc_hir/src/intravisit.rs b/compiler/rustc_hir/src/intravisit.rs index e58e4c8fe0edb..dd3633b6b4f74 100644 --- a/compiler/rustc_hir/src/intravisit.rs +++ b/compiler/rustc_hir/src/intravisit.rs @@ -619,13 +619,8 @@ pub fn walk_stmt<'v, V: Visitor<'v>>(visitor: &mut V, statement: &'v Stmt<'v>) { pub fn walk_arm<'v, V: Visitor<'v>>(visitor: &mut V, arm: &'v Arm<'v>) { visitor.visit_id(arm.hir_id); visitor.visit_pat(arm.pat); - if let Some(ref g) = arm.guard { - match g { - Guard::If(ref e) => visitor.visit_expr(e), - Guard::IfLet(ref l) => { - visitor.visit_let_expr(l); - } - } + if let Some(ref e) = arm.guard { + visitor.visit_expr(e); } visitor.visit_expr(arm.body); } diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index eab83c7a25467..c3bdf94d30f97 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -184,7 +184,8 @@ fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir visitor.enter_node_scope_with_dtor(arm.hir_id.local_id); visitor.cx.var_parent = visitor.cx.parent; - if let Some(hir::Guard::If(expr)) = arm.guard { + if let Some(expr) = arm.guard { + // Check for if?? visitor.terminating_scopes.insert(expr.hir_id.local_id); } diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs index d6eea07cfbc38..1eaec9970537d 100644 --- a/compiler/rustc_hir_pretty/src/lib.rs +++ b/compiler/rustc_hir_pretty/src/lib.rs @@ -1874,17 +1874,9 @@ impl<'a> State<'a> { self.print_pat(arm.pat); self.space(); if let Some(ref g) = arm.guard { - match *g { - hir::Guard::If(e) => { - self.word_space("if"); - self.print_expr(e); - self.space(); - } - hir::Guard::IfLet(&hir::Let { pat, ty, init, .. }) => { - self.word_nbsp("if"); - self.print_let(pat, ty, init); - } - } + self.word_space("if"); + self.print_expr(g); + self.space(); } self.word_space("=>"); diff --git a/compiler/rustc_hir_typeck/src/_match.rs b/compiler/rustc_hir_typeck/src/_match.rs index 181de37284053..cf1f232229d51 100644 --- a/compiler/rustc_hir_typeck/src/_match.rs +++ b/compiler/rustc_hir_typeck/src/_match.rs @@ -78,16 +78,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let mut other_arms = vec![]; // Used only for diagnostics. let mut prior_arm = None; for arm in arms { - if let Some(g) = &arm.guard { + if let Some(e) = &arm.guard { self.diverges.set(Diverges::Maybe); - match g { - hir::Guard::If(e) => { - self.check_expr_has_type_or_error(e, tcx.types.bool, |_| {}); - } - hir::Guard::IfLet(l) => { - self.check_expr_let(l); - } - }; + self.check_expr_has_type_or_error(e, tcx.types.bool, |_| {}); } self.diverges.set(Diverges::Maybe); diff --git a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs index e952a7ff9e8ce..ed3dd1e39dfc9 100644 --- a/compiler/rustc_hir_typeck/src/expr_use_visitor.rs +++ b/compiler/rustc_hir_typeck/src/expr_use_visitor.rs @@ -669,12 +669,8 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> { ); self.walk_pat(discr_place, arm.pat, arm.guard.is_some()); - match arm.guard { - Some(hir::Guard::If(e)) => self.consume_expr(e), - Some(hir::Guard::IfLet(l)) => { - self.walk_local(l.init, l.pat, None, |t| t.borrow_expr(l.init, ty::ImmBorrow)) - } - None => {} + if let Some(ref e) = arm.guard { + self.consume_expr(e) } self.consume_expr(arm.body); diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index f50945a4de05d..060a3b521a4f3 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -82,7 +82,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { cond, Some(condition_scope), condition_scope, - source_info + source_info, + true, )); this.expr_into_dest(destination, then_blk, then) @@ -173,6 +174,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { Some(condition_scope), condition_scope, source_info, + true, ) }); let (short_circuit, continuation, constant) = match op { diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 7f29e3308f4f5..1872c762edbdc 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -40,6 +40,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override: Option, break_scope: region::Scope, variable_source_info: SourceInfo, + declare_bindings: bool, ) -> BlockAnd<()> { let this = self; let expr = &this.thir[expr_id]; @@ -53,6 +54,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + declare_bindings, )); let rhs_then_block = unpack!(this.then_else_break( @@ -61,6 +63,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + declare_bindings, )); rhs_then_block.unit() @@ -75,6 +78,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, local_scope, variable_source_info, + true, ) }); let rhs_success_block = unpack!(this.then_else_break( @@ -83,6 +87,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + true, )); this.cfg.goto(lhs_success_block, variable_source_info, rhs_success_block); rhs_success_block.unit() @@ -102,6 +107,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, local_scope, variable_source_info, + true, ) }); this.break_for_else(success_block, break_scope, variable_source_info); @@ -116,6 +122,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + declare_bindings, ) }) } @@ -125,6 +132,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { temp_scope_override, break_scope, variable_source_info, + declare_bindings, ), ExprKind::Let { expr, ref pat } => this.lower_let_expr( block, @@ -133,7 +141,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { break_scope, Some(variable_source_info.scope), variable_source_info.span, - true, + declare_bindings, ), _ => { let temp_scope = temp_scope_override.unwrap_or_else(|| this.local_scope()); @@ -737,13 +745,40 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); }, ); - if let Some(Guard::IfLet(guard_pat, _)) = guard { - // FIXME: pass a proper `opt_match_place` - self.declare_bindings(visibility_scope, scope_span, guard_pat, None, None); + if let Some(&Guard::If(guard_expr)) = guard { + self.declare_guard_bindings(guard_expr, scope_span, visibility_scope); } visibility_scope } + /// Declare bindings in a guard. This has to be done when declaring bindings + /// for an arm to ensure that or patterns only have one version of each + /// variable. + pub(crate) fn declare_guard_bindings( + &mut self, + guard_expr: ExprId, + scope_span: Span, + visibility_scope: Option, + ) { + match self.thir.exprs[guard_expr].kind { + ExprKind::Let { expr: _, pat: ref guard_pat } => { + // FIXME: pass a proper `opt_match_place` + self.declare_bindings(visibility_scope, scope_span, guard_pat, None, None); + } + ExprKind::Scope { value, .. } => { + self.declare_guard_bindings(value, scope_span, visibility_scope); + } + ExprKind::Use { source } => { + self.declare_guard_bindings(source, scope_span, visibility_scope); + } + ExprKind::LogicalOp { op: LogicalOp::And, lhs, rhs } => { + self.declare_guard_bindings(lhs, scope_span, visibility_scope); + self.declare_guard_bindings(rhs, scope_span, visibility_scope); + } + _ => {} + } + } + pub(crate) fn storage_live_binding( &mut self, block: BasicBlock, @@ -2043,6 +2078,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None, match_scope, this.source_info(arm.span), + false, ) } Guard::IfLet(ref pat, s) => { diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index 8ec70c58c4618..da5bd3550ac9e 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -855,13 +855,8 @@ impl<'tcx> Cx<'tcx> { fn convert_arm(&mut self, arm: &'tcx hir::Arm<'tcx>) -> ArmId { let arm = Arm { - pattern: self.pattern_from_hir(arm.pat), - guard: arm.guard.as_ref().map(|g| match g { - hir::Guard::If(e) => Guard::If(self.mirror_expr(e)), - hir::Guard::IfLet(l) => { - Guard::IfLet(self.pattern_from_hir(l.pat), self.mirror_expr(l.init)) - } - }), + pattern: self.pattern_from_hir(&arm.pat), + guard: arm.guard.as_ref().map(|g| Guard::If(self.mirror_expr(g))), body: self.mirror_expr(arm.body), lint_level: LintLevel::Explicit(arm.hir_id), scope: region::Scope { id: arm.hir_id.local_id, data: region::ScopeData::Node }, diff --git a/compiler/rustc_passes/src/liveness.rs b/compiler/rustc_passes/src/liveness.rs index cfe829f170f7e..8fa4fa1e38488 100644 --- a/compiler/rustc_passes/src/liveness.rs +++ b/compiler/rustc_passes/src/liveness.rs @@ -351,10 +351,7 @@ impl<'tcx> Visitor<'tcx> for IrMaps<'tcx> { } fn visit_arm(&mut self, arm: &'tcx hir::Arm<'tcx>) { - self.add_from_pat(arm.pat); - if let Some(hir::Guard::IfLet(let_expr)) = arm.guard { - self.add_from_pat(let_expr.pat); - } + self.add_from_pat(&arm.pat); intravisit::walk_arm(self, arm); } @@ -921,14 +918,11 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { for arm in arms { let body_succ = self.propagate_through_expr(arm.body, succ); - let guard_succ = arm.guard.as_ref().map_or(body_succ, |g| match g { - hir::Guard::If(e) => self.propagate_through_expr(e, body_succ), - hir::Guard::IfLet(let_expr) => { - let let_bind = self.define_bindings_in_pat(let_expr.pat, body_succ); - self.propagate_through_expr(let_expr.init, let_bind) - } - }); - let arm_succ = self.define_bindings_in_pat(arm.pat, guard_succ); + let guard_succ = arm + .guard + .as_ref() + .map_or(body_succ, |g| self.propagate_through_expr(g, body_succ)); + let arm_succ = self.define_bindings_in_pat(&arm.pat, guard_succ); self.merge_from_succ(ln, arm_succ); } self.propagate_through_expr(e, ln) @@ -1328,9 +1322,6 @@ impl<'a, 'tcx> Visitor<'tcx> for Liveness<'a, 'tcx> { fn visit_arm(&mut self, arm: &'tcx hir::Arm<'tcx>) { self.check_unused_vars_in_pat(arm.pat, None, None, |_, _, _, _| {}); - if let Some(hir::Guard::IfLet(let_expr)) = arm.guard { - self.check_unused_vars_in_pat(let_expr.pat, None, None, |_, _, _, _| {}); - } intravisit::walk_arm(self, arm); } } diff --git a/tests/ui/closures/2229_closure_analysis/issue-88118-2.stderr b/tests/ui/closures/2229_closure_analysis/issue-88118-2.stderr index b3cb558f97679..34b3eab2345bd 100644 --- a/tests/ui/closures/2229_closure_analysis/issue-88118-2.stderr +++ b/tests/ui/closures/2229_closure_analysis/issue-88118-2.stderr @@ -1,8 +1,8 @@ warning: irrefutable `if let` guard pattern - --> $DIR/issue-88118-2.rs:10:29 + --> $DIR/issue-88118-2.rs:10:25 | LL | Registry if let _ = registry.try_find_description() => { } - | ^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this pattern will always match, so the guard is useless = help: consider removing the guard and adding a `let` inside the match arm diff --git a/tests/ui/lint/lint-match-arms-2.stderr b/tests/ui/lint/lint-match-arms-2.stderr index 062d5c12e9610..5e803ef193480 100644 --- a/tests/ui/lint/lint-match-arms-2.stderr +++ b/tests/ui/lint/lint-match-arms-2.stderr @@ -11,10 +11,10 @@ LL | #[deny(bindings_with_variant_name)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: irrefutable `if let` guard pattern - --> $DIR/lint-match-arms-2.rs:18:18 + --> $DIR/lint-match-arms-2.rs:18:14 | LL | a if let b = a => {} - | ^ + | ^^^^^^^^^ | = note: this pattern will always match, so the guard is useless = help: consider removing the guard and adding a `let` inside the match arm diff --git a/tests/ui/pattern/usefulness/deny-irrefutable-let-patterns.stderr b/tests/ui/pattern/usefulness/deny-irrefutable-let-patterns.stderr index cdb6b5c7a4971..e8b7f40c70e48 100644 --- a/tests/ui/pattern/usefulness/deny-irrefutable-let-patterns.stderr +++ b/tests/ui/pattern/usefulness/deny-irrefutable-let-patterns.stderr @@ -22,10 +22,10 @@ LL | while let _ = 5 { = help: consider instead using a `loop { ... }` with a `let` inside it error: irrefutable `if let` guard pattern - --> $DIR/deny-irrefutable-let-patterns.rs:13:18 + --> $DIR/deny-irrefutable-let-patterns.rs:13:14 | LL | _ if let _ = 2 => {} - | ^ + | ^^^^^^^^^ | = note: this pattern will always match, so the guard is useless = help: consider removing the guard and adding a `let` inside the match arm diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/warns.stderr b/tests/ui/rfcs/rfc-2294-if-let-guard/warns.stderr index 75f22ac8dc03a..eed5dbb88deb0 100644 --- a/tests/ui/rfcs/rfc-2294-if-let-guard/warns.stderr +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/warns.stderr @@ -1,8 +1,8 @@ error: irrefutable `if let` guard pattern - --> $DIR/warns.rs:6:24 + --> $DIR/warns.rs:6:20 | LL | Some(x) if let () = x => {} - | ^^ + | ^^^^^^^^^^ | = note: this pattern will always match, so the guard is useless = help: consider removing the guard and adding a `let` inside the match arm diff --git a/tests/ui/stats/hir-stats.stderr b/tests/ui/stats/hir-stats.stderr index 5296475c94a78..8b9ec30db63f4 100644 --- a/tests/ui/stats/hir-stats.stderr +++ b/tests/ui/stats/hir-stats.stderr @@ -128,8 +128,8 @@ hir-stats Param 64 ( 0.7%) 2 32 hir-stats Body 72 ( 0.8%) 3 24 hir-stats InlineAsm 72 ( 0.8%) 1 72 hir-stats ImplItemRef 72 ( 0.8%) 2 36 +hir-stats Arm 80 ( 0.9%) 2 40 hir-stats FieldDef 96 ( 1.1%) 2 48 -hir-stats Arm 96 ( 1.1%) 2 48 hir-stats Stmt 96 ( 1.1%) 3 32 hir-stats - Local 32 ( 0.4%) 1 hir-stats - Semi 32 ( 0.4%) 1 @@ -151,11 +151,11 @@ hir-stats - Wild 72 ( 0.8%) 1 hir-stats - Struct 72 ( 0.8%) 1 hir-stats - Binding 216 ( 2.4%) 3 hir-stats GenericParam 400 ( 4.4%) 5 80 -hir-stats Generics 560 ( 6.1%) 10 56 +hir-stats Generics 560 ( 6.2%) 10 56 hir-stats Ty 720 ( 7.9%) 15 48 hir-stats - Ptr 48 ( 0.5%) 1 hir-stats - Ref 48 ( 0.5%) 1 -hir-stats - Path 624 ( 6.8%) 13 +hir-stats - Path 624 ( 6.9%) 13 hir-stats Expr 768 ( 8.4%) 12 64 hir-stats - Path 64 ( 0.7%) 1 hir-stats - Struct 64 ( 0.7%) 1 @@ -174,5 +174,5 @@ hir-stats - Use 352 ( 3.9%) 4 hir-stats Path 1_240 (13.6%) 31 40 hir-stats PathSegment 1_920 (21.1%) 40 48 hir-stats ---------------------------------------------------------------- -hir-stats Total 9_112 +hir-stats Total 9_096 hir-stats From a549711f6e3dc804783652810a40653719dd0af7 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 21 Sep 2023 11:27:29 +0000 Subject: [PATCH 10/14] Remove `thir::Guard` Use Expr instead. Use `ExprKind::Let` to represent if let guards. --- compiler/rustc_middle/src/thir.rs | 9 +---- compiler/rustc_middle/src/thir/visit.rs | 11 ++---- .../rustc_mir_build/src/build/matches/mod.rs | 34 ++++++++----------- compiler/rustc_mir_build/src/thir/cx/expr.rs | 2 +- .../src/thir/pattern/check_match.rs | 18 +++------- compiler/rustc_mir_build/src/thir/print.rs | 25 ++------------ tests/ui/rfcs/rfc-2294-if-let-guard/scope.rs | 30 ++++++++++++++++ 7 files changed, 55 insertions(+), 74 deletions(-) create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/scope.rs diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs index b6759d3521023..2b5983314eecf 100644 --- a/compiler/rustc_middle/src/thir.rs +++ b/compiler/rustc_middle/src/thir.rs @@ -519,20 +519,13 @@ pub struct FruInfo<'tcx> { #[derive(Clone, Debug, HashStable)] pub struct Arm<'tcx> { pub pattern: Box>, - pub guard: Option>, + pub guard: Option, pub body: ExprId, pub lint_level: LintLevel, pub scope: region::Scope, pub span: Span, } -/// A `match` guard. -#[derive(Clone, Debug, HashStable)] -pub enum Guard<'tcx> { - If(ExprId), - IfLet(Box>, ExprId), -} - #[derive(Copy, Clone, Debug, HashStable)] pub enum LogicalOp { /// The `&&` operator. diff --git a/compiler/rustc_middle/src/thir/visit.rs b/compiler/rustc_middle/src/thir/visit.rs index ade3ea289cc59..4847a7bea91d7 100644 --- a/compiler/rustc_middle/src/thir/visit.rs +++ b/compiler/rustc_middle/src/thir/visit.rs @@ -1,5 +1,5 @@ use super::{ - AdtExpr, Arm, Block, ClosureExpr, Expr, ExprKind, Guard, InlineAsmExpr, InlineAsmOperand, Pat, + AdtExpr, Arm, Block, ClosureExpr, Expr, ExprKind, InlineAsmExpr, InlineAsmOperand, Pat, PatKind, Stmt, StmtKind, Thir, }; @@ -213,13 +213,8 @@ pub fn walk_arm<'thir, 'tcx: 'thir, V: Visitor<'thir, 'tcx>>( visitor: &mut V, arm: &'thir Arm<'tcx>, ) { - match arm.guard { - Some(Guard::If(expr)) => visitor.visit_expr(&visitor.thir()[expr]), - Some(Guard::IfLet(ref pat, expr)) => { - visitor.visit_pat(pat); - visitor.visit_expr(&visitor.thir()[expr]); - } - None => {} + if let Some(expr) = arm.guard { + visitor.visit_expr(&visitor.thir()[expr]) } visitor.visit_pat(&arm.pattern); visitor.visit_expr(&visitor.thir()[arm.body]); diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 1872c762edbdc..483e70fd6f167 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -425,7 +425,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None, arm.span, &arm.pattern, - arm.guard.as_ref(), + arm.guard, opt_scrutinee_place, ); @@ -717,7 +717,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { mut visibility_scope: Option, scope_span: Span, pattern: &Pat<'tcx>, - guard: Option<&Guard<'tcx>>, + guard: Option, opt_match_place: Option<(Option<&Place<'tcx>>, Span)>, ) -> Option { self.visit_primary_bindings( @@ -745,7 +745,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); }, ); - if let Some(&Guard::If(guard_expr)) = guard { + if let Some(guard_expr) = guard { self.declare_guard_bindings(guard_expr, scope_span, visibility_scope); } visibility_scope @@ -2044,7 +2044,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // * So we eagerly create the reference for the arm and then take a // reference to that. if let Some((arm, match_scope)) = arm_match_scope - && let Some(guard) = &arm.guard + && let Some(guard) = arm.guard { let tcx = self.tcx; let bindings = parent_bindings @@ -2069,22 +2069,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let mut guard_span = rustc_span::DUMMY_SP; let (post_guard_block, otherwise_post_guard_block) = - self.in_if_then_scope(match_scope, guard_span, |this| match *guard { - Guard::If(e) => { - guard_span = this.thir[e].span; - this.then_else_break( - block, - e, - None, - match_scope, - this.source_info(arm.span), - false, - ) - } - Guard::IfLet(ref pat, s) => { - guard_span = this.thir[s].span; - this.lower_let_expr(block, s, pat, match_scope, None, arm.span, false) - } + self.in_if_then_scope(match_scope, guard_span, |this| { + guard_span = this.thir[guard].span; + this.then_else_break( + block, + guard, + None, + match_scope, + this.source_info(arm.span), + false, + ) }); let source_info = self.source_info(guard_span); diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index da5bd3550ac9e..78d72b3028416 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -856,7 +856,7 @@ impl<'tcx> Cx<'tcx> { fn convert_arm(&mut self, arm: &'tcx hir::Arm<'tcx>) -> ArmId { let arm = Arm { pattern: self.pattern_from_hir(&arm.pat), - guard: arm.guard.as_ref().map(|g| Guard::If(self.mirror_expr(g))), + guard: arm.guard.as_ref().map(|g| self.mirror_expr(g)), body: self.mirror_expr(arm.body), lint_level: LintLevel::Explicit(arm.hir_id), scope: region::Scope { id: arm.hir_id.local_id, data: region::ScopeData::Node }, diff --git a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs index 0bcc2a315ff73..f0c767e6ca1ba 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs @@ -100,20 +100,10 @@ impl<'p, 'tcx> Visitor<'p, 'tcx> for MatchVisitor<'p, 'tcx> { #[instrument(level = "trace", skip(self))] fn visit_arm(&mut self, arm: &'p Arm<'tcx>) { self.with_lint_level(arm.lint_level, |this| { - match arm.guard { - Some(Guard::If(expr)) => { - this.with_let_source(LetSource::IfLetGuard, |this| { - this.visit_expr(&this.thir[expr]) - }); - } - Some(Guard::IfLet(ref pat, expr)) => { - this.with_let_source(LetSource::IfLetGuard, |this| { - this.check_let(pat, Some(expr), pat.span); - this.visit_pat(pat); - this.visit_expr(&this.thir[expr]); - }); - } - None => {} + if let Some(expr) = arm.guard { + this.with_let_source(LetSource::IfLetGuard, |this| { + this.visit_expr(&this.thir[expr]) + }); } this.visit_pat(&arm.pattern); this.visit_expr(&self.thir[arm.body]); diff --git a/compiler/rustc_mir_build/src/thir/print.rs b/compiler/rustc_mir_build/src/thir/print.rs index 28be313990594..267ea3aa3e12c 100644 --- a/compiler/rustc_mir_build/src/thir/print.rs +++ b/compiler/rustc_mir_build/src/thir/print.rs @@ -593,9 +593,9 @@ impl<'a, 'tcx> ThirPrinter<'a, 'tcx> { print_indented!(self, "pattern: ", depth_lvl + 1); self.print_pat(pattern, depth_lvl + 2); - if let Some(guard) = guard { + if let Some(guard) = *guard { print_indented!(self, "guard: ", depth_lvl + 1); - self.print_guard(guard, depth_lvl + 2); + self.print_expr(guard, depth_lvl + 2); } else { print_indented!(self, "guard: None", depth_lvl + 1); } @@ -764,27 +764,6 @@ impl<'a, 'tcx> ThirPrinter<'a, 'tcx> { print_indented!(self, "}", depth_lvl); } - fn print_guard(&mut self, guard: &Guard<'tcx>, depth_lvl: usize) { - print_indented!(self, "Guard {", depth_lvl); - - match guard { - Guard::If(expr_id) => { - print_indented!(self, "If (", depth_lvl + 1); - self.print_expr(*expr_id, depth_lvl + 2); - print_indented!(self, ")", depth_lvl + 1); - } - Guard::IfLet(pat, expr_id) => { - print_indented!(self, "IfLet (", depth_lvl + 1); - self.print_pat(pat, depth_lvl + 2); - print_indented!(self, ",", depth_lvl + 1); - self.print_expr(*expr_id, depth_lvl + 2); - print_indented!(self, ")", depth_lvl + 1); - } - } - - print_indented!(self, "}", depth_lvl); - } - fn print_closure_expr(&mut self, expr: &ClosureExpr<'tcx>, depth_lvl: usize) { let ClosureExpr { closure_id, args, upvars, movability, fake_reads } = expr; diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/scope.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/scope.rs new file mode 100644 index 0000000000000..9a3520661a6f6 --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/scope.rs @@ -0,0 +1,30 @@ +// Tests for #88015 when using if let chains in match guards + +//run-pass + +#![feature(if_let_guard)] +#![feature(let_chains)] +#![allow(irrefutable_let_patterns)] + +fn lhs_let(opt: Option) { + match opt { + None | Some(false) | Some(true) if let x = 42 && true => assert_eq!(x, 42), + _ => panic!() + } +} + +fn rhs_let(opt: Option) { + match opt { + None | Some(false) | Some(true) if true && let x = 41 => assert_eq!(x, 41), + _ => panic!() + } +} + +fn main() { + lhs_let(None); + lhs_let(Some(false)); + lhs_let(Some(true)); + rhs_let(None); + rhs_let(Some(false)); + rhs_let(Some(true)); +} From 1a267e3f40c4c6e32482a7dd98c512f4664a329e Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Wed, 3 Jan 2024 16:32:13 +0000 Subject: [PATCH 11/14] Restore if let guard temporary scoping difference Match guards with an if let guard or an if let chain guard should have a temporary scope of the whole arm. This is to allow ref bindings to temporaries to borrow check. --- .../rustc_hir_analysis/src/check/region.rs | 13 +++- .../rustc_mir_build/src/build/matches/mod.rs | 6 ++ .../rfcs/rfc-2294-if-let-guard/drop-scope.rs | 72 +++++++++++++++++++ 3 files changed, 89 insertions(+), 2 deletions(-) create mode 100644 tests/ui/rfcs/rfc-2294-if-let-guard/drop-scope.rs diff --git a/compiler/rustc_hir_analysis/src/check/region.rs b/compiler/rustc_hir_analysis/src/check/region.rs index c3bdf94d30f97..542e69a6c34de 100644 --- a/compiler/rustc_hir_analysis/src/check/region.rs +++ b/compiler/rustc_hir_analysis/src/check/region.rs @@ -177,6 +177,14 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h } fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir::Arm<'tcx>) { + fn has_let_expr(expr: &Expr<'_>) -> bool { + match &expr.kind { + hir::ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs), + hir::ExprKind::Let(..) => true, + _ => false, + } + } + let prev_cx = visitor.cx; visitor.terminating_scopes.insert(arm.hir_id.local_id); @@ -184,8 +192,9 @@ fn resolve_arm<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, arm: &'tcx hir visitor.enter_node_scope_with_dtor(arm.hir_id.local_id); visitor.cx.var_parent = visitor.cx.parent; - if let Some(expr) = arm.guard { - // Check for if?? + if let Some(expr) = arm.guard + && !has_let_expr(expr) + { visitor.terminating_scopes.insert(expr.hir_id.local_id); } diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 483e70fd6f167..906b3205ca75d 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -33,6 +33,12 @@ use std::borrow::Borrow; use std::mem; impl<'a, 'tcx> Builder<'a, 'tcx> { + /// Lowers a condition in a way that ensures that variables bound in any let + /// expressions are definitely initialized in the if body. + /// + /// If `declare_bindings` is false then variables created in `let` + /// expressions will not be declared. This is for if let guards on arms with + /// an or pattern, where the guard is lowered multiple times. pub(crate) fn then_else_break( &mut self, mut block: BasicBlock, diff --git a/tests/ui/rfcs/rfc-2294-if-let-guard/drop-scope.rs b/tests/ui/rfcs/rfc-2294-if-let-guard/drop-scope.rs new file mode 100644 index 0000000000000..9e6e23e8882c6 --- /dev/null +++ b/tests/ui/rfcs/rfc-2294-if-let-guard/drop-scope.rs @@ -0,0 +1,72 @@ +// Ensure that temporaries in if-let guards live for the arm +// regression test for #118593 + +// check-pass + +#![feature(if_let_guard)] +#![feature(let_chains)] + +fn get_temp() -> Option { + None +} + +fn let_guard(num: u8) { + match num { + 1 | 2 if let Some(ref a) = get_temp() => { + let _b = a; + } + _ => {} + } + match num { + 3 | 4 if let Some(ref mut c) = get_temp() => { + let _d = c; + } + _ => {} + } +} + +fn let_let_chain_guard(num: u8) { + match num { + 5 | 6 + if let Some(ref a) = get_temp() + && let Some(ref b) = get_temp() => + { + let _x = a; + let _y = b; + } + _ => {} + } + match num { + 7 | 8 + if let Some(ref mut c) = get_temp() + && let Some(ref mut d) = get_temp() => + { + let _w = c; + let _z = d; + } + _ => {} + } +} + +fn let_cond_chain_guard(num: u8) { + match num { + 9 | 10 + if let Some(ref a) = get_temp() + && true => + { + let _x = a; + } + _ => {} + } + match num { + 11 | 12 + if let Some(ref mut b) = get_temp() + && true => + { + let _w = b; + } + _ => {} + } +} + +fn main() {} From 44bba5486eebcb6a67f92f43694e42bc074c69ab Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Thu, 4 Jan 2024 10:29:47 +0000 Subject: [PATCH 12/14] Update clippy for hir::Guard removal --- src/tools/clippy/clippy_lints/src/entry.rs | 4 +- .../clippy/clippy_lints/src/manual_clamp.rs | 4 +- .../src/matches/collapsible_match.rs | 8 +-- .../src/matches/match_like_matches.rs | 23 ++------- .../src/matches/needless_match.rs | 17 ++----- .../src/matches/redundant_guards.rs | 50 ++++++++----------- .../src/matches/redundant_pattern_match.rs | 20 ++++---- .../src/mixed_read_write_in_expression.rs | 4 +- .../clippy/clippy_lints/src/utils/author.rs | 10 +--- .../clippy/clippy_utils/src/hir_utils.rs | 24 ++------- src/tools/clippy/clippy_utils/src/lib.rs | 4 +- 11 files changed, 56 insertions(+), 112 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/entry.rs b/src/tools/clippy/clippy_lints/src/entry.rs index ce0a1dfdc61f2..0b4bc375df0d9 100644 --- a/src/tools/clippy/clippy_lints/src/entry.rs +++ b/src/tools/clippy/clippy_lints/src/entry.rs @@ -8,7 +8,7 @@ use core::fmt::{self, Write}; use rustc_errors::Applicability; use rustc_hir::hir_id::HirIdSet; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{Block, Expr, ExprKind, Guard, HirId, Let, Pat, Stmt, StmtKind, UnOp}; +use rustc_hir::{Block, Expr, ExprKind, HirId, Pat, Stmt, StmtKind, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::{Span, SyntaxContext, DUMMY_SP}; @@ -465,7 +465,7 @@ impl<'tcx> Visitor<'tcx> for InsertSearcher<'_, 'tcx> { let mut is_map_used = self.is_map_used; for arm in arms { self.visit_pat(arm.pat); - if let Some(Guard::If(guard) | Guard::IfLet(&Let { init: guard, .. })) = arm.guard { + if let Some(guard) = arm.guard { self.visit_non_tail_expr(guard); } is_map_used |= self.visit_cond_arm(arm.body); diff --git a/src/tools/clippy/clippy_lints/src/manual_clamp.rs b/src/tools/clippy/clippy_lints/src/manual_clamp.rs index 385fe387a314f..0da309f9531e0 100644 --- a/src/tools/clippy/clippy_lints/src/manual_clamp.rs +++ b/src/tools/clippy/clippy_lints/src/manual_clamp.rs @@ -11,7 +11,7 @@ use clippy_utils::{ use itertools::Itertools; use rustc_errors::{Applicability, Diagnostic}; use rustc_hir::def::Res; -use rustc_hir::{Arm, BinOpKind, Block, Expr, ExprKind, Guard, HirId, PatKind, PathSegment, PrimTy, QPath, StmtKind}; +use rustc_hir::{Arm, BinOpKind, Block, Expr, ExprKind, HirId, PatKind, PathSegment, PrimTy, QPath, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty; use rustc_session::impl_lint_pass; @@ -394,7 +394,7 @@ fn is_match_pattern<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Opt // Find possible min/max branches let minmax_values = |a: &'tcx Arm<'tcx>| { if let PatKind::Binding(_, var_hir_id, _, None) = &a.pat.kind - && let Some(Guard::If(e)) = a.guard + && let Some(e) = a.guard { Some((e, var_hir_id, a.body)) } else { diff --git a/src/tools/clippy/clippy_lints/src/matches/collapsible_match.rs b/src/tools/clippy/clippy_lints/src/matches/collapsible_match.rs index 91e6ca7fa8bc1..5fef5930fab25 100644 --- a/src/tools/clippy/clippy_lints/src/matches/collapsible_match.rs +++ b/src/tools/clippy/clippy_lints/src/matches/collapsible_match.rs @@ -7,7 +7,7 @@ use clippy_utils::{ }; use rustc_errors::MultiSpan; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, Expr, Guard, HirId, Let, Pat, PatKind}; +use rustc_hir::{Arm, Expr, HirId, Pat, PatKind}; use rustc_lint::LateContext; use rustc_span::Span; @@ -16,7 +16,7 @@ use super::COLLAPSIBLE_MATCH; pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>]) { if let Some(els_arm) = arms.iter().rfind(|arm| arm_is_wild_like(cx, arm)) { for arm in arms { - check_arm(cx, true, arm.pat, arm.body, arm.guard.as_ref(), Some(els_arm.body)); + check_arm(cx, true, arm.pat, arm.body, arm.guard, Some(els_arm.body)); } } } @@ -35,7 +35,7 @@ fn check_arm<'tcx>( outer_is_match: bool, outer_pat: &'tcx Pat<'tcx>, outer_then_body: &'tcx Expr<'tcx>, - outer_guard: Option<&'tcx Guard<'tcx>>, + outer_guard: Option<&'tcx Expr<'tcx>>, outer_else_body: Option<&'tcx Expr<'tcx>>, ) { let inner_expr = peel_blocks_with_stmt(outer_then_body); @@ -71,7 +71,7 @@ fn check_arm<'tcx>( // the binding must not be used in the if guard && outer_guard.map_or( true, - |(Guard::If(e) | Guard::IfLet(Let { init: e, .. }))| !is_local_used(cx, *e, binding_id) + |e| !is_local_used(cx, e, binding_id) ) // ...or anywhere in the inner expression && match inner { diff --git a/src/tools/clippy/clippy_lints/src/matches/match_like_matches.rs b/src/tools/clippy/clippy_lints/src/matches/match_like_matches.rs index 56123326fe4a7..b062e81cefddd 100644 --- a/src/tools/clippy/clippy_lints/src/matches/match_like_matches.rs +++ b/src/tools/clippy/clippy_lints/src/matches/match_like_matches.rs @@ -4,7 +4,7 @@ use clippy_utils::source::snippet_with_applicability; use clippy_utils::{is_lint_allowed, is_wild, span_contains_comment}; use rustc_ast::{Attribute, LitKind}; use rustc_errors::Applicability; -use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Guard, Pat, PatKind, QPath}; +use rustc_hir::{Arm, BorrowKind, Expr, ExprKind, Pat, PatKind, QPath}; use rustc_lint::{LateContext, LintContext}; use rustc_middle::ty; use rustc_span::source_map::Spanned; @@ -41,14 +41,8 @@ pub(super) fn check_match<'tcx>( find_matches_sugg( cx, scrutinee, - arms.iter().map(|arm| { - ( - cx.tcx.hir().attrs(arm.hir_id), - Some(arm.pat), - arm.body, - arm.guard.as_ref(), - ) - }), + arms.iter() + .map(|arm| (cx.tcx.hir().attrs(arm.hir_id), Some(arm.pat), arm.body, arm.guard)), e, false, ) @@ -67,14 +61,7 @@ where I: Clone + DoubleEndedIterator + ExactSizeIterator - + Iterator< - Item = ( - &'a [Attribute], - Option<&'a Pat<'b>>, - &'a Expr<'b>, - Option<&'a Guard<'b>>, - ), - >, + + Iterator>, &'a Expr<'b>, Option<&'a Expr<'b>>)>, { if !span_contains_comment(cx.sess().source_map(), expr.span) && iter.len() >= 2 @@ -115,7 +102,7 @@ where }) .join(" | ") }; - let pat_and_guard = if let Some(Guard::If(g)) = first_guard { + let pat_and_guard = if let Some(g) = first_guard { format!( "{pat} if {}", snippet_with_applicability(cx, g.span, "..", &mut applicability) diff --git a/src/tools/clippy/clippy_lints/src/matches/needless_match.rs b/src/tools/clippy/clippy_lints/src/matches/needless_match.rs index 44dc29c36a6ba..cc482f15a91df 100644 --- a/src/tools/clippy/clippy_lints/src/matches/needless_match.rs +++ b/src/tools/clippy/clippy_lints/src/matches/needless_match.rs @@ -8,7 +8,7 @@ use clippy_utils::{ }; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, BindingAnnotation, ByRef, Expr, ExprKind, Guard, ItemKind, Node, Pat, PatKind, Path, QPath}; +use rustc_hir::{Arm, BindingAnnotation, ByRef, Expr, ExprKind, ItemKind, Node, Pat, PatKind, Path, QPath}; use rustc_lint::LateContext; use rustc_span::sym; @@ -66,18 +66,9 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>]) let arm_expr = peel_blocks_with_stmt(arm.body); if let Some(guard_expr) = &arm.guard { - match guard_expr { - // gives up if `pat if expr` can have side effects - Guard::If(if_cond) => { - if if_cond.can_have_side_effects() { - return false; - } - }, - // gives up `pat if let ...` arm - Guard::IfLet(_) => { - return false; - }, - }; + if guard_expr.can_have_side_effects() { + return false; + } } if let PatKind::Wild = arm.pat.kind { diff --git a/src/tools/clippy/clippy_lints/src/matches/redundant_guards.rs b/src/tools/clippy/clippy_lints/src/matches/redundant_guards.rs index f57b22374c8e8..dfaaeb14ca3cb 100644 --- a/src/tools/clippy/clippy_lints/src/matches/redundant_guards.rs +++ b/src/tools/clippy/clippy_lints/src/matches/redundant_guards.rs @@ -5,7 +5,7 @@ use clippy_utils::visitors::{for_each_expr, is_local_used}; use rustc_ast::{BorrowKind, LitKind}; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, Guard, MatchSource, Node, Pat, PatKind}; +use rustc_hir::{Arm, BinOpKind, Expr, ExprKind, MatchSource, Node, Pat, PatKind}; use rustc_lint::LateContext; use rustc_span::symbol::Ident; use rustc_span::{Span, Symbol}; @@ -21,20 +21,19 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { }; // `Some(x) if matches!(x, y)` - if let Guard::If(if_expr) = guard - && let ExprKind::Match( - scrutinee, - [ - arm, - Arm { - pat: Pat { - kind: PatKind::Wild, .. - }, - .. + if let ExprKind::Match( + scrutinee, + [ + arm, + Arm { + pat: Pat { + kind: PatKind::Wild, .. }, - ], - MatchSource::Normal, - ) = if_expr.kind + .. + }, + ], + MatchSource::Normal, + ) = guard.kind && let Some(binding) = get_pat_binding(cx, scrutinee, outer_arm) { let pat_span = match (arm.pat.kind, binding.byref_ident) { @@ -45,14 +44,14 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { emit_redundant_guards( cx, outer_arm, - if_expr.span, + guard.span, snippet(cx, pat_span, ""), &binding, arm.guard, ); } // `Some(x) if let Some(2) = x` - else if let Guard::IfLet(let_expr) = guard + else if let ExprKind::Let(let_expr) = guard.kind && let Some(binding) = get_pat_binding(cx, let_expr.init, outer_arm) { let pat_span = match (let_expr.pat.kind, binding.byref_ident) { @@ -71,8 +70,7 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { } // `Some(x) if x == Some(2)` // `Some(x) if Some(2) == x` - else if let Guard::If(if_expr) = guard - && let ExprKind::Binary(bin_op, local, pat) = if_expr.kind + else if let ExprKind::Binary(bin_op, local, pat) = guard.kind && matches!(bin_op.node, BinOpKind::Eq) // Ensure they have the same type. If they don't, we'd need deref coercion which isn't // possible (currently) in a pattern. In some cases, you can use something like @@ -96,16 +94,15 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) { emit_redundant_guards( cx, outer_arm, - if_expr.span, + guard.span, snippet(cx, pat_span, ""), &binding, None, ); - } else if let Guard::If(if_expr) = guard - && let ExprKind::MethodCall(path, recv, args, ..) = if_expr.kind + } else if let ExprKind::MethodCall(path, recv, args, ..) = guard.kind && let Some(binding) = get_pat_binding(cx, recv, outer_arm) { - check_method_calls(cx, outer_arm, path.ident.name, recv, args, if_expr, &binding); + check_method_calls(cx, outer_arm, path.ident.name, recv, args, guard, &binding); } } } @@ -216,7 +213,7 @@ fn emit_redundant_guards<'tcx>( guard_span: Span, binding_replacement: Cow<'static, str>, pat_binding: &PatBindingInfo, - inner_guard: Option>, + inner_guard: Option<&Expr<'_>>, ) { span_lint_and_then( cx, @@ -242,12 +239,7 @@ fn emit_redundant_guards<'tcx>( ( guard_span.source_callsite().with_lo(outer_arm.pat.span.hi()), inner_guard.map_or_else(String::new, |guard| { - let (prefix, span) = match guard { - Guard::If(e) => ("if", e.span), - Guard::IfLet(l) => ("if let", l.span), - }; - - format!(" {prefix} {}", snippet(cx, span, "")) + format!(" if {}", snippet(cx, guard.span, "")) }), ), ], diff --git a/src/tools/clippy/clippy_lints/src/matches/redundant_pattern_match.rs b/src/tools/clippy/clippy_lints/src/matches/redundant_pattern_match.rs index a4acdfb1db4e1..b5870d94d996a 100644 --- a/src/tools/clippy/clippy_lints/src/matches/redundant_pattern_match.rs +++ b/src/tools/clippy/clippy_lints/src/matches/redundant_pattern_match.rs @@ -9,7 +9,7 @@ use rustc_ast::ast::LitKind; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; use rustc_hir::LangItem::{self, OptionNone, OptionSome, PollPending, PollReady, ResultErr, ResultOk}; -use rustc_hir::{Arm, Expr, ExprKind, Guard, Node, Pat, PatKind, QPath, UnOp}; +use rustc_hir::{Arm, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp}; use rustc_lint::LateContext; use rustc_middle::ty::{self, GenericArgKind, Ty}; use rustc_span::{sym, Span, Symbol}; @@ -277,8 +277,6 @@ pub(super) fn check_match<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, op let mut sugg = format!("{}.{good_method}", snippet(cx, result_expr.span, "_")); if let Some(guard) = maybe_guard { - let Guard::If(guard) = *guard else { return }; // `...is_none() && let ...` is a syntax error - // wow, the HIR for match guards in `PAT if let PAT = expr && expr => ...` is annoying! // `guard` here is `Guard::If` with the let expression somewhere deep in the tree of exprs, // counter to the intuition that it should be `Guard::IfLet`, so we need another check @@ -319,7 +317,7 @@ fn found_good_method<'tcx>( cx: &LateContext<'_>, arms: &'tcx [Arm<'tcx>], node: (&PatKind<'_>, &PatKind<'_>), -) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> { +) -> Option<(&'static str, Option<&'tcx Expr<'tcx>>)> { match node { ( PatKind::TupleStruct(ref path_left, patterns_left, _), @@ -409,7 +407,7 @@ fn get_good_method<'tcx>( cx: &LateContext<'_>, arms: &'tcx [Arm<'tcx>], path_left: &QPath<'_>, -) -> Option<(&'static str, Option<&'tcx Guard<'tcx>>)> { +) -> Option<(&'static str, Option<&'tcx Expr<'tcx>>)> { if let Some(name) = get_ident(path_left) { let (expected_item_left, should_be_left, should_be_right) = match name.as_str() { "Ok" => (Item::Lang(ResultOk), "is_ok()", "is_err()"), @@ -478,7 +476,7 @@ fn find_good_method_for_match<'a, 'tcx>( expected_item_right: Item, should_be_left: &'a str, should_be_right: &'a str, -) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> { +) -> Option<(&'a str, Option<&'tcx Expr<'tcx>>)> { let first_pat = arms[0].pat; let second_pat = arms[1].pat; @@ -496,8 +494,8 @@ fn find_good_method_for_match<'a, 'tcx>( match body_node_pair { (ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) { - (LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())), - (LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())), + (LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard)), + (LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard)), _ => None, }, _ => None, @@ -511,7 +509,7 @@ fn find_good_method_for_matches_macro<'a, 'tcx>( expected_item_left: Item, should_be_left: &'a str, should_be_right: &'a str, -) -> Option<(&'a str, Option<&'tcx Guard<'tcx>>)> { +) -> Option<(&'a str, Option<&'tcx Expr<'tcx>>)> { let first_pat = arms[0].pat; let body_node_pair = if is_pat_variant(cx, first_pat, path_left, expected_item_left) { @@ -522,8 +520,8 @@ fn find_good_method_for_matches_macro<'a, 'tcx>( match body_node_pair { (ExprKind::Lit(lit_left), ExprKind::Lit(lit_right)) => match (&lit_left.node, &lit_right.node) { - (LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard.as_ref())), - (LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard.as_ref())), + (LitKind::Bool(true), LitKind::Bool(false)) => Some((should_be_left, arms[0].guard)), + (LitKind::Bool(false), LitKind::Bool(true)) => Some((should_be_right, arms[1].guard)), _ => None, }, _ => None, diff --git a/src/tools/clippy/clippy_lints/src/mixed_read_write_in_expression.rs b/src/tools/clippy/clippy_lints/src/mixed_read_write_in_expression.rs index 3ff40081c4724..195ce17629a71 100644 --- a/src/tools/clippy/clippy_lints/src/mixed_read_write_in_expression.rs +++ b/src/tools/clippy/clippy_lints/src/mixed_read_write_in_expression.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::{span_lint, span_lint_and_note}; use clippy_utils::{get_parent_expr, path_to_local, path_to_local_id}; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind}; +use rustc_hir::{BinOpKind, Block, Expr, ExprKind, HirId, Local, Node, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty; use rustc_session::declare_lint_pass; @@ -119,7 +119,7 @@ impl<'a, 'tcx> DivergenceVisitor<'a, 'tcx> { ExprKind::Match(e, arms, _) => { self.visit_expr(e); for arm in arms { - if let Some(Guard::If(if_expr)) = arm.guard { + if let Some(if_expr) = arm.guard { self.visit_expr(if_expr); } // make sure top level arm expressions aren't linted diff --git a/src/tools/clippy/clippy_lints/src/utils/author.rs b/src/tools/clippy/clippy_lints/src/utils/author.rs index 8817e46b3c8c0..8d38b87e1d794 100644 --- a/src/tools/clippy/clippy_lints/src/utils/author.rs +++ b/src/tools/clippy/clippy_lints/src/utils/author.rs @@ -318,17 +318,11 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> { self.pat(field!(arm.pat)); match arm.value.guard { None => chain!(self, "{arm}.guard.is_none()"), - Some(hir::Guard::If(expr)) => { + Some(expr) => { bind!(self, expr); - chain!(self, "let Some(Guard::If({expr})) = {arm}.guard"); + chain!(self, "let Some({expr}) = {arm}.guard"); self.expr(expr); }, - Some(hir::Guard::IfLet(let_expr)) => { - bind!(self, let_expr); - chain!(self, "let Some(Guard::IfLet({let_expr}) = {arm}.guard"); - self.pat(field!(let_expr.pat)); - self.expr(field!(let_expr.init)); - }, } self.expr(field!(arm.body)); } diff --git a/src/tools/clippy/clippy_utils/src/hir_utils.rs b/src/tools/clippy/clippy_utils/src/hir_utils.rs index e610ed930505b..a23105691bf3b 100644 --- a/src/tools/clippy/clippy_utils/src/hir_utils.rs +++ b/src/tools/clippy/clippy_utils/src/hir_utils.rs @@ -8,7 +8,7 @@ use rustc_hir::def::Res; use rustc_hir::MatchSource::TryDesugar; use rustc_hir::{ ArrayLen, BinOpKind, BindingAnnotation, Block, BodyId, Closure, Expr, ExprField, ExprKind, FnRetTy, GenericArg, - GenericArgs, Guard, HirId, HirIdMap, InlineAsmOperand, Let, Lifetime, LifetimeName, Pat, PatField, PatKind, Path, + GenericArgs, HirId, HirIdMap, InlineAsmOperand, Let, Lifetime, LifetimeName, Pat, PatField, PatKind, Path, PathSegment, PrimTy, QPath, Stmt, StmtKind, Ty, TyKind, TypeBinding, }; use rustc_lexer::{tokenize, TokenKind}; @@ -320,7 +320,7 @@ impl HirEqInterExpr<'_, '_, '_> { && self.eq_expr(le, re) && over(la, ra, |l, r| { self.eq_pat(l.pat, r.pat) - && both(&l.guard, &r.guard, |l, r| self.eq_guard(l, r)) + && both(&l.guard, &r.guard, |l, r| self.eq_expr(l, r)) && self.eq_expr(l.body, r.body) }) }, @@ -410,16 +410,6 @@ impl HirEqInterExpr<'_, '_, '_> { left.ident.name == right.ident.name && self.eq_expr(left.expr, right.expr) } - fn eq_guard(&mut self, left: &Guard<'_>, right: &Guard<'_>) -> bool { - match (left, right) { - (Guard::If(l), Guard::If(r)) => self.eq_expr(l, r), - (Guard::IfLet(l), Guard::IfLet(r)) => { - self.eq_pat(l.pat, r.pat) && both(&l.ty, &r.ty, |l, r| self.eq_ty(l, r)) && self.eq_expr(l.init, r.init) - }, - _ => false, - } - } - fn eq_generic_arg(&mut self, left: &GenericArg<'_>, right: &GenericArg<'_>) -> bool { match (left, right) { (GenericArg::Const(l), GenericArg::Const(r)) => self.eq_body(l.value.body, r.value.body), @@ -876,7 +866,7 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> { for arm in arms { self.hash_pat(arm.pat); if let Some(ref e) = arm.guard { - self.hash_guard(e); + self.hash_expr(e); } self.hash_expr(arm.body); } @@ -1056,14 +1046,6 @@ impl<'a, 'tcx> SpanlessHash<'a, 'tcx> { } } - pub fn hash_guard(&mut self, g: &Guard<'_>) { - match g { - Guard::If(expr) | Guard::IfLet(Let { init: expr, .. }) => { - self.hash_expr(expr); - }, - } - } - pub fn hash_lifetime(&mut self, lifetime: &Lifetime) { lifetime.ident.name.hash(&mut self.s); std::mem::discriminant(&lifetime.res).hash(&mut self.s); diff --git a/src/tools/clippy/clippy_utils/src/lib.rs b/src/tools/clippy/clippy_utils/src/lib.rs index 70a3c6f82c1cd..cdf8528f48a27 100644 --- a/src/tools/clippy/clippy_utils/src/lib.rs +++ b/src/tools/clippy/clippy_utils/src/lib.rs @@ -3164,7 +3164,7 @@ pub fn is_never_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> Option< self.is_never = false; if let Some(guard) = arm.guard { let in_final_expr = mem::replace(&mut self.in_final_expr, false); - self.visit_expr(guard.body()); + self.visit_expr(guard); self.in_final_expr = in_final_expr; // The compiler doesn't consider diverging guards as causing the arm to diverge. self.is_never = false; @@ -3223,7 +3223,7 @@ pub fn is_never_expr<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> Option< fn visit_arm(&mut self, arm: &Arm<'tcx>) { if let Some(guard) = arm.guard { let in_final_expr = mem::replace(&mut self.in_final_expr, false); - self.visit_expr(guard.body()); + self.visit_expr(guard); self.in_final_expr = in_final_expr; } self.visit_expr(arm.body); From 6a2bd5acd664e6bd118563ea9033245c624fec2e Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Thu, 4 Jan 2024 01:48:39 +0000 Subject: [PATCH 13/14] Use `resolutions(()).effective_visiblities` to avoid cycle errors --- .../src/traits/error_reporting/mod.rs | 5 +- ...ctive-visibilities-during-object-safety.rs | 28 +++++++ ...e-visibilities-during-object-safety.stderr | 78 +++++++++++++++++++ 3 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.rs create mode 100644 tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.stderr diff --git a/compiler/rustc_infer/src/traits/error_reporting/mod.rs b/compiler/rustc_infer/src/traits/error_reporting/mod.rs index d89c205da3f52..34163111d3c78 100644 --- a/compiler/rustc_infer/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_infer/src/traits/error_reporting/mod.rs @@ -132,7 +132,10 @@ pub fn report_object_safety_error<'tcx>( }; let externally_visible = if !impls.is_empty() && let Some(def_id) = trait_def_id.as_local() - && tcx.effective_visibilities(()).is_exported(def_id) + // We may be executing this during typeck, which would result in cycle + // if we used effective_visibilities query, which looks into opaque types + // (and therefore calls typeck). + && tcx.resolutions(()).effective_visibilities.is_exported(def_id) { true } else { diff --git a/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.rs b/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.rs new file mode 100644 index 0000000000000..650cb3870d581 --- /dev/null +++ b/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.rs @@ -0,0 +1,28 @@ +trait Marker {} +impl Marker for u32 {} + +trait MyTrait { + fn foo(&self) -> impl Marker; +} + +struct Outer; + +impl MyTrait for Outer { + fn foo(&self) -> impl Marker { + 42 + } +} + +impl dyn MyTrait { + //~^ ERROR the trait `MyTrait` cannot be made into an object + fn other(&self) -> impl Marker { + //~^ ERROR the trait `MyTrait` cannot be made into an object + MyTrait::foo(&self) + //~^ ERROR the trait bound `&dyn MyTrait: MyTrait` is not satisfied + //~| ERROR the trait bound `&dyn MyTrait: MyTrait` is not satisfied + //~| ERROR the trait bound `&dyn MyTrait: MyTrait` is not satisfied + //~| ERROR the trait `MyTrait` cannot be made into an object + } +} + +fn main() {} diff --git a/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.stderr b/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.stderr new file mode 100644 index 0000000000000..01de3e531952c --- /dev/null +++ b/tests/ui/impl-trait/in-trait/cycle-effective-visibilities-during-object-safety.stderr @@ -0,0 +1,78 @@ +error[E0277]: the trait bound `&dyn MyTrait: MyTrait` is not satisfied + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:20:22 + | +LL | MyTrait::foo(&self) + | ------------ ^^^^^ the trait `MyTrait` is not implemented for `&dyn MyTrait` + | | + | required by a bound introduced by this call + | + = help: the trait `MyTrait` is implemented for `Outer` + +error[E0038]: the trait `MyTrait` cannot be made into an object + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:20:9 + | +LL | MyTrait::foo(&self) + | ^^^^^^^^^^^^ `MyTrait` cannot be made into an object + | +note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:5:22 + | +LL | trait MyTrait { + | ------- this trait cannot be made into an object... +LL | fn foo(&self) -> impl Marker; + | ^^^^^^^^^^^ ...because method `foo` references an `impl Trait` type in its return type + = help: consider moving `foo` to another trait + = help: only type `Outer` implements the trait, consider using it directly instead + +error[E0277]: the trait bound `&dyn MyTrait: MyTrait` is not satisfied + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:20:9 + | +LL | MyTrait::foo(&self) + | ^^^^^^^^^^^^^^^^^^^ the trait `MyTrait` is not implemented for `&dyn MyTrait` + | + = help: the trait `MyTrait` is implemented for `Outer` + +error[E0277]: the trait bound `&dyn MyTrait: MyTrait` is not satisfied + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:20:9 + | +LL | MyTrait::foo(&self) + | ^^^^^^^^^^^^ the trait `MyTrait` is not implemented for `&dyn MyTrait` + | + = help: the trait `MyTrait` is implemented for `Outer` + +error[E0038]: the trait `MyTrait` cannot be made into an object + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:16:6 + | +LL | impl dyn MyTrait { + | ^^^^^^^^^^^ `MyTrait` cannot be made into an object + | +note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:5:22 + | +LL | trait MyTrait { + | ------- this trait cannot be made into an object... +LL | fn foo(&self) -> impl Marker; + | ^^^^^^^^^^^ ...because method `foo` references an `impl Trait` type in its return type + = help: consider moving `foo` to another trait + = help: only type `Outer` implements the trait, consider using it directly instead + +error[E0038]: the trait `MyTrait` cannot be made into an object + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:18:15 + | +LL | fn other(&self) -> impl Marker { + | ^^^^ `MyTrait` cannot be made into an object + | +note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit + --> $DIR/cycle-effective-visibilities-during-object-safety.rs:5:22 + | +LL | trait MyTrait { + | ------- this trait cannot be made into an object... +LL | fn foo(&self) -> impl Marker; + | ^^^^^^^^^^^ ...because method `foo` references an `impl Trait` type in its return type + = help: consider moving `foo` to another trait + = help: only type `Outer` implements the trait, consider using it directly instead + +error: aborting due to 6 previous errors + +Some errors have detailed explanations: E0038, E0277. +For more information about an error, try `rustc --explain E0038`. From 718a4337acfd2f28682bfaf9179a9e18bf4040b9 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 5 Jan 2024 16:44:42 +0100 Subject: [PATCH 14/14] Document behavior of `!` with MbE --- .../ui/rfcs/rfc-0000-never_patterns/macros.rs | 49 +++++++++++++++++++ .../ui/rfcs/rfc-0000-never_patterns/parse.rs | 5 ++ 2 files changed, 54 insertions(+) create mode 100644 tests/ui/rfcs/rfc-0000-never_patterns/macros.rs diff --git a/tests/ui/rfcs/rfc-0000-never_patterns/macros.rs b/tests/ui/rfcs/rfc-0000-never_patterns/macros.rs new file mode 100644 index 0000000000000..3c04b4517cb7b --- /dev/null +++ b/tests/ui/rfcs/rfc-0000-never_patterns/macros.rs @@ -0,0 +1,49 @@ +// check-pass +// revisions: e2018 e2021 +//[e2018] edition:2018 +//[e2021] edition:2021 +#![feature(never_patterns)] +#![allow(incomplete_features)] + +#[derive(Debug, PartialEq, Eq)] +struct Pattern; +#[derive(Debug, PartialEq, Eq)] +struct Never; +#[derive(Debug, PartialEq, Eq)] +struct Other; + +macro_rules! detect_pat { + ($p:pat) => { + Pattern + }; + (!) => { + Never + }; + ($($x:tt)*) => { + Other + }; +} + +// For backwards-compatibility, all the cases that parse as `Pattern` under the feature gate must +// have been parse errors before. +fn main() { + // For backwards compatibility this does not match `$p:pat`. + assert_eq!(detect_pat!(!), Never); + + // Edition 2018 parses both of these cases as `Other`. Both editions have been parsing the + // first case as `Other` before, so we mustn't change that. + assert_eq!(detect_pat!(! | true), Other); + #[cfg(e2018)] + assert_eq!(detect_pat!(true | !), Other); + #[cfg(e2021)] + assert_eq!(detect_pat!(true | !), Pattern); + + // These are never patterns; they take no body when they're in a match arm. + assert_eq!(detect_pat!((!)), Pattern); + assert_eq!(detect_pat!((true, !)), Pattern); + assert_eq!(detect_pat!(Some(!)), Pattern); + + // These count as normal patterns. + assert_eq!(detect_pat!((! | true)), Pattern); + assert_eq!(detect_pat!((Ok(x) | Err(&!))), Pattern); +} diff --git a/tests/ui/rfcs/rfc-0000-never_patterns/parse.rs b/tests/ui/rfcs/rfc-0000-never_patterns/parse.rs index 1b23e60e0cac7..f254b9c201c1c 100644 --- a/tests/ui/rfcs/rfc-0000-never_patterns/parse.rs +++ b/tests/ui/rfcs/rfc-0000-never_patterns/parse.rs @@ -68,4 +68,9 @@ fn parse(x: Void) { //~^ ERROR top-level or-patterns are not allowed in `let` bindings let (Ok(_) | Err(!)) = &res; let (Ok(_) | Err(&!)) = res.as_ref(); + + let ! = x; + let y @ ! = x; } + +fn foo(!: Void) {}