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: extract rustc_test_marker attr in test_case #113315

Closed
wants to merge 1 commit into from

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Jul 4, 2023

Fixes #100263

To prevent ice caused by normal.tokens.as_ref.unwrap(), which attempts to unwrap the None generated by attr_name_value_str

@rustbot
Copy link
Collaborator

rustbot commented Jul 4, 2023

r? @WaffleLapkin

(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 Jul 4, 2023
@WaffleLapkin
Copy link
Member

r? compiler
(I'm not really available this week)

@rustbot rustbot assigned jackh726 and unassigned WaffleLapkin Jul 4, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Jul 4, 2023

@petrochenkov I noticed that you were the reviewer for #53410. Could you assist in reviewing this PR?

@jackh726
Copy link
Member

So, very unfamiliar with this code. But this doesn't seem right to me?

The behavior of this code on master seems to change item visibility to public and adds rustc_test_marker attr. With this change, the item is duplicated (without a visibility change) and the rustc_test_marker attr is added to that? And the actual item gets its visibility changed to public?

I think this needs at least a bit more explanation for me to feel comfortable. Let's see what @petrochenkov has to say though.

r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned jackh726 Aug 13, 2023
@petrochenkov
Copy link
Contributor

I also don't understand why this PR generates an additional item.

If some built-in macro generates code (AST) visible to other macros as input, then it should set tokens in it as well (preferably correct tokens).
@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 Aug 15, 2023
@bvanjoi
Copy link
Contributor Author

bvanjoi commented Aug 15, 2023

With this change, the item is duplicated (without a visibility change) and the rustc_test_marker attr is added to that?

Its goal is to prevent the attribute(generated by attr_name_value_str) from being appended to outer_attrs, which would then unwrap the token field and potentially trigger a panic.

If some built-in macro generates code (AST) visible to other macros as input, then it should set tokens in it as well (preferably correct tokens).

Thanks. I will look into this matter further.

I have a question: is this method applicable to the #[test] macro? Because these changes in this PR make #[test_case] more similar to #[test] by generating multiple attributes and ensuring that rustc_test_marker is not inserted into outer_attrs.

@petrochenkov
Copy link
Contributor

Is test_case even used anymore?
It was originally emitted by #[test] and #[bench], but that's not the case anymore.
All this stuff may be an outdated garbage, and the whole system likely needs a redesign.
So it doesn't matter much how it works, as longs as nothing stable breaks, until some new future system is implemented.

So I suggest just setting the missing tokens to a new empty token stream and move on, nobody is using test_case with other proc macros anyway.
Or you can report an error when a non-builtin macro is found when expanding test_case.
I don't want to spend time on this.

@Dylan-DPC
Copy link
Member

@bvanjoi any updates on this?

@bvanjoi
Copy link
Contributor Author

bvanjoi commented Nov 4, 2023

Closing this as it doesn't provide substantial help.

@bvanjoi bvanjoi closed this Nov 4, 2023
@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 Nov 7, 2023
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 with multiple attributes on a test function
7 participants