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

Inert attributes in $expr nonterminals are sometimes duplicated #86055

Closed
petrochenkov opened this issue Jun 6, 2021 · 1 comment
Closed
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug.

Comments

@petrochenkov
Copy link
Contributor

Reproducer:

macro mac($expr:expr) {
    print_bang!($expr);
}

fn main() {
    mac!(#[allow(warnings)] 0);
}

where print_bang is a proc macro from https://github.com/rust-lang/rust/blob/master/src/test/ui/proc-macro/auxiliary/test-macros.rs

The result looks like this:

PRINT-BANG INPUT (DISPLAY): #[allow(warnings)] 0
PRINT-BANG RE-COLLECTED (DISPLAY): #[allow(warnings)] #[allow(warnings)] 0

The issue can be reproduced after merging #84995, before that PR the example ICEs.

@petrochenkov petrochenkov added the C-bug Category: This is a bug. label Jun 6, 2021
@jonas-schievink jonas-schievink added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Jun 6, 2021
@bvanjoi
Copy link
Contributor

bvanjoi commented Jun 5, 2023

It appears that parse_expr_force_collect has caused duplication of the Expr between the attrs and tokens. Can we remove these duplicated attrs by comparing tokens and attrs?

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2024
…nt-expr, r=petrochenkov

Fix duplicated attributes on nonterminal expressions

This PR fixes a long-standing bug (rust-lang#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`
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 19, 2024
…nt-expr, r=petrochenkov

Fix duplicated attributes on nonterminal expressions

This PR fixes a long-standing bug (rust-lang#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`
@bors bors closed this as completed in 64c2e9e Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

3 participants