Skip to content

fix: Do not replace items annotated with builtin attrs with the attr input #9943

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

Merged
merged 3 commits into from
Aug 20, 2021

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Aug 18, 2021

This causes runnables to work for paths that actually resolve to the test attribute now.
Code_aUhX1KQfw3

Prior to this we replaced items annotated with builtin attributes by the attribute input instead of the item in our dummy expansion which is why the fully written path didn't work as we actually discarded the item while test was just ignored.

Fixes #9935

@Veykril Veykril force-pushed the attr-repl branch 2 times, most recently from e4929bd to 45d7f30 Compare August 18, 2021 13:36
@Veykril
Copy link
Member Author

Veykril commented Aug 18, 2021

Interestingly this does not break the runnables tests even though this breaks the runnables functionality 😕 Do our tests not expand (built-in) attributes yet?

@Veykril Veykril changed the title Do not replace items annotated with builtin attrs with the attr input fix: Do not replace items annotated with builtin attrs with the attr input Aug 18, 2021
@Veykril
Copy link
Member Author

Veykril commented Aug 18, 2021

Hmm fixing this is tricky... Basically we want to go up all attribute expansions so we get to the item with the #[test] attribute but we do not want to go up more in case this item is generated by a macro since then we can't grab the item's syntax anymore as it doesn't exist in the top level non-macro file.

I don't think we can fix this properly with our current macro mapping infra can we?

@Veykril
Copy link
Member Author

Veykril commented Aug 18, 2021

I don't think we can fix this properly with our current macro mapping infra can we?

Nevermind I think we can 🎉

@Veykril
Copy link
Member Author

Veykril commented Aug 20, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 20, 2021

@bors bors bot merged commit da5a5ba into rust-lang:master Aug 20, 2021
@Veykril Veykril deleted the attr-repl branch August 20, 2021 12:06
bors bot added a commit that referenced this pull request Aug 21, 2021
9965: minor: Don't ask for the builtin attribute input twice r=Veykril a=Veykril

`tt` and `item` here were the same, I misunderstood what the main input for attributes was in #9943
bors r+

Co-authored-by: Lukas Wirth <[email protected]>
@lnicola
Copy link
Member

lnicola commented Aug 22, 2021

This increased the memory usage on self from 915 to 1045 MB.

@Veykril
Copy link
Member Author

Veykril commented Aug 22, 2021

This is to be expected, as we now expand builtin attributes back to the item so basically duplicate builtin-attr annotated items into a macrofile.

Might make sense to investigate whether we actually need this expansion for builtin attributes?

bors bot added a commit that referenced this pull request Aug 27, 2021
10044: minor: Ignore text and bench attributes again r=Veykril a=Veykril

Reverts part of #9943

Co-authored-by: Lukas Wirth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokio::test fn not recognized as a test when procedural macros are enabled
3 participants