Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parser: Ensure that all nonterminals have tokens after parsing #84995

Merged
merged 1 commit into from
Jun 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion compiler/rustc_ast/src/ast_like.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ impl AstLike for crate::token::Nonterminal {
Nonterminal::NtMeta(attr_item) => attr_item.tokens_mut(),
Nonterminal::NtPath(path) => path.tokens_mut(),
Nonterminal::NtVis(vis) => vis.tokens_mut(),
_ => panic!("Called tokens_mut on {:?}", self),
Nonterminal::NtBlock(block) => block.tokens_mut(),
Nonterminal::NtIdent(..) | Nonterminal::NtLifetime(..) | Nonterminal::NtTT(..) => None,
}
}
}
Expand Down
10 changes: 2 additions & 8 deletions compiler/rustc_parse/src/parser/attr_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -342,16 +342,10 @@ impl<'a> Parser<'a> {

// If we support tokens at all
if let Some(target_tokens) = ret.tokens_mut() {
if let Some(target_tokens) = target_tokens {
assert!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hit in cases like #[cfg_eval] #[rustc_dummy] 0.

Tokens for #[rustc_dummy] 0 are collected twice due to the new behavior of parse_expr_force_collect, but we still get to here despite the

        if !self.capture_cfg && matches!(ret.tokens_mut(), None | Some(Some(_))) {
            return Ok(ret);
        }

above because in case of cfg_eval the self.capture_cfg condition is true.

!self.capture_cfg,
"Encountered existing tokens with capture_cfg set: {:?}",
target_tokens
);
} else {
if target_tokens.is_none() {
// Store se our newly captured tokens into the AST node
*target_tokens = Some(tokens.clone());
};
}
}

let final_attrs = ret.attrs();
Expand Down
12 changes: 1 addition & 11 deletions compiler/rustc_parse/src/parser/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,7 @@ impl<'a> Parser<'a> {

/// Parses an expression, forcing tokens to be collected
pub fn parse_expr_force_collect(&mut self) -> PResult<'a, P<Expr>> {
// If we have outer attributes, then the call to `collect_tokens_trailing_token`
// will be made for us.
if matches!(self.token.kind, TokenKind::Pound | TokenKind::DocComment(..)) {
self.parse_expr()
} else {
// If we don't have outer attributes, then we need to ensure
// that collection happens by using `collect_tokens_no_attrs`.
// Expression don't support custom inner attributes, so `parse_expr`
// will never try to collect tokens if we don't have outer attributes.
self.collect_tokens_no_attrs(|this| this.parse_expr())
}
self.collect_tokens_no_attrs(|this| this.parse_expr())
Copy link
Member

@Aaron1011 Aaron1011 May 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, this would be incorrect. collect_tokens_no_attrs was only supposed to be used when the target was guaranteed to not have any outer attributes - if outer attributes are present, we need to capture their tokens separately so that we can create a proper ReplaceRange.

However, we only actually need to know the attribute start position when we're in capture_cfg mode, which never happens when we're parsing nonterminals (we flatten all nonterminals before entering capture_cfg mode). Can you add a debug_assert!(!self.capture_cfg) to parse_expr_force_collect to make sure this method doesn't get misused?

I may do some refactoring in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert will fire because parse_expr_force_collect is indeed used with capture_cfg == true.
Here - https://github.com/rust-lang/rust/blob/master/compiler/rustc_builtin_macros/src/cfg_eval.rs#L194

(The result of cfg_eval should have tokens collected, especially with #85073.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that wrapping into collect_tokens_no_attrs is ok because there are two cases:

  • There are active attributes, then token collection will be called once again in parse_expr, will set tokens for the returned expression, so the tokens collected by collect_tokens_no_attrs will simply be thrown away.
    (I'm not sure what happens with range replacement in this case.)
  • There are no active attributes, then collect_tokens_no_attrs is actually doing the right thing and it will set tokens for the returned expression. (I'm again not sure what will happen with range replacement in this case.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will happen to do the right thing with replace ranges - we'll end up creating a ReplaceRange covering the entire expression (including outer attributes), as expected. Since we'll throw away the new tokens, the final captured tokens will not include the outer attributes, which will allow us to correctly perform cfg-expansion.

I forgot about the fact that we use force collection at the 'top level' of cfg_eval. It's unfortunate that we'll lose the asertion, but I don't think keeping it is worth the extra complexity of keeping track of the 'top-level' status on top of everything else.

}

pub fn parse_anon_const_expr(&mut self) -> PResult<'a, AnonConst> {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_parse/src/parser/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ enum BlockMode {

/// Whether or not we should force collection of tokens for an AST node,
/// regardless of whether or not it has attributes
#[derive(Clone, Copy, PartialEq)]
pub enum ForceCollect {
Yes,
No,
Expand Down
17 changes: 15 additions & 2 deletions compiler/rustc_parse/src/parser/nonterminal.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use rustc_ast::ptr::P;
use rustc_ast::token::{self, Nonterminal, NonterminalKind, Token};
use rustc_ast::AstLike;
use rustc_ast_pretty::pprust;
use rustc_errors::PResult;
use rustc_span::symbol::{kw, Ident};
Expand Down Expand Up @@ -102,7 +103,7 @@ impl<'a> Parser<'a> {
// which requires having captured tokens available. Since we cannot determine
// in advance whether or not a proc-macro will be (transitively) invoked,
// we always capture tokens for any `Nonterminal` which needs them.
Ok(match kind {
let mut nt = match kind {
NonterminalKind::Item => match self.parse_item(ForceCollect::Yes)? {
Some(item) => token::NtItem(item),
None => {
Expand Down Expand Up @@ -169,7 +170,19 @@ impl<'a> Parser<'a> {
return Err(self.struct_span_err(self.token.span, msg));
}
}
})
};

// If tokens are supported at all, they should be collected.
if matches!(nt.tokens_mut(), Some(None)) {
panic!(
"Missing tokens for nt {:?} at {:?}: {:?}",
nt,
nt.span(),
pprust::nonterminal_to_string(&nt)
);
}

Ok(nt)
}
}

Expand Down
23 changes: 14 additions & 9 deletions compiler/rustc_parse/src/parser/stmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ impl<'a> Parser<'a> {
// or `auto trait` items. We aim to parse an arbitrary path `a::b` but not something
// that starts like a path (1 token), but it fact not a path.
// Also, we avoid stealing syntax from `parse_item_`.
self.parse_stmt_path_start(lo, attrs, force_collect)?
if force_collect == ForceCollect::Yes {
self.collect_tokens_no_attrs(|this| this.parse_stmt_path_start(lo, attrs))
} else {
self.parse_stmt_path_start(lo, attrs)
}?
} else if let Some(item) =
self.parse_item_common(attrs.clone(), false, true, |_| true, force_collect)?
{
Expand All @@ -85,21 +89,22 @@ impl<'a> Parser<'a> {
self.mk_stmt(lo, StmtKind::Empty)
} else if self.token != token::CloseDelim(token::Brace) {
// Remainder are line-expr stmts.
let e = self.parse_expr_res(Restrictions::STMT_EXPR, Some(attrs))?;
let e = if force_collect == ForceCollect::Yes {
self.collect_tokens_no_attrs(|this| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a call to collect_tokens_trailing_token, with attrs passed in (since we do have outer attributes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed this case is analogous to parse_expr_force_collect (https://github.com/rust-lang/rust/pull/84995/files#r629329546) so we can use collect_tokens_no_attrs.

Also, I don't think we can simply pass attrs here because they don't necessary apply to the whole statement, they can apply only to some sub-expression like in #[attr] 1 + 2 (grouped as (#[attr] 1) + 2).

this.parse_expr_res(Restrictions::STMT_EXPR, Some(attrs))
})
} else {
self.parse_expr_res(Restrictions::STMT_EXPR, Some(attrs))
}?;
self.mk_stmt(lo.to(e.span), StmtKind::Expr(e))
} else {
self.error_outer_attrs(&attrs.take_for_recovery());
return Ok(None);
}))
}

fn parse_stmt_path_start(
&mut self,
lo: Span,
attrs: AttrWrapper,
force_collect: ForceCollect,
) -> PResult<'a, Stmt> {
let stmt = self.collect_tokens_trailing_token(attrs, force_collect, |this, attrs| {
fn parse_stmt_path_start(&mut self, lo: Span, attrs: AttrWrapper) -> PResult<'a, Stmt> {
let stmt = self.collect_tokens_trailing_token(attrs, ForceCollect::No, |this, attrs| {
let path = this.parse_path(PathStyle::Expr)?;

if this.eat(&token::Not) {
Expand Down
37 changes: 37 additions & 0 deletions src/test/ui/proc-macro/expr-stmt-nonterminal-tokens.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// check-pass
// aux-build:test-macros.rs

#![feature(decl_macro)]
#![feature(stmt_expr_attributes)]

#![no_std] // Don't load unnecessary hygiene information from std
extern crate std;

#[macro_use]
extern crate test_macros;

macro mac {
(expr $expr:expr) => {
#[derive(Print)]
enum E {
V = { let _ = $expr; 0 },
}
},
(stmt $stmt:stmt) => {
#[derive(Print)]
enum E {
V = { let _ = { $stmt }; 0 },
}
},
}

const PATH: u8 = 2;

fn main() {
mac!(expr #[allow(warnings)] 0);
mac!(stmt 0);
mac!(stmt {});
mac!(stmt PATH);
mac!(stmt 0 + 1);
mac!(stmt PATH + 1);
}
Loading