From 1ee095fe02947b0592bdb1357c2df98832ba0b6a Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 28 Feb 2020 05:08:18 +0100 Subject: [PATCH 1/2] make `.stash(..)` work in macros without ICEing --- src/librustc_errors/diagnostic_builder.rs | 23 +++++++-- src/librustc_parse/parser/item.rs | 4 +- .../issue-69396-const-no-type-in-macro.rs | 17 +++++++ .../issue-69396-const-no-type-in-macro.stderr | 49 +++++++++++++++++++ 4 files changed, 87 insertions(+), 6 deletions(-) create mode 100644 src/test/ui/issues/issue-69396-const-no-type-in-macro.rs create mode 100644 src/test/ui/issues/issue-69396-const-no-type-in-macro.stderr diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 39f585231eea4..6b9de8df45072 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -115,13 +115,28 @@ impl<'a> DiagnosticBuilder<'a> { /// Stashes diagnostic for possible later improvement in a different, /// later stage of the compiler. The diagnostic can be accessed with - /// the provided `span` and `key` through `.steal_diagnostic` on `Handler`. + /// the *returned* `span` and `key` through `.steal_diagnostic` on `Handler`. + /// Do not use the `span` passed into this function when calling `.steal_diagnostic`. /// /// As with `buffer`, this is unless the handler has disabled such buffering. - pub fn stash(self, span: Span, key: StashKey) { - if let Some((diag, handler)) = self.into_diagnostic() { + #[must_use = "`Span` returned by `.stash(...)` must be later used in `.steal_diagnostic(..)`"] + pub fn stash(self, span: Span, key: StashKey) -> Span { + self.into_diagnostic().map_or(span, |(diag, handler)| { + // Before stashing the diagnostic, make sure the `span` is unique so that + // we do not attempt overwriting the key's slot later when dealing with macros. + // If we don't do this, then something like the following...: + // ``` + // macro_rules! suite { + // ( $( $fn:ident; )* ) => { $(const A = "A".$fn();)* } + // } + // + // suite! { len; is_empty; } + // ``` + // ...would result in overwriting due to using the same `def_site` span for `A`. + let span = span.fresh_expansion(span.ctxt().outer_expn_data()); handler.stash_diagnostic(span, key, diag); - } + span + }) } /// Converts the builder to a `Diagnostic` for later emission, diff --git a/src/librustc_parse/parser/item.rs b/src/librustc_parse/parser/item.rs index d7d6fcd05b795..d2eb7dba63783 100644 --- a/src/librustc_parse/parser/item.rs +++ b/src/librustc_parse/parser/item.rs @@ -936,11 +936,11 @@ impl<'a> Parser<'a> { format!("{}: ", id), Applicability::HasPlaceholders, ); - err.stash(id.span, StashKey::ItemNoType); + let span = err.stash(id.span, StashKey::ItemNoType); // The user intended that the type be inferred, // so treat this as if the user wrote e.g. `const A: _ = expr;`. - P(Ty { kind: TyKind::Infer, span: id.span, id: ast::DUMMY_NODE_ID }) + P(Ty { kind: TyKind::Infer, span, id: ast::DUMMY_NODE_ID }) } /// Parses an enum declaration. diff --git a/src/test/ui/issues/issue-69396-const-no-type-in-macro.rs b/src/test/ui/issues/issue-69396-const-no-type-in-macro.rs new file mode 100644 index 0000000000000..bb2fae8f7bdb1 --- /dev/null +++ b/src/test/ui/issues/issue-69396-const-no-type-in-macro.rs @@ -0,0 +1,17 @@ +macro_rules! suite { + ( $( $fn:ident; )* ) => { + $( + const A = "A".$fn(); + //~^ ERROR the name `A` is defined multiple times + //~| ERROR missing type for `const` item + //~| ERROR missing type for `const` item + )* + } +} + +suite! { + len; + is_empty; +} + +fn main() {} diff --git a/src/test/ui/issues/issue-69396-const-no-type-in-macro.stderr b/src/test/ui/issues/issue-69396-const-no-type-in-macro.stderr new file mode 100644 index 0000000000000..02b94565c2ac8 --- /dev/null +++ b/src/test/ui/issues/issue-69396-const-no-type-in-macro.stderr @@ -0,0 +1,49 @@ +error[E0428]: the name `A` is defined multiple times + --> $DIR/issue-69396-const-no-type-in-macro.rs:4:13 + | +LL | const A = "A".$fn(); + | ^^^^^^^^^^^^^^^^^^^^ + | | + | `A` redefined here + | previous definition of the value `A` here +... +LL | / suite! { +LL | | len; +LL | | is_empty; +LL | | } + | |_- in this macro invocation + | + = note: `A` must be defined only once in the value namespace of this module + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: missing type for `const` item + --> $DIR/issue-69396-const-no-type-in-macro.rs:4:19 + | +LL | const A = "A".$fn(); + | ^ help: provide a type for the item: `A: usize` +... +LL | / suite! { +LL | | len; +LL | | is_empty; +LL | | } + | |_- in this macro invocation + | + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: missing type for `const` item + --> $DIR/issue-69396-const-no-type-in-macro.rs:4:19 + | +LL | const A = "A".$fn(); + | ^ help: provide a type for the item: `A: bool` +... +LL | / suite! { +LL | | len; +LL | | is_empty; +LL | | } + | |_- in this macro invocation + | + = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0428`. From 461d798bff50a9d7660ebbf962615f5636aeb2a5 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Fri, 28 Feb 2020 06:45:34 +0100 Subject: [PATCH 2/2] stashing: tweak `ExpnData` based on eddyb's feedback. --- src/librustc/lint.rs | 3 ++- src/librustc_errors/diagnostic_builder.rs | 5 +++-- src/librustc_errors/emitter.rs | 4 +++- src/librustc_save_analysis/lib.rs | 5 ++++- src/librustc_span/hygiene.rs | 3 +++ 5 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/librustc/lint.rs b/src/librustc/lint.rs index 004835b230ab4..0cdbba64daffe 100644 --- a/src/librustc/lint.rs +++ b/src/librustc/lint.rs @@ -342,7 +342,8 @@ pub fn in_external_macro(sess: &Session, span: Span) -> bool { let expn_data = span.ctxt().outer_expn_data(); match expn_data.kind { ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop) => false, - ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) => true, // well, it's "external" + // well, it's "external" + ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) | ExpnKind::ParserRecovery => true, ExpnKind::Macro(MacroKind::Bang, _) => { if expn_data.def_site.is_dummy() { // Dummy span for the `def_site` means it's an external macro. diff --git a/src/librustc_errors/diagnostic_builder.rs b/src/librustc_errors/diagnostic_builder.rs index 6b9de8df45072..2f19b69e7eee2 100644 --- a/src/librustc_errors/diagnostic_builder.rs +++ b/src/librustc_errors/diagnostic_builder.rs @@ -2,7 +2,7 @@ use crate::{Applicability, Handler, Level, StashKey}; use crate::{Diagnostic, DiagnosticId, DiagnosticStyledString}; use log::debug; -use rustc_span::{MultiSpan, Span}; +use rustc_span::{ExpnData, ExpnKind, MultiSpan, Span}; use std::fmt::{self, Debug}; use std::ops::{Deref, DerefMut}; use std::thread::panicking; @@ -133,7 +133,8 @@ impl<'a> DiagnosticBuilder<'a> { // suite! { len; is_empty; } // ``` // ...would result in overwriting due to using the same `def_site` span for `A`. - let span = span.fresh_expansion(span.ctxt().outer_expn_data()); + let data = ExpnData::default(ExpnKind::ParserRecovery, span, span.edition()); + let span = span.fresh_expansion(data); handler.stash_diagnostic(span, key, diag); span }) diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs index f3653da4be666..a3340c5fb6bc0 100644 --- a/src/librustc_errors/emitter.rs +++ b/src/librustc_errors/emitter.rs @@ -293,7 +293,9 @@ pub trait Emitter { // Skip past non-macro entries, just in case there // are some which do actually involve macros. - ExpnKind::Desugaring(..) | ExpnKind::AstPass(..) => None, + ExpnKind::Desugaring(..) + | ExpnKind::AstPass(..) + | ExpnKind::ParserRecovery => None, ExpnKind::Macro(macro_kind, _) => Some(macro_kind), } diff --git a/src/librustc_save_analysis/lib.rs b/src/librustc_save_analysis/lib.rs index b253559dd5cd5..f81aaac37ae96 100644 --- a/src/librustc_save_analysis/lib.rs +++ b/src/librustc_save_analysis/lib.rs @@ -789,7 +789,10 @@ impl<'l, 'tcx> SaveContext<'l, 'tcx> { // These are not macros. // FIXME(eddyb) maybe there is a way to handle them usefully? - ExpnKind::Root | ExpnKind::AstPass(_) | ExpnKind::Desugaring(_) => return None, + ExpnKind::Root + | ExpnKind::AstPass(_) + | ExpnKind::Desugaring(_) + | ExpnKind::ParserRecovery => return None, }; // If the callee is an imported macro from an external crate, need to get diff --git a/src/librustc_span/hygiene.rs b/src/librustc_span/hygiene.rs index a368a881674d8..806321c475f35 100644 --- a/src/librustc_span/hygiene.rs +++ b/src/librustc_span/hygiene.rs @@ -729,6 +729,8 @@ pub enum ExpnKind { AstPass(AstPass), /// Desugaring done by the compiler during HIR lowering. Desugaring(DesugaringKind), + /// AST fragements produced by parser recovery. + ParserRecovery, } impl ExpnKind { @@ -742,6 +744,7 @@ impl ExpnKind { }, ExpnKind::AstPass(kind) => kind.descr().to_string(), ExpnKind::Desugaring(kind) => format!("desugaring of {}", kind.descr()), + ExpnKind::ParserRecovery => "parser recovery".to_string(), } } }