Skip to content

Commit

Permalink
expand: Remove ParseSess::missing_fragment_specifiers
Browse files Browse the repository at this point in the history
It was used for deduplicating some errors for legacy code which are mostly deduplicated even without that, but at cost of global mutable state, which is not a good tradeoff.
  • Loading branch information
petrochenkov committed Apr 9, 2022
1 parent 399dd80 commit 379ae12
Show file tree
Hide file tree
Showing 13 changed files with 137 additions and 47 deletions.
15 changes: 13 additions & 2 deletions compiler/rustc_expand/src/mbe/macro_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ use rustc_ast::token::{DelimToken, Token, TokenKind};
use rustc_ast::{NodeId, DUMMY_NODE_ID};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::MultiSpan;
use rustc_session::lint::builtin::META_VARIABLE_MISUSE;
use rustc_session::lint::builtin::{META_VARIABLE_MISUSE, MISSING_FRAGMENT_SPECIFIER};
use rustc_session::parse::ParseSess;
use rustc_span::symbol::kw;
use rustc_span::{symbol::MacroRulesNormalizedIdent, Span};
Expand Down Expand Up @@ -261,7 +261,18 @@ fn check_binders(
}
}
// Similarly, this can only happen when checking a toplevel macro.
TokenTree::MetaVarDecl(span, name, _kind) => {
TokenTree::MetaVarDecl(span, name, kind) => {
if kind.is_none() && node_id != DUMMY_NODE_ID {
// FIXME: Report this as a hard error eventually and remove equivalent errors from
// `parse_tt_inner` and `nameize`. Until then the error may be reported twice, once
// as a hard error and then once as a buffered lint.
sess.buffer_lint(
MISSING_FRAGMENT_SPECIFIER,
span,
node_id,
&format!("missing fragment specifier"),
);
}
if !macros.is_empty() {
sess.span_diagnostic.span_bug(span, "unexpected MetaVarDecl in nested lhs");
}
Expand Down
18 changes: 6 additions & 12 deletions compiler/rustc_expand/src/mbe/macro_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,6 @@ impl TtParser {
/// track of through the mps generated.
fn parse_tt_inner(
&mut self,
sess: &ParseSess,
matcher: &[MatcherLoc],
token: &Token,
) -> Option<NamedParseResult> {
Expand Down Expand Up @@ -519,11 +518,9 @@ impl TtParser {
self.bb_mps.push(mp);
}
} else {
// E.g. `$e` instead of `$e:expr`, reported as a hard error if actually used.
// Both this check and the one in `nameize` are necessary, surprisingly.
if sess.missing_fragment_specifiers.borrow_mut().remove(&span).is_some() {
// E.g. `$e` instead of `$e:expr`.
return Some(Error(span, "missing fragment specifier".to_string()));
}
return Some(Error(span, "missing fragment specifier".to_string()));
}
}
MatcherLoc::Eof => {
Expand All @@ -549,7 +546,7 @@ impl TtParser {
// Need to take ownership of the matches from within the `Lrc`.
Lrc::make_mut(&mut eof_mp.matches);
let matches = Lrc::try_unwrap(eof_mp.matches).unwrap().into_iter();
self.nameize(sess, matcher, matches)
self.nameize(matcher, matches)
}
EofMatcherPositions::Multiple => {
Error(token.span, "ambiguity: multiple successful parses".to_string())
Expand Down Expand Up @@ -587,7 +584,7 @@ impl TtParser {

// Process `cur_mps` until either we have finished the input or we need to get some
// parsing from the black-box parser done.
if let Some(res) = self.parse_tt_inner(&parser.sess, matcher, &parser.token) {
if let Some(res) = self.parse_tt_inner(matcher, &parser.token) {
return res;
}

Expand Down Expand Up @@ -694,7 +691,6 @@ impl TtParser {

fn nameize<I: Iterator<Item = NamedMatch>>(
&self,
sess: &ParseSess,
matcher: &[MatcherLoc],
mut res: I,
) -> NamedParseResult {
Expand All @@ -711,11 +707,9 @@ impl TtParser {
}
};
} else {
// E.g. `$e` instead of `$e:expr`, reported as a hard error if actually used.
// Both this check and the one in `parse_tt_inner` are necessary, surprisingly.
if sess.missing_fragment_specifiers.borrow_mut().remove(&span).is_some() {
// E.g. `$e` instead of `$e:expr`.
return Error(span, "missing fragment specifier".to_string());
}
return Error(span, "missing fragment specifier".to_string());
}
}
}
Expand Down
8 changes: 2 additions & 6 deletions compiler/rustc_expand/src/mbe/quoted.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ use crate::mbe::macro_parser::count_metavar_decls;
use crate::mbe::{Delimited, KleeneOp, KleeneToken, MetaVarExpr, SequenceRepetition, TokenTree};

use rustc_ast::token::{self, Token};
use rustc_ast::tokenstream;
use rustc_ast::{NodeId, DUMMY_NODE_ID};
use rustc_ast::{tokenstream, NodeId};
use rustc_ast_pretty::pprust;
use rustc_feature::Features;
use rustc_session::parse::{feature_err, ParseSess};
Expand Down Expand Up @@ -104,10 +103,7 @@ pub(super) fn parse(
}
tree => tree.as_ref().map_or(start_sp, tokenstream::TokenTree::span),
};
if node_id != DUMMY_NODE_ID {
// Macros loaded from other crates have dummy node ids.
sess.missing_fragment_specifiers.borrow_mut().insert(span, node_id);
}

result.push(TokenTree::MetaVarDecl(span, ident, None));
}

Expand Down
16 changes: 0 additions & 16 deletions compiler/rustc_interface/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ use rustc_resolve::{Resolver, ResolverArenas};
use rustc_serialize::json;
use rustc_session::config::{CrateType, Input, OutputFilenames, OutputType};
use rustc_session::cstore::{MetadataLoader, MetadataLoaderDyn};
use rustc_session::lint;
use rustc_session::output::{filename_for_input, filename_for_metadata};
use rustc_session::search_paths::PathKind;
use rustc_session::{Limit, Session};
Expand Down Expand Up @@ -349,23 +348,8 @@ pub fn configure_and_expand(
ecx.check_unused_macros();
});

let mut missing_fragment_specifiers: Vec<_> = ecx
.sess
.parse_sess
.missing_fragment_specifiers
.borrow()
.iter()
.map(|(span, node_id)| (*span, *node_id))
.collect();
missing_fragment_specifiers.sort_unstable_by_key(|(span, _)| *span);

let recursion_limit_hit = ecx.reduced_recursion_limit.is_some();

for (span, node_id) in missing_fragment_specifiers {
let lint = lint::builtin::MISSING_FRAGMENT_SPECIFIER;
let msg = "missing fragment specifier";
resolver.lint_buffer().buffer_lint(lint, node_id, span, msg);
}
if cfg!(windows) {
env::set_var("PATH", &old_path);
}
Expand Down
2 changes: 0 additions & 2 deletions compiler/rustc_session/src/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ pub struct ParseSess {
pub config: CrateConfig,
pub check_config: CrateCheckConfig,
pub edition: Edition,
pub missing_fragment_specifiers: Lock<FxHashMap<Span, NodeId>>,
/// Places where raw identifiers were used. This is used to avoid complaining about idents
/// clashing with keywords in new editions.
pub raw_identifier_spans: Lock<Vec<Span>>,
Expand Down Expand Up @@ -195,7 +194,6 @@ impl ParseSess {
config: FxHashSet::default(),
check_config: CrateCheckConfig::default(),
edition: ExpnId::root().expn_data().edition,
missing_fragment_specifiers: Default::default(),
raw_identifier_spans: Lock::new(Vec::new()),
bad_unicode_identifiers: Lock::new(Default::default()),
source_map,
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/macros/macro-match-nonterminal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ macro_rules! test {
($a, $b) => {
//~^ ERROR missing fragment
//~| ERROR missing fragment
//~| ERROR missing fragment
//~| WARN this was previously accepted
//~| WARN this was previously accepted
()
};
Expand Down
13 changes: 11 additions & 2 deletions src/test/ui/macros/macro-match-nonterminal.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,24 @@ error: missing fragment specifier
LL | ($a, $b) => {
| ^

error: missing fragment specifier
--> $DIR/macro-match-nonterminal.rs:2:8
|
LL | ($a, $b) => {
| ^
|
= note: `#[deny(missing_fragment_specifier)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

error: missing fragment specifier
--> $DIR/macro-match-nonterminal.rs:2:10
|
LL | ($a, $b) => {
| ^^
|
= note: `#[deny(missing_fragment_specifier)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

15 changes: 15 additions & 0 deletions src/test/ui/macros/macro-missing-fragment-deduplication.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// compile-flags: -Zdeduplicate-diagnostics=yes

macro_rules! m {
($name) => {}
//~^ ERROR missing fragment
//~| ERROR missing fragment
//~| WARN this was previously accepted
}

fn main() {
m!();
m!();
m!();
m!();
}
18 changes: 18 additions & 0 deletions src/test/ui/macros/macro-missing-fragment-deduplication.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error: missing fragment specifier
--> $DIR/macro-missing-fragment-deduplication.rs:4:6
|
LL | ($name) => {}
| ^^^^^

error: missing fragment specifier
--> $DIR/macro-missing-fragment-deduplication.rs:4:6
|
LL | ($name) => {}
| ^^^^^
|
= note: `#[deny(missing_fragment_specifier)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

error: aborting due to 2 previous errors

25 changes: 22 additions & 3 deletions src/test/ui/macros/macro-missing-fragment.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,26 @@
macro_rules! m {
( $( any_token $field_rust_type )* ) => {}; //~ ERROR missing fragment
#![warn(missing_fragment_specifier)]

macro_rules! used_arm {
( $( any_token $field_rust_type )* ) => {};
//~^ ERROR missing fragment
//~| WARN missing fragment
//~| WARN this was previously accepted
}

macro_rules! used_macro_unused_arm {
() => {};
( $name ) => {};
//~^ WARN missing fragment
//~| WARN this was previously accepted
}

macro_rules! unused_macro {
( $name ) => {};
//~^ WARN missing fragment
//~| WARN this was previously accepted
}

fn main() {
m!();
used_arm!();
used_macro_unused_arm!();
}
36 changes: 34 additions & 2 deletions src/test/ui/macros/macro-missing-fragment.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,40 @@
error: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:2:20
--> $DIR/macro-missing-fragment.rs:4:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^

error: aborting due to previous error
warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:4:20
|
LL | ( $( any_token $field_rust_type )* ) => {};
| ^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> $DIR/macro-missing-fragment.rs:1:9
|
LL | #![warn(missing_fragment_specifier)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:12:7
|
LL | ( $name ) => {};
| ^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

warning: missing fragment specifier
--> $DIR/macro-missing-fragment.rs:18:7
|
LL | ( $name ) => {};
| ^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

error: aborting due to previous error; 3 warnings emitted

2 changes: 2 additions & 0 deletions src/test/ui/parser/macro/issue-33569.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
macro_rules! foo {
{ $+ } => { //~ ERROR expected identifier, found `+`
//~^ ERROR missing fragment specifier
//~| ERROR missing fragment specifier
//~| WARN this was previously accepted
$(x)(y) //~ ERROR expected one of: `*`, `+`, or `?`
}
}
Expand Down
14 changes: 12 additions & 2 deletions src/test/ui/parser/macro/issue-33569.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | { $+ } => {
| ^

error: expected one of: `*`, `+`, or `?`
--> $DIR/issue-33569.rs:4:13
--> $DIR/issue-33569.rs:6:13
|
LL | $(x)(y)
| ^^^
Expand All @@ -16,5 +16,15 @@ error: missing fragment specifier
LL | { $+ } => {
| ^

error: aborting due to 3 previous errors
error: missing fragment specifier
--> $DIR/issue-33569.rs:2:8
|
LL | { $+ } => {
| ^
|
= note: `#[deny(missing_fragment_specifier)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #40107 <https://github.com/rust-lang/rust/issues/40107>

error: aborting due to 4 previous errors

0 comments on commit 379ae12

Please sign in to comment.