From b6844489f9dc3ca7aa23c3111a4576099919d65f Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sun, 17 May 2020 18:28:12 -0400 Subject: [PATCH 1/8] Emit a better diagnostic when function actually has a 'self' parameter Fixes #66898 When we are unable to resolve a reference to `self`, we current assume that the containing function doesn't have a `self` parameter, and emit an error message accordingly. However, if the reference to `self` was created by a macro invocation, then resolution will correctly fail, due to hygiene. In this case, we don't want to tell the user that the containing fuction doesn't have a 'self' paramter if it actually has one. This PR checks for the precense of a 'self' parameter, and adjusts the error message we emit accordingly. TODO: The exact error message we emit could probably be improved. Should we explicitly mention hygiene? --- src/librustc_resolve/late.rs | 5 +++-- src/librustc_resolve/late/diagnostics.rs | 11 ++++++++-- src/test/ui/hygiene/missing-self-diag.rs | 23 ++++++++++++++++++++ src/test/ui/hygiene/missing-self-diag.stderr | 17 +++++++++++++++ 4 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 src/test/ui/hygiene/missing-self-diag.rs create mode 100644 src/test/ui/hygiene/missing-self-diag.stderr diff --git a/src/librustc_resolve/late.rs b/src/librustc_resolve/late.rs index e541920e89ed4..477e3be5cc2f8 100644 --- a/src/librustc_resolve/late.rs +++ b/src/librustc_resolve/late.rs @@ -347,7 +347,7 @@ struct DiagnosticMetadata<'ast> { currently_processing_generics: bool, /// The current enclosing function (used for better errors). - current_function: Option, + current_function: Option<(FnKind<'ast>, Span)>, /// A list of labels as of yet unused. Labels will be removed from this map when /// they are used (in a `break` or `continue` statement) @@ -466,7 +466,8 @@ impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> { FnKind::Fn(FnCtxt::Free | FnCtxt::Foreign, ..) => FnItemRibKind, FnKind::Fn(FnCtxt::Assoc(_), ..) | FnKind::Closure(..) => NormalRibKind, }; - let previous_value = replace(&mut self.diagnostic_metadata.current_function, Some(sp)); + let previous_value = + replace(&mut self.diagnostic_metadata.current_function, Some((fn_kind, sp))); debug!("(resolving function) entering function"); let declaration = fn_kind.decl(); diff --git a/src/librustc_resolve/late/diagnostics.rs b/src/librustc_resolve/late/diagnostics.rs index dc92b465c2bc1..b1a1f8725a180 100644 --- a/src/librustc_resolve/late/diagnostics.rs +++ b/src/librustc_resolve/late/diagnostics.rs @@ -195,8 +195,15 @@ impl<'a> LateResolutionVisitor<'a, '_, '_> { _ => "`self` value is a keyword only available in methods with a `self` parameter" .to_string(), }); - if let Some(span) = &self.diagnostic_metadata.current_function { - err.span_label(*span, "this function doesn't have a `self` parameter"); + if let Some((fn_kind, span)) = &self.diagnostic_metadata.current_function { + // The current function has a `self' parameter, but we were unable to resolve + // a reference to `self`. This can only happen if the `self` identifier we + // are resolving came from a different hygiene context. + if fn_kind.decl().inputs.get(0).map(|p| p.is_self()).unwrap_or(false) { + err.span_label(*span, "this function has a `self` parameter, but a macro invocation can only access identifiers it receives from parameters"); + } else { + err.span_label(*span, "this function doesn't have a `self` parameter"); + } } return (err, Vec::new()); } diff --git a/src/test/ui/hygiene/missing-self-diag.rs b/src/test/ui/hygiene/missing-self-diag.rs new file mode 100644 index 0000000000000..f934f793c7f27 --- /dev/null +++ b/src/test/ui/hygiene/missing-self-diag.rs @@ -0,0 +1,23 @@ +// Regression test for issue #66898 +// Tests that we don't emit a nonsensical error message +// when a macro invocation tries to access `self` from a function +// that has a 'self' parameter + +pub struct Foo; + +macro_rules! call_bar { + () => { + self.bar(); //~ ERROR expected value + } +} + +impl Foo { + pub fn foo(&self) { + call_bar!(); + } + + pub fn bar(&self) { + } +} + +fn main() {} diff --git a/src/test/ui/hygiene/missing-self-diag.stderr b/src/test/ui/hygiene/missing-self-diag.stderr new file mode 100644 index 0000000000000..075d6b76bb7b2 --- /dev/null +++ b/src/test/ui/hygiene/missing-self-diag.stderr @@ -0,0 +1,17 @@ +error[E0424]: expected value, found module `self` + --> $DIR/missing-self-diag.rs:10:9 + | +LL | self.bar(); + | ^^^^ `self` value is a keyword only available in methods with a `self` parameter +... +LL | / pub fn foo(&self) { +LL | | call_bar!(); + | | ------------ in this macro invocation +LL | | } + | |_____- this function has a `self` parameter, but a macro invocation can only access identifiers it receives from parameters + | + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0424`. From 038523963ad884349edca1689e5b11ad4405d3ba Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Tue, 12 May 2020 22:18:55 +0200 Subject: [PATCH 2/8] exhaustively match during structural match checking --- .../hair/pattern/const_to_pat.rs | 5 ++- .../traits/structural_match.rs | 35 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir_build/hair/pattern/const_to_pat.rs b/src/librustc_mir_build/hair/pattern/const_to_pat.rs index 28ec2ca13d5af..67e24a1333a32 100644 --- a/src/librustc_mir_build/hair/pattern/const_to_pat.rs +++ b/src/librustc_mir_build/hair/pattern/const_to_pat.rs @@ -125,7 +125,10 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> { "trait objects cannot be used in patterns".to_string() } traits::NonStructuralMatchTy::Param => { - bug!("use of constant whose type is a parameter inside a pattern") + bug!("use of a constant whose type is a parameter inside a pattern") + } + traits::NonStructuralMatchTy::Foreign => { + bug!("use of a value of a foreign type inside a pattern") } }; diff --git a/src/librustc_trait_selection/traits/structural_match.rs b/src/librustc_trait_selection/traits/structural_match.rs index eb63505b69b41..9bd1334bc8e83 100644 --- a/src/librustc_trait_selection/traits/structural_match.rs +++ b/src/librustc_trait_selection/traits/structural_match.rs @@ -13,6 +13,7 @@ pub enum NonStructuralMatchTy<'tcx> { Adt(&'tcx AdtDef), Param, Dynamic, + Foreign, } /// This method traverses the structure of `ty`, trying to find an @@ -143,6 +144,10 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { self.found = Some(NonStructuralMatchTy::Dynamic); return true; // Stop visiting. } + ty::Foreign(_) => { + self.found = Some(NonStructuralMatchTy::Foreign); + return true; // Stop visiting + } ty::RawPtr(..) => { // structural-match ignores substructure of // `*const _`/`*mut _`, so skip `super_visit_with`. @@ -163,7 +168,7 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { return false; } ty::FnDef(..) | ty::FnPtr(..) => { - // types of formals and return in `fn(_) -> _` are also irrelevant; + // Types of formals and return in `fn(_) -> _` are also irrelevant; // so we do not recur into them via `super_visit_with` // // (But still tell caller to continue search.) @@ -176,7 +181,33 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { // for empty array. return false; } - _ => { + ty::Bool + | ty::Char + | ty::Int(_) + | ty::Uint(_) + | ty::Float(_) + | ty::Str + | ty::Never + | ty::Error => { + // These primitive types are always structural match. + // + // `Never` is kind of special here, but as it is not inhabitable, this should be fine. + return false; + } + + ty::Array(..) + | ty::Slice(_) + | ty::Ref(..) + | ty::Closure(..) + | ty::Generator(..) + | ty::GeneratorWitness(..) + | ty::Tuple(..) + | ty::Projection(..) + | ty::UnnormalizedProjection(..) + | ty::Opaque(..) + | ty::Bound(..) + | ty::Placeholder(_) + | ty::Infer(_) => { ty.super_visit_with(self); return false; } From ecab35b45a0d6d33beecbd11d7be50f2511cc4f0 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Tue, 12 May 2020 23:35:29 +0200 Subject: [PATCH 3/8] note for `ty::Error`. --- .../traits/structural_match.rs | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/librustc_trait_selection/traits/structural_match.rs b/src/librustc_trait_selection/traits/structural_match.rs index 9bd1334bc8e83..da207cf7d3804 100644 --- a/src/librustc_trait_selection/traits/structural_match.rs +++ b/src/librustc_trait_selection/traits/structural_match.rs @@ -187,8 +187,7 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { | ty::Uint(_) | ty::Float(_) | ty::Str - | ty::Never - | ty::Error => { + | ty::Never => { // These primitive types are always structural match. // // `Never` is kind of special here, but as it is not inhabitable, this should be fine. @@ -200,17 +199,25 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { | ty::Ref(..) | ty::Closure(..) | ty::Generator(..) - | ty::GeneratorWitness(..) | ty::Tuple(..) | ty::Projection(..) - | ty::UnnormalizedProjection(..) | ty::Opaque(..) - | ty::Bound(..) - | ty::Placeholder(_) - | ty::Infer(_) => { + | ty::GeneratorWitness(..) => { ty.super_visit_with(self); return false; } + | ty::Infer(_) + | ty::Placeholder(_) + | ty::UnnormalizedProjection(..) + | ty::Bound(..) => { + bug!("unexpected type during structural-match checking: {:?}", ty); + } + ty::Error => { + self.tcx().delay_span_bug(self.span, "ty::Error in structural-match check"); + // We still want to check other types after encountering an error, + // as this may still emit relevant errors. + return false; + } }; if !self.seen.insert(adt_def.did) { From 5a5017ec634cef0ff11b40dd08fdd4572d605d01 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Wed, 13 May 2020 00:40:28 +0200 Subject: [PATCH 4/8] Be more conservative concerning `structural_match` --- .../hair/pattern/const_to_pat.rs | 9 +++++ .../traits/structural_match.rs | 40 +++++++++---------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/librustc_mir_build/hair/pattern/const_to_pat.rs b/src/librustc_mir_build/hair/pattern/const_to_pat.rs index 67e24a1333a32..9e3f75fdc078c 100644 --- a/src/librustc_mir_build/hair/pattern/const_to_pat.rs +++ b/src/librustc_mir_build/hair/pattern/const_to_pat.rs @@ -124,9 +124,18 @@ impl<'a, 'tcx> ConstToPat<'a, 'tcx> { traits::NonStructuralMatchTy::Dynamic => { "trait objects cannot be used in patterns".to_string() } + traits::NonStructuralMatchTy::Opaque => { + "opaque types cannot be used in patterns".to_string() + } + traits::NonStructuralMatchTy::Generator => { + "generators cannot be used in patterns".to_string() + } traits::NonStructuralMatchTy::Param => { bug!("use of a constant whose type is a parameter inside a pattern") } + traits::NonStructuralMatchTy::Projection => { + bug!("use of a constant whose type is a projection inside a pattern") + } traits::NonStructuralMatchTy::Foreign => { bug!("use of a value of a foreign type inside a pattern") } diff --git a/src/librustc_trait_selection/traits/structural_match.rs b/src/librustc_trait_selection/traits/structural_match.rs index da207cf7d3804..71fa46ccdedb7 100644 --- a/src/librustc_trait_selection/traits/structural_match.rs +++ b/src/librustc_trait_selection/traits/structural_match.rs @@ -14,6 +14,9 @@ pub enum NonStructuralMatchTy<'tcx> { Param, Dynamic, Foreign, + Opaque, + Generator, + Projection, } /// This method traverses the structure of `ty`, trying to find an @@ -148,6 +151,18 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { self.found = Some(NonStructuralMatchTy::Foreign); return true; // Stop visiting } + ty::Opaque(..) => { + self.found = Some(NonStructuralMatchTy::Opaque); + return true; + } + ty::Projection(..) => { + self.found = Some(NonStructuralMatchTy::Projection); + return true; + } + ty::Generator(..) | ty::GeneratorWitness(..) => { + self.found = Some(NonStructuralMatchTy::Generator); + return true; + } ty::RawPtr(..) => { // structural-match ignores substructure of // `*const _`/`*mut _`, so skip `super_visit_with`. @@ -181,39 +196,22 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { // for empty array. return false; } - ty::Bool - | ty::Char - | ty::Int(_) - | ty::Uint(_) - | ty::Float(_) - | ty::Str - | ty::Never => { + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str | ty::Never => { // These primitive types are always structural match. // // `Never` is kind of special here, but as it is not inhabitable, this should be fine. return false; } - ty::Array(..) - | ty::Slice(_) - | ty::Ref(..) - | ty::Closure(..) - | ty::Generator(..) - | ty::Tuple(..) - | ty::Projection(..) - | ty::Opaque(..) - | ty::GeneratorWitness(..) => { + ty::Array(..) | ty::Slice(_) | ty::Ref(..) | ty::Tuple(..) => { ty.super_visit_with(self); return false; } - | ty::Infer(_) - | ty::Placeholder(_) - | ty::UnnormalizedProjection(..) - | ty::Bound(..) => { + ty::Closure(..) | ty::Infer(_) | ty::Placeholder(_) | ty::Bound(..) => { bug!("unexpected type during structural-match checking: {:?}", ty); } ty::Error => { - self.tcx().delay_span_bug(self.span, "ty::Error in structural-match check"); + self.tcx().sess.delay_span_bug(self.span, "ty::Error in structural-match check"); // We still want to check other types after encountering an error, // as this may still emit relevant errors. return false; From ea47fdf78538e615eb7824ff5c194b6af4565b7f Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Wed, 13 May 2020 11:52:22 +0200 Subject: [PATCH 5/8] comment return sites --- .../traits/structural_match.rs | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/src/librustc_trait_selection/traits/structural_match.rs b/src/librustc_trait_selection/traits/structural_match.rs index 71fa46ccdedb7..b877049fcf667 100644 --- a/src/librustc_trait_selection/traits/structural_match.rs +++ b/src/librustc_trait_selection/traits/structural_match.rs @@ -149,19 +149,19 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { } ty::Foreign(_) => { self.found = Some(NonStructuralMatchTy::Foreign); - return true; // Stop visiting + return true; // Stop visiting. } ty::Opaque(..) => { self.found = Some(NonStructuralMatchTy::Opaque); - return true; + return true; // Stop visiting. } ty::Projection(..) => { self.found = Some(NonStructuralMatchTy::Projection); - return true; + return true; // Stop visiting. } ty::Generator(..) | ty::GeneratorWitness(..) => { self.found = Some(NonStructuralMatchTy::Generator); - return true; + return true; // Stop visiting. } ty::RawPtr(..) => { // structural-match ignores substructure of @@ -179,14 +179,14 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { // structural equality on `T` does not recur into the raw // pointer. Therefore, one can still use `C` in a pattern. - // (But still tell caller to continue search.) + // (But still tell the caller to continue search.) return false; } ty::FnDef(..) | ty::FnPtr(..) => { // Types of formals and return in `fn(_) -> _` are also irrelevant; // so we do not recur into them via `super_visit_with` // - // (But still tell caller to continue search.) + // (But still tell the caller to continue search.) return false; } ty::Array(_, n) @@ -194,16 +194,21 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { { // rust-lang/rust#62336: ignore type of contents // for empty array. + // + // (But still tell the caller to continue search.) return false; } ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str | ty::Never => { // These primitive types are always structural match. // // `Never` is kind of special here, but as it is not inhabitable, this should be fine. + // + // (But still tell the caller to continue search.) return false; } ty::Array(..) | ty::Slice(_) | ty::Ref(..) | ty::Tuple(..) => { + // First check all contained types and then tell the caller to continue searching. ty.super_visit_with(self); return false; } @@ -214,13 +219,15 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { self.tcx().sess.delay_span_bug(self.span, "ty::Error in structural-match check"); // We still want to check other types after encountering an error, // as this may still emit relevant errors. + // + // So we continue searching here. return false; } }; if !self.seen.insert(adt_def.did) { debug!("Search already seen adt_def: {:?}", adt_def); - // let caller continue its search + // Let caller continue its search. return false; } From a5a4ec98e20e4b863cfb2f6c44faa15824fce9f3 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Wed, 13 May 2020 13:14:53 +0200 Subject: [PATCH 6/8] Add tests for opaque types --- .../structural-match-no-leak.rs | 20 ++++++++++++++++++ .../structural-match-no-leak.stderr | 8 +++++++ .../type-alias-impl-trait/structural-match.rs | 21 +++++++++++++++++++ .../structural-match.stderr | 8 +++++++ 4 files changed, 57 insertions(+) create mode 100644 src/test/ui/type-alias-impl-trait/structural-match-no-leak.rs create mode 100644 src/test/ui/type-alias-impl-trait/structural-match-no-leak.stderr create mode 100644 src/test/ui/type-alias-impl-trait/structural-match.rs create mode 100644 src/test/ui/type-alias-impl-trait/structural-match.stderr diff --git a/src/test/ui/type-alias-impl-trait/structural-match-no-leak.rs b/src/test/ui/type-alias-impl-trait/structural-match-no-leak.rs new file mode 100644 index 0000000000000..479d6cd9af765 --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/structural-match-no-leak.rs @@ -0,0 +1,20 @@ +#![feature(const_fn, type_alias_impl_trait)] + +type Bar = impl Send; + +// While i32 is structural-match, we do not want to leak this information. +// (See https://github.com/rust-lang/rust/issues/72156) +const fn leak_free() -> Bar { + 7i32 +} +const LEAK_FREE: Bar = leak_free(); + +fn leak_free_test() { + match todo!() { + LEAK_FREE => (), + //~^ opaque types cannot be used in patterns + _ => (), + } +} + +fn main() { } diff --git a/src/test/ui/type-alias-impl-trait/structural-match-no-leak.stderr b/src/test/ui/type-alias-impl-trait/structural-match-no-leak.stderr new file mode 100644 index 0000000000000..ae0d8e8d4239c --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/structural-match-no-leak.stderr @@ -0,0 +1,8 @@ +error: opaque types cannot be used in patterns + --> $DIR/structural-match-no-leak.rs:14:9 + | +LL | LEAK_FREE => (), + | ^^^^^^^^^ + +error: aborting due to previous error + diff --git a/src/test/ui/type-alias-impl-trait/structural-match.rs b/src/test/ui/type-alias-impl-trait/structural-match.rs new file mode 100644 index 0000000000000..481448d64b1aa --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/structural-match.rs @@ -0,0 +1,21 @@ +#![feature(const_fn, type_alias_impl_trait)] + +type Foo = impl Send; + +// This is not structural-match +struct A; + +const fn value() -> Foo { + A +} +const VALUE: Foo = value(); + +fn test() { + match todo!() { + VALUE => (), + //~^ opaque types cannot be used in patterns + _ => (), + } +} + +fn main() { } diff --git a/src/test/ui/type-alias-impl-trait/structural-match.stderr b/src/test/ui/type-alias-impl-trait/structural-match.stderr new file mode 100644 index 0000000000000..ad9036a87d1d9 --- /dev/null +++ b/src/test/ui/type-alias-impl-trait/structural-match.stderr @@ -0,0 +1,8 @@ +error: opaque types cannot be used in patterns + --> $DIR/structural-match.rs:15:9 + | +LL | VALUE => (), + | ^^^^^ + +error: aborting due to previous error + From 3fcb0f4bfaea1de5fa86a464a7a9d6a0fb30cbca Mon Sep 17 00:00:00 2001 From: Yuki Okushi Date: Mon, 25 May 2020 15:07:55 +0900 Subject: [PATCH 7/8] Enable `glacier` command via triagebot --- triagebot.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/triagebot.toml b/triagebot.toml index f12d516376322..e43cff5538659 100644 --- a/triagebot.toml +++ b/triagebot.toml @@ -10,6 +10,8 @@ allow-unauthenticated = [ [assign] +[glacier] + [ping.icebreakers-llvm] alias = ["llvm", "llvms"] message = """\ From 632f3de12bc71167485253510b2e1644b25302ce Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Mon, 25 May 2020 13:03:38 +0200 Subject: [PATCH 8/8] Clean up E0608 explanation --- src/librustc_error_codes/error_codes/E0608.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_error_codes/error_codes/E0608.md b/src/librustc_error_codes/error_codes/E0608.md index 598f1e655e964..d0ebc3a26f082 100644 --- a/src/librustc_error_codes/error_codes/E0608.md +++ b/src/librustc_error_codes/error_codes/E0608.md @@ -1,4 +1,4 @@ -An attempt to index into a type which doesn't implement the `std::ops::Index` +An attempt to use index on a type which doesn't implement the `std::ops::Index` trait was performed. Erroneous code example: