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

fix(parse): avoid panic when cfg wrapper by bracket under capture-cfg mode #113056

Closed
wants to merge 1 commit into from

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jun 26, 2023

Fixes #87577

The parser expected to encounter the Token::Gt but instead encountered BinOp(shr) when parsing A<T> during macro expand. As a result, the self.token_cursor.break_last_token = true in break_and_eat had been executed. This caused the parser to encounter assert!(!self.token_cursor.break_last_token, xxx) in collect_tokens_trailing_token under cpature-cfg mode.

To prevent this issue, an extra restriction was added to the assert statement. By the way, I think the better solution may be to simply remove the assert altogether.

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 26, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jun 26, 2023

Hi @Aaron1011, I see that you assigned this task to yourself, but since it has been two years since the original and I try to solve it.

By the way, I will appreciate it if you can help to review this change from the author view?

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2023
@davidtwco
Copy link
Member

I'm not familiar enough with this part of the compiler, I'll let Aaron take a look.

r? @Aaron1011

@rustbot rustbot assigned Aaron1011 and unassigned davidtwco Jun 28, 2023
@bors

This comment was marked as resolved.

@Aaron1011
Copy link
Member

Sorry for the delay in getting to this.

I don't think this is right - that assertion is to ensure that we correctly capture tokens, since capturing an unglued token didn't work at the time I wrote this.

Can you add a test using an attribute that prints/modifies the tokens, and make sure that it sees the correct tokens with this change?

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 6, 2023

I don't think this is right

However, it appears that there is no appropriate position for updating self.break_last_token during macro expansion.

Can you add a test using an attribute that prints/modifies the tokens

Since attributes cannot print the result of the macro expansion, I've used hir printing to ensure correctness.

@Dylan-DPC Dylan-DPC added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 3, 2023
@wesleywiser
Copy link
Member

Hi @Aaron1011, it looks like this is ready for another round of review when you get a chance. Cheers!

@wesleywiser
Copy link
Member

r? @estebank

@rustbot rustbot assigned estebank and unassigned Aaron1011 Dec 14, 2023
Comment on lines +365 to +367
if !(self.token.kind == TokenKind::Gt && self.unmatched_angle_bracket_count > 0) {
assert!(!self.break_last_token, "Should not have unglued last token with cfg attr");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@petrochenkov this change is so that we no longer ICE on the face of

struct S<#[cfg(feature = "alloc")] N: A<T>>;
                                         ^^ these currently get split and cause the assertion to fail unconditionally

I believe that this change is reasonable. Is there any reason we shouldn't land this PR as is? Put another way, are there any non-local knock down effects from this change that come to mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

gentle ping @petrochenkov for this comment. thanks.

@petrochenkov
Copy link
Contributor

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned estebank Mar 21, 2024
@petrochenkov
Copy link
Contributor

This needs to be tested with proc macros working on tokens, not with Debug which works on AST.
print_derive from tests\ui\proc-macro\auxiliary\test-macros.rs can be used for testing.

The derive should see the structure with the #[cfg(feature = "alloc")] part configured out, that means struct S<>;.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2024
@JohnCSimon JohnCSimon closed this May 26, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 26, 2024
@JohnCSimon JohnCSimon reopened this May 26, 2024
@JohnCSimon JohnCSimon removed the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label May 26, 2024
@JohnCSimon
Copy link
Member

@bvanjoi triage: can you post your status on this?

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#16 exporting to docker image format
#16 sending tarball 29.3s done
#16 DONE 34.3s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---
   Compiling compiletest v0.0.0 (/checkout/src/tools/compiletest)
    Finished `release` profile [optimized] target(s) in 14.07s
##[endgroup]
##[group]Testing stage2 compiletest suite=ui mode=ui (x86_64-unknown-linux-gnu)
error: detected legacy-style directive check-pass in compiletest test: /checkout/tests/ui/unpretty/unglued-token.rs:1, please use `ui_test`-style directives `//@` instead: Some(
        None,
        "check-pass",
    ),
)
)
errors encountered during EarlyProps parsing: /checkout/tests/ui/unpretty/unglued-token.rs
errors encountered during EarlyProps parsing
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:12:27
  local time: Sun May 26 23:38:11 UTC 2024

@bvanjoi
Copy link
Contributor Author

bvanjoi commented May 27, 2024

I still haven't found the right solution, so I'm closing this.

@bvanjoi bvanjoi closed this May 27, 2024
@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: Should not have unglued last token with cfg attr