Make the current crate's name available in Session#153924
Make the current crate's name available in Session#153924jyn514 wants to merge 3 commits intorust-lang:mainfrom
Session#153924Conversation
|
Some changes occurred in check-cfg diagnostics cc @Urgau Some changes occurred in src/tools/clippy cc @rust-lang/clippy The Miri subtree was changed cc @rust-lang/miri |
|
rustbot has assigned @jdonszelmann. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
This comment has been minimized.
This comment has been minimized.
|
Do we have something ensuring that incremental compilation doesn't assume the crate name isn't changed? Maybe it's already part of the base key getting hashed into everything? |
cd84bcc to
376b6c8
Compare
We still don't allow changing the crate name during compilation. Once we set the OnceLock it can't be changed again. Everything that accesses the OnceLock panics if it isn't set. |
| let feed = tcx.create_crate_num(stable_crate_id).unwrap(); | ||
| assert_eq!(feed.key(), LOCAL_CRATE); | ||
| feed.crate_name(crate_name); | ||
| feed.crate_name(sess.crate_name()); |
There was a problem hiding this comment.
this needs to stay a query so that it's possible to look up the crate name for other dependent crates.
|
The Miri changes seem like a straightforward adaptation of the subtree to an API change. LGTM. |
This comment has been minimized.
This comment has been minimized.
|
r? rustdoc |
|
@fmease Waffle has already reviewed the compiler changes, could you look at the rustdoc changes and see if they look good? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Previously, this was only accessible with a full `TyCtxt`, or by looking at `sess.opts.crate_name`. The option is not reliable though -- it doesn't consider `#![crate_name]` and doesn't infer the name from the file stem. Add all variants of the crate name to `Session` instead: - Add a new `CrateName` struct to make it more clear what's going on - Distinguish between the crate name and filestem (previously the filestem was often called the crate name even though it was unnormalized) - Add `is_build_script` helper - Distinguish between crate name and filestem in tools - Distinguish the normalized and unnormalized crate name while calculating the filestem - Remove unnecessary `crate_name` arguments
09bffb5 to
f53995d
Compare
|
I need to review, from the description it seems like it may go into a wrong direction generally. |
| { | ||
| return; | ||
| } | ||
| sess.crate_name.set(new_name).expect("`load_crate_name` called more than once!"); |
There was a problem hiding this comment.
I'm not that big of a fan of modifying global state.
There was a problem hiding this comment.
do you see an alternative? I document on the crate_name session option why it's hard to get this before Session construction.
There was a problem hiding this comment.
Respecting #![crate_name] in compiler/rustc_metadata/src/locator.rs is just cosmetic and src/tools/miri/src/machine.rs it is not that much of an issue either as miri doesn't have a stable way of invoking it without cargo miri either. For the rest of the places manually passing crate_name like we currently do works fine, right?
There was a problem hiding this comment.
I agree, if tcx is directly available, then use tcx.crate_name(), otherwise pass crate name from the outside (probably from the same tcx).
tcx is created very early now, almost immediately after pre-configuring crate root attributes and evaluating get_crate_name, so there's a very small window in which the non-approximated crate name is available, but tcx is not.
There was a problem hiding this comment.
removing dependencies on TyCtxt that only need the crate name, such as in
rustc_attr_parsing
I don't think parsing should depend on knowing the crate name in general, I guess occasionally it may be useful for diagnostics (but need examples), but then it can be passed from the outside as well.
There was a problem hiding this comment.
My inclination would also have been to figure out how to have TyCtxt available earlier or do the parts that need the crate name later.
I'd need to do a dive to get the full picture here, but I'm happy to do that and then do some brainstorming session to see what other options we have to make this all nicer
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| prints: Vec<PrintRequest> [UNTRACKED], | ||
| cg: CodegenOptions [SUBSTRUCT CodegenOptionsTargetModifiers CodegenOptions], | ||
| externs: Externs [UNTRACKED], | ||
| #[deprecated = "use `Session::crate_name()` instead, which accounts for `#![crate_name]`"] |
There was a problem hiding this comment.
Maybe rustc::bad_opt_access is a better option?
|
☔ The latest upstream changes (presumably #154032) made this pull request unmergeable. Please resolve the merge conflicts. |
Previously, this was only accessible with a full
TyCtxt, or by looking atsess.opts.crate_name. The option is not reliable though -- it doesn't consider#![crate_name]and doesn't infer the name from the file stem.Add all variants of the crate name to
Sessioninstead:CrateNamestruct to make it more clear what's going onfilestem was often called the crate name even though it was
unnormalized)
is_build_scripthelpercalculating the filestem
crate_nameargumentsThe short-term goal is slightly better correctness around crate-name handling, and easier to understand code, but the long-term goal for this is to be able to further split up the crate graph by removing dependencies on TyCtxt that only need the crate name, such as in
rustc_attr_parsing.I promise very hard that I did not change any behavior in this PR. Here is a transcript of some existing behavior on nightly: