Fix crate name validation #127581
Fix crate name validation #127581
Conversation
fmease
added
the
relnotes
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
fmease
force-pushed
the
fix-crate_name-validation
branch
from
7b933ad
to
3de58e6
Compare
This comment has been minimized.
This comment has been minimized.
fmease
changed the title
fmease
force-pushed
the
fix-crate_name-validation
branch
from
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.
compiler/rustc_expand/src/module.rs
Outdated
| @@ -179,12 +179,12 @@ fn mod_file_path_from_attr( | |||
| let first_path = attrs.iter().find(|at| at.has_name(sym::path))?; | |||
| let Some(path_sym) = first_path.value_str() else { | |||
| // This check is here mainly to catch attempting to use a macro, | |||
| // such as #[path = concat!(...)]. This isn't currently supported | |||
| // such as `#[path = concat!(...)]`. This isn't currently supported | |||
There was a problem hiding this comment.
Nit, maybe say "This isn't supported" and drop the "currently" because it sounds like something we want to fix in the future and we've conclusively decided we do not want to do that.
fmease
changed the title
fmease
force-pushed
the
fix-crate_name-validation
branch
from
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.
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.
fmease
force-pushed
the
fix-crate_name-validation
branch
from
d169eee
to
29a5379
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-nameand--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-nameor 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.