Skip to content

Commit

Permalink
Auto merge of #126678 - nnethercote:fix-duplicated-attrs-on-nt-expr, …
Browse files Browse the repository at this point in the history
…r=petrochenkov

Fix duplicated attributes on nonterminal expressions

This PR fixes a long-standing bug (#86055) whereby expression attributes can be duplicated when expanded through declarative macros.

First, consider how items are parsed in declarative macros:
```
Items:
- parse_nonterminal
  - parse_item(ForceCollect::Yes)
    - parse_item_
      - attrs = parse_outer_attributes
      - parse_item_common(attrs)
        - maybe_whole!
        - collect_tokens_trailing_token
```
The important thing is that the parsing of outer attributes is outside token collection, so the item's tokens don't include the attributes. This is how it's supposed to be.

Now consider how expression are parsed in declarative macros:
```
Exprs:
- parse_nonterminal
  - parse_expr_force_collect
    - collect_tokens_no_attrs
      - collect_tokens_trailing_token
        - parse_expr
          - parse_expr_res(None)
            - parse_expr_assoc_with
              - parse_expr_prefix
                - parse_or_use_outer_attributes
                - parse_expr_dot_or_call
```
The important thing is that the parsing of outer attributes is inside token collection, so the the expr's tokens do include the attributes, i.e. in `AttributesData::tokens`.

This PR fixes the bug by rearranging expression parsing to that outer attribute parsing happens outside of token collection. This requires a number of small refactorings because expression parsing is somewhat complicated. While doing so the PR makes the code a bit cleaner and simpler, by eliminating `parse_or_use_outer_attributes` and `Option<AttrWrapper>` arguments (in favour of the simpler `parse_outer_attributes` and `AttrWrapper` arguments), and simplifying `LhsExpr`.

r? `@petrochenkov`
  • Loading branch information
bors committed Jun 19, 2024
2 parents 3186d17 + 64c2e9e commit 79d1c3f
Show file tree
Hide file tree
Showing 13 changed files with 118 additions and 144 deletions.
4 changes: 3 additions & 1 deletion compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,9 @@ impl UnOp {
}
}

/// A statement
/// A statement. No `attrs` or `tokens` fields because each `StmtKind` variant
/// contains an AST node with those fields. (Except for `StmtKind::Empty`,
/// which never has attrs or tokens)
#[derive(Clone, Encodable, Decodable, Debug)]
pub struct Stmt {
pub id: NodeId,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_parse/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ parse_dot_dot_dot_for_remaining_fields = expected field pattern, found `{$token_
parse_dot_dot_dot_range_to_pattern_not_allowed = range-to patterns with `...` are not allowed
.suggestion = use `..=` instead
parse_dot_dot_range_attribute = attributes are not allowed on range expressions starting with `..`
parse_dotdotdot = unexpected token: `...`
.suggest_exclusive_range = use `..` for an exclusive range
.suggest_inclusive_range = or `..=` for an inclusive range
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_parse/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2990,3 +2990,10 @@ pub(crate) struct ExprRArrowCall {
#[suggestion(style = "short", applicability = "machine-applicable", code = ".")]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(parse_dot_dot_range_attribute)]
pub(crate) struct DotDotRangeAttribute {
#[primary_span]
pub span: Span,
}
6 changes: 5 additions & 1 deletion compiler/rustc_parse/src/parser/attr_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::ops::Range;
/// for the attribute target. This allows us to perform cfg-expansion on
/// a token stream before we invoke a derive proc-macro.
///
/// This wrapper prevents direct access to the underlying `ast::AttrVec>`.
/// This wrapper prevents direct access to the underlying `ast::AttrVec`.
/// Parsing code can only get access to the underlying attributes
/// by passing an `AttrWrapper` to `collect_tokens_trailing_tokens`.
/// This makes it difficult to accidentally construct an AST node
Expand Down Expand Up @@ -177,6 +177,10 @@ impl<'a> Parser<'a> {
/// into a `LazyAttrTokenStream`, and returned along with the result
/// of the callback.
///
/// The `attrs` passed in are in `AttrWrapper` form, which is opaque. The
/// `AttrVec` within is passed to `f`. See the comment on `AttrWrapper` for
/// details.
///
/// Note: If your callback consumes an opening delimiter
/// (including the case where you call `collect_tokens`
/// when the current token is an opening delimiter),
Expand Down
13 changes: 10 additions & 3 deletions compiler/rustc_parse/src/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2499,7 +2499,8 @@ impl<'a> Parser<'a> {
/// wrapped in braces.
pub(super) fn handle_unambiguous_unbraced_const_arg(&mut self) -> PResult<'a, P<Expr>> {
let start = self.token.span;
let expr = self.parse_expr_res(Restrictions::CONST_EXPR, None).map_err(|mut err| {
let attrs = self.parse_outer_attributes()?;
let expr = self.parse_expr_res(Restrictions::CONST_EXPR, attrs).map_err(|mut err| {
err.span_label(
start.shrink_to_lo(),
"while parsing a const generic argument starting here",
Expand Down Expand Up @@ -2621,7 +2622,10 @@ impl<'a> Parser<'a> {
if is_op_or_dot {
self.bump();
}
match self.parse_expr_res(Restrictions::CONST_EXPR, None) {
match (|| {
let attrs = self.parse_outer_attributes()?;
self.parse_expr_res(Restrictions::CONST_EXPR, attrs)
})() {
Ok(expr) => {
// Find a mistake like `MyTrait<Assoc == S::Assoc>`.
if token::EqEq == snapshot.token.kind {
Expand Down Expand Up @@ -2675,7 +2679,10 @@ impl<'a> Parser<'a> {
&mut self,
mut snapshot: SnapshotParser<'a>,
) -> Option<P<ast::Expr>> {
match snapshot.parse_expr_res(Restrictions::CONST_EXPR, None) {
match (|| {
let attrs = self.parse_outer_attributes()?;
snapshot.parse_expr_res(Restrictions::CONST_EXPR, attrs)
})() {
// Since we don't know the exact reason why we failed to parse the type or the
// expression, employ a simple heuristic to weed out some pathological cases.
Ok(expr) if let token::Comma | token::Gt = snapshot.token.kind => {
Expand Down
Loading

0 comments on commit 79d1c3f

Please sign in to comment.