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

create dummy placeholder crate to prevent compiler from panicing #107721

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

megakorre
Copy link
Contributor

@megakorre megakorre commented Feb 6, 2023

This PR is to address the panic found in #105700.

There are 2 separate things going on with this panic.
First the code could not generate a dummy response for crate fragment types when it hits the recursion limit.
This PR adds the method to the trait implementation for DymmyResult to be able to create a dummy crate node.
This stops the panic from happening.

The second thing that is not addressed (and maybe does not need addressing? 🤷🏻)
is that when you have multiple attributes it ends up treating attributes that follow another as being the result of expanding the former (maybe there is a better way to say that). So you end up hitting the recursion limit. Even though you would think there is no expansion happening here.

If you did not hit the recursion limit the compiler would output that invalid_attribute does not exists. But it currently exits before the resolution step when the recursion limit is reached here.

@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2023

r? @michaelwoerister

(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 Feb 6, 2023
@megakorre megakorre marked this pull request as ready for review February 6, 2023 12:58
@michaelwoerister
Copy link
Member

Thanks for the PR, @megakorre! Adding the make_crate implementation for DummyResult seems like a good idea.

However, I don't quite understand how a non-existing attribute interacts with the recursion limit. Maybe @petrochenkov knows what's exactly going on here? Maybe unknown attributes are speculatively assumed to be proc-macros and the recursion limit is checked before expansion?

@petrochenkov
Copy link
Contributor

r? @petrochenkov

compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved
compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

The second thing that is not addressed ... is that when you have multiple attributes it ends up treating attributes that follow another as being the result of expanding the former

That's an expected behavior.
Before the first attribute is expanded the second attribute basically doesn't exist, it's just some token stream in the shape of an attribute, the first attribute can arbitrarily change or remove it.
So the actual post-expansion second attribute is indeed an expansion result of the first attribute.

@petrochenkov
Copy link
Contributor

However, I don't quite understand how a non-existing attribute interacts with the recursion limit.
Maybe unknown attributes are speculatively assumed to be proc-macros and the recursion limit is checked before expansion?

All non-builtin attributes are expanded (maybe trivially) and contribute to the recursion count, including unresolved ones.
Even something like #![rustfmt::skip] is counted, because it needs to be resolved and trivially expanded.

@petrochenkov petrochenkov 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 Feb 18, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2023

📌 Commit 0fd2a70 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2023
@bors
Copy link
Contributor

bors commented Feb 20, 2023

⌛ Testing commit 0fd2a70 with merge 267cd1d...

@bors
Copy link
Contributor

bors commented Feb 20, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 267cd1d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2023
@bors bors merged commit 267cd1d into rust-lang:master Feb 20, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (267cd1d): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

None yet

6 participants