Refactor crate name handling; pass a TyCtxt into macro expansion#155844
Refactor crate name handling; pass a TyCtxt into macro expansion#155844jyn514 wants to merge 13 commits intorust-lang:mainfrom
Conversation
rustbot
added
S-waiting-on-review
|
r? @eholk rustbot has assigned @eholk. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
This comment has been minimized.
This comment has been minimized.
This caught several bugs.
Using it is almost always a mistake. Get rid of the temptation. Rustdoc has to keep using it because it doesn't have a real crate name when parsing markdown files.
jyn514
force-pushed
the
crate-name-refactors
branch
from
a3fd25e to
8cbe665
Compare
| let exe_path = tcx.output_filenames(()).path(OutputType::Exe); | ||
| let exe_stem = exe_path.filestem().unwrap().to_string_lossy(); | ||
| config.args.insert(0, exe_stem.to_string()); |
There was a problem hiding this comment.
@saethlin already approved these changes in #153924 (comment).
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
No complaints from me, but one question about the motivation. #153924 said:
Some of the early commits (involving |
|
@nnethercote no, I gave up on that part of it. Now the motivation is just to have a consistent handling of crate-name so that it’s not implicit based on re-parsing the crate root over and over. |
|
|
||
| pub trait ResolverExpand { | ||
| pub trait ResolverExpand<'tcx> { | ||
| fn tcx(&self) -> TyCtxt<'tcx>; |
There was a problem hiding this comment.
I'd add this much earlier, if the required amount of additional lifetime annotations weren't so enormous. The resulting cleanups barely compensate it.
But I guess it needs to happen sooner or later, given the common trend of using tcx earlier.
There was a problem hiding this comment.
I suspect that we can actually allocate Resolver on tcx arena now, so ExtCtxt will contain pub resolver: &'tcx mut dyn ResolverExpand<'tcx> and the need in the second lifetime will disappear.
(Preferably in a separate PR, creating mega-PRs with combined different changes doesn't make sense if reviewers are consistently available rather than appear once in a few weeks. E.g. this PR currently hides a few meaningful changes among hundreds of added '_s.)
There was a problem hiding this comment.
it may be possible to make this function return a "short lived" TyCtxt<'_> without being an issue for the current use cases (they don't need to mutate via ResolverExpand after obtaining the TyCtxt<'_>. That would avoid back & forth churn if the lifetimes get merged in the future anyway, and anyone trying to use it more generally later would first need to do the big lifetime commit and not just be able to rely on the lifetime
| /// -> expn_data` of their expansion context stored into their span. | ||
| pub struct ExtCtxt<'a> { | ||
| pub struct ExtCtxt<'a, 'tcx> { | ||
| pub sess: &'a Session, |
There was a problem hiding this comment.
This field is no longer necessary.
| /// Operations on the crate name. | ||
| impl<'tcx> TyCtxt<'tcx> { | ||
| pub fn local_crate_name(self) -> Symbol { | ||
| self.crate_name(LOCAL_CRATE) |
There was a problem hiding this comment.
This is too trivial and doesn't deserve a separate function, IMO.
The remaining is_build_script can then be moved to one of the existing impl TyCtxt blocks.
| } | ||
|
|
||
| pub fn interface_path(&self) -> PathBuf { | ||
| debug!("using crate_name={} for interface_path", self.crate_stem); |
There was a problem hiding this comment.
Nit: could you remove the added debugging statements before merging, they are almost never useful outside of some specific debugging session.
| format!("{pkg_name} build script") | ||
| } else { | ||
| crate_name.to_string() | ||
| ecx.resolver.tcx().crate_name(LOCAL_CRATE).as_str().to_string() |
There was a problem hiding this comment.
| ecx.resolver.tcx().crate_name(LOCAL_CRATE).as_str().to_string() | |
| ecx.resolver.tcx().crate_name(LOCAL_CRATE).to_string() |
| validate_crate_name(sess, normalized, span); | ||
|
|
||
| CrateName { normalized, unnormalized: Symbol::intern(unnormalized) } | ||
| } |
There was a problem hiding this comment.
The from_(un)normalized are small, private to this file and used only once in another "constructor" for CrateName, I'd rather inline and remove them.
petrochenkov
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #155760) made this pull request unmergeable. Please resolve the merge conflicts. |
7 participants
Modified version of #153924 that doesn't try to change any dataflow, just clarify the existing behavior.
crate_namecleanups #155835, which has already been approved.TyCtxtinto macro expansion, and make some easy cleanups that are possible as a result. b84658a should be the only commit that changes behavior (in diagnostics), I kept it intentionally small.CrateNamestruct that documents the difference between the filestem and crate name. Previously the compiler confused the two (unsure if this was intentional, the code was quite complicated). I have preserved the behavior even though I'm not sure it's correct; a bunch of run-make tests depend on it.opts.crate_nameand mops up a last couple places that shouldn't have been using it.filestem()instead ofCrateName.I promise that I tried very hard not to change any behavior in this PR. Here is a transcript of some existing behavior on nightly: