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.

Sign up for GitHub

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

Jump to bottom

Disallow attribute macros named cfg #53531

Closed
djrenren opened this issue Aug 20, 2018 · 8 comments
Closed

Disallow attribute macros named cfg #53531

djrenren opened this issue Aug 20, 2018 · 8 comments
Labels
A-macros-1.2 Area: Declarative macros 1.2

Comments

@djrenren
Copy link
Contributor

#[cfg()] runs before name resolution so no proc macro attribute named cfg could ever be called. We should make this a hard error at the declaration site to protect people from this confusion.
@djrenren
Copy link
Contributor Author

I think this would be a good issue for mentoring. I'm happy to help out.

@Havvy
Copy link
Contributor

Havvy commented Aug 20, 2018

The test attribute seems to be similarly ignored.

@otavio
Copy link
Contributor

otavio commented Aug 20, 2018

I could give it a go. As a first time contributor please be patient 😉

@djrenren
Copy link
Contributor Author

@Havvy Yeah, though that's not by necessity, so I wanna do a crater run and see if we could give it normal resolution.

@djrenren
Copy link
Contributor Author

djrenren commented Aug 20, 2018 •
edited
Loading

@otavio That's awesome! I'm happy to help in any way you need.

I recommend taking a look at src/libsyntax_ext/proc_macro_registrar.rs

@eddyb
Copy link
Member

eddyb commented Aug 21, 2018 •
edited
Loading

I'd check it during name resolution instead, so you can do #[foo::cfg(...)] just fine, but not import it unqualified.

cc @petrochenkov

@Havvy Havvy added the A-macros-1.2 Area: Declarative macros 1.2 label Aug 21, 2018
@petrochenkov
Copy link
Contributor

I have a PR in progress that resolves the penultimate item in #50911 (comment) and passes all attributes through name resolution, that includes cfg as well.

(Disallowing proc macros named cfg is both too much - #[foo::cfg] is ok, and not enough - use my_non_cfg_attr_macro as cfg;).

@djrenren
Copy link
Contributor Author

@petrochenkov Ah yeah I hadn't thought about that! That's awesome.

@petrochenkov petrochenkov mentioned this issue Sep 3, 2018
Merged
bors added a commit that referenced this issue Sep 3, 2018
@bors
Auto merge of #53913 - petrochenkov:biattr4, r=<try>
5fe695a 
resolve: Future proof resolutions for potentially built-in attributes

Based on #53778

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.
TODO: Explain what complications arise with the full solution.

cc #50911 (comment)
Closes #53531
bors added a commit that referenced this issue Sep 11, 2018
@bors
Auto merge of #53913 - petrochenkov:biattr4, r=alexcrichton
5806389 
resolve: Future proof resolutions for potentially built-in attributes

This is not full "pass all attributes through name resolution", but a more conservative solution.
If built-in attribute is ambiguous with any other macro in scope, then an error is reported.

What complications arise with the full solution - #53913 (comment).

cc #50911 (comment)
cc #52269
Closes #53531
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-macros-1.2 Area: Declarative macros 1.2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@otavio @eddyb @djrenren @Havvy @petrochenkov