-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 crate name validation #127581
base: master
Are you sure you want to change the base?
Fix crate name validation #127581
Conversation
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
This comment has been minimized.
This comment has been minimized.
7b933ad
to
3de58e6
Compare
This comment has been minimized.
This comment has been minimized.
3de58e6
to
e688f5e
Compare
I've finally added a proper PR description. Sorry about the delay ^^'. |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the last commit still WIP?
e688f5e
to
d169eee
Compare
I've now dropped the WIP commit as I still need to investigate the implications of its changes. I'll submit it as a follow-up PR if everything works out. I've adjusted the wording in the comment to no longer use the word 'currently'. Ready for re-review. Nothing WIP left. |
This comment was marked as resolved.
This comment was marked as resolved.
d169eee
to
29a5379
Compare
This comment was marked as resolved.
This comment was marked as resolved.
29a5379
to
70053f9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
70053f9
to
34721b7
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Note that `need-crate-arg-ignore-tidy$x.rs` used to be in `tests/ui/command/` which wasn't 'correct' as that directory concerns `std::process::Command` tests not CLI tests.
Gets rid of two top-level UI tests which is always great.
34721b7
to
924d44d
Compare
Reject macro calls inside attribute
#![crate_name]
like#![crate_name = concat!("na", "me")]
.Prior to #117584, the result of the expansion (here:
"name"
) would actually be properly picked up by the compiler and used as the crate name. However since #117584 / on master, we extract the "value" (i.e., the literal string literal) of the#![crate_name]
much earlier in the pipeline way before macro expansion and skip/ignore any#![crate_name]
s "assigned to" a macro call. See also #122001.T-lang has ruled to reject
#![crate_name = MACRO!(...)]
outright very similar to other built-in attributes whose value we need early like#![crate_type]
. See accepted FCP: #122001 (comment).Note that the check as implemented in this PR is even more "aggressive" compared to the one of
#![crate_type]
by running as early as possible in order to reject#![crate_name = MACRO!(...)]
even in "non-normal" executions ofrustc
, namely on print requests (e.g.,--print=crate-name
and--print=file-names
). If I were to move the validation step a bit further back close to the#![crate_type]
one,--print=crate-name
(etc.) would not exit fatally with an error in this kind of situation but happily report an incorrect crate name (i.e., the "crate name" as if#![crate_name]
didn't exist / deduced from other sources like--crate-name
or the file name) which would match the behavior on master. Again, see also #122001.I'm mentioning this explicitly because I'm not sure if it was that clear in the FCP'ed issue. I argue that my current approach is the most reasonable one. I know (from reading the code and from past experiments) that various print requests are still quite broken (mostly lack of validation).
To the best of my knowledge, there's no print request whose output references/contains a crate type, so there's no "inherent need" to move
#![crate_type]
's validation to happen earlier.Fixes #122001.
relnotesMarks issues that should be documented in the release notes of the next release.
: Compatibility. Breaking change.