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

#![crate_name = EXPR] semantically allows EXPR to be a macro call but otherwise mostly ignores it #122001

Open
fmease opened this issue Mar 4, 2024 · 18 comments · May be fixed by #127581
Open
Assignees
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@fmease
Copy link
Member

fmease commented Mar 4, 2024

Contrary to #![crate_type = EXPR], #![crate_name = EXPR] does not semantically reject macro calls inside EXPR.
Instead, it eagerly expands them but otherwise ignores the result (apart from errors).

I presume this regressed when #78835 ( F-extended_key_value_attributes `#![feature(extended_key_value_attributes)] ) was stabilized.
If so, this is a 1.53→1.54 stable-to-stable regression.

Update: It's a 1.75→1.76 stable-to-stable regression. Regressing PR: #117584.
Thanks ehuss, for the investigation! The result of the expansion used to be used!

Examples

The following examples all pass compilation and rustc completely ignores the crate name that comes from the expansion. This can be observed by for example running rustc file.rs --print=crate-name prints file (the file name is assumed to be file.rs).

(A)

#![crate_name = concat!("alia", "s")] // ignored, crate name is `file`, not `alias`
fn main() {}

(B)

#![crate_name = include_str!("crate_name.txt")] // ignored, crate name is `file`, not `alias`
fn main() {}

where crate_name.txt exists and consists of alias.

(C)

#![crate_name = dep::generate!()] // ignored, crate name is `file`, not `alias`
fn main() {}

where we compile file.rs with --extern=dep -L. --edition=2021 and where dep.rs is:

#[macro_export]
macro_rules! generate { () => { "alias" } }
@fmease fmease added A-attributes Area: Attributes (`#[…]`, `#![…]`) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 4, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 4, 2024
@fmease fmease closed this as not planned Won't fix, can't repro, duplicate, stale Mar 4, 2024
@fmease fmease added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 4, 2024
@fmease fmease changed the title #![crate_name = EXPR] lacks several semantic checks #![crate_name = EXPR] semantically allows EXPR to be a macro call but otherwise ignores the result of the expansion (apart from errors) Mar 4, 2024
@fmease fmease reopened this Mar 4, 2024
@fmease fmease changed the title #![crate_name = EXPR] semantically allows EXPR to be a macro call but otherwise ignores the result of the expansion (apart from errors) #![crate_name = EXPR] semantically allows EXPR to be a macro call, eagerly expands it but otherwise ignores the result of the expansion apart from errors Mar 4, 2024
@fmease fmease changed the title #![crate_name = EXPR] semantically allows EXPR to be a macro call, eagerly expands it but otherwise ignores the result of the expansion apart from errors #![crate_name = EXPR] semantically allows EXPR to be a macro call but otherwise mostly ignores it Mar 4, 2024
@fmease
Copy link
Member Author

fmease commented Mar 4, 2024

Needs a future-incompat warn-by-default lint as a first step.

@fmease fmease self-assigned this Mar 4, 2024
@ehuss
Copy link
Contributor

ehuss commented Mar 4, 2024

😦 Not sure how I missed this one in #88680.

@fmease
Copy link
Member Author

fmease commented Mar 5, 2024

@ehuss Don't beat yourself up, nobody else noticed it either ^^.

Oh, I've just double-checked the dates, your validation PR was merged Sep 26, 2021 which is after the aforementioned stabilization PR, merged May 19, 2021. I've only skimmed the thread but it seems to have been FCP'ed because it was a breaking change?

Maybe we can even get away with making macro calls inside #![crate_name] a hard error after an FCP? Well, almost three years have passed since EKVA's stab PR but on the other hand, I presume that very few people actually use #![crate_name] since the majority just uses Cargo.

AFAIK, crater only supports Cargo projects (makes sense), so we can't use it to look for breakages.
However, GitHub Code Search returns zero matches given the regex #!\[crate_name\s*=\s*[\w_:]+ ... so there's hope.

@ehuss
Copy link
Contributor

ehuss commented Mar 5, 2024

Oh wait, this used to work. It regressed relatively recently in #117584. cc @bjorn3

We could hard error it if nobody is using it. Before doing that, I think we'd need a good motivation to not support it (such as, is the crate_name needed before expansion?).

@fmease

This comment was marked as outdated.

@fmease
Copy link
Member Author

fmease commented Mar 5, 2024

We could hard error it if nobody is using it. Before doing that, I think we'd need a good motivation to not support it (such as, is the crate_name needed before expansion?).

I haven't checked the code yet but I don't know if rustc's current architecture permits macro expansion for --print=crate-name. This relates to the deprecation of #![cfg_attr(SPEC, crate_name = "...")] and #![cfg_attr(SPEC, crate_type = "...")] (#91632), I think.

Well, we could theoretically expand macros for --print=crate-name but that would also be a breaking change since #![crate_name = compile_error!("...")] currently compiles successfully under --print=crate-name because it only parses the crates I believe and if we were to start expanding, this would obviously regress. Note that contrary to --print=crate-name, --print=link-args for example does parse & expand (& typeck & compile) the crate but that's an outlier under all print request flags.

Edit: Clarification: The CLI flag --print=crate-name is just a representative for rustc's internal understanding of the name of the local crate. Oh well, in 1.75 all examples used to work “as expected” while --print=crate-name didn't, so the struck-through sentence isn't grounded in reality.

@ehuss
Copy link
Contributor

ehuss commented Mar 5, 2024

Error for example (A) in Rust 1.75:

Hm, I'm not getting the same result. When I compile A with 1.75, it generates a binary alias. With 1.76, it generates a binary named based on the source filename.

rustc's current architecture permits macro expansion for --print=crate-name

Oh, that's interesting. Yea, --print=crate-name does not seem to work for eager expansion in any version.

@fmease
Copy link
Member Author

fmease commented Mar 5, 2024

Hm, I'm not getting the same result. When I compile A with 1.75, it generates a binary alias. With 1.76, it generates a binary named based on the source filename.

Oh 🤦, I tested this with #![crate_name = concat!()]. I can confirm that with 1.75 the result of the expansion used to get used.

@apiraino
Copy link
Contributor

apiraino commented Mar 5, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 5, 2024
@fmease fmease removed their assignment Mar 5, 2024
@fmease fmease added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Mar 5, 2024
@fmease
Copy link
Member Author

fmease commented Mar 5, 2024

I nominate this for T-compiler discussion since I wonder if it was ever intentional to (correctly) support #![crate_name = MACRO_CALL] (i.e., use the result of the expansion) between versions 1.54 (stabilization of EKVA1) and 1.75 inclusive (the expansion gets ignored since PR #117584 / 1.76 which is definitely a bug) or if #![crate_name = MACRO_CALL] should've always lead to a semantic error similar to #![crate_type = MACRO_CALL].

Possibly relevant to the discussion:

This might be a T-lang question at its core but I figure it makes sense for T-compiler to chime in first since its members might have more background information on this (compiler stages / passes, architectural limitations, when to expand when not to for simplicity of implementation).

Footnotes

  1. #![feature(extended_key_value_attributes)].

@pnkfelix
Copy link
Member

pnkfelix commented Mar 7, 2024

@rustbot label: +I-lang-nominated

I think the question of whether we ever intended to support #![crate_name = MACRO_CALL] belongs under T-lang rather than T-compiler.

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Mar 7, 2024
@petrochenkov
Copy link
Contributor

There's a list of attributes that need to be used very early, before macro expansion - #108221 (comment) (also see uses of pre_configured_attrs in the compiler).
In such attributes #![attr = mac_call!()] cannot be supported.

There are also some proposals that may reduce the number of attributes in this list, like rust-lang/compiler-team#726.

@fmease fmease removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Mar 7, 2024
@scottmcm scottmcm added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 27, 2024
@scottmcm
Copy link
Member

I think that intentionally-breaking this, so that it works like crate_type would make sense. Let's see if lang agrees:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Mar 27, 2024

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Mar 27, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@fmease fmease self-assigned this May 21, 2024
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 21, 2024
@rfcbot
Copy link

rfcbot commented Jun 21, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jun 21, 2024
@tmandry
Copy link
Member

tmandry commented Jun 22, 2024

@rfcbot reviewed

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 1, 2024
@rfcbot
Copy link

rfcbot commented Jul 1, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@tmandry tmandry removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jul 3, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 4, 2024
@fmease fmease linked a pull request Jul 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants