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

rustc: remove unnecessary extern_prelude logic from ty::item_path. #56655

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@eddyb
Copy link
Member

eddyb commented Dec 9, 2018

The checks added in 02357e4 effectively turned crate::std into std, but they were too general (affecting any crate::foo where foo was in the extern prelude, not just extern crates), and unnecessary, as only the extern crates created by "std injection" need any special-casing.

I've replaced the check for an extern prelude name with not printing the full path to an extern crate if the span inside the middle::cstore::ExternCrate is a dummy one.

Since this only affects the user-facing "relative" mode, it shouldn't have interactions with linking, and the only observable effect should be sometimes-shorter paths in diagnostics.

r? @nikomatsakis cc @davidtwco

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

Do we want some sort of test?

r=me with the comments + (ideally) a test

direct: true,
span,
..
}) if !span.is_dummy() => {

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 10, 2018

Contributor

I think a comment would be good here -- it's pretty non-obvious what's going on.

..
}) => {
debug!("try_push_visible_item_path: def_id={:?}", def_id);
self.push_item_path(buffer, def_id, pushed_prelude_crate);
if !span.is_dummy() {

This comment has been minimized.

@nikomatsakis

nikomatsakis Dec 10, 2018

Contributor

Similarly, comment here -- or maybe we can pull this into some kind of common helper function with the code above?

This comment has been minimized.

@eddyb

eddyb Dec 10, 2018

Member

So in a different branch I was able to deduplicate this by moving the local mode logic from push_krate_path to try_push_visible_item_path and relying on the fact that try_push_visible_item_path can override the behavior of push_item_path. But maybe we can avoid the dummy span check here by some other means.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 10, 2018

@eddyb

I guess that this means that if you have an extern crate and something in the prelude, you will get .. what?

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 10, 2018

@nikomatsakis You will get whatever is preferred by the rest of the system, likely the extern crate (with a path like crate::foo) - maybe we can fix all of this separately by making extern crate items unpreferred when they're in the extern prelude?

cc @petrochenkov

@petrochenkov petrochenkov self-assigned this Dec 10, 2018

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 11, 2018

@eddyb
Values in the GlobalCtxt::extern_prelude map are bool flags telling whether the crate was introduced by an extern crate item or not.
(Not accidentally introduced, like passed with both --extern and extern crate foo;, but really introduced only with extern crate foo;.)

I'm not sure what exactly ty::item_path needs, but hope that helps.

@petrochenkov petrochenkov removed their assignment Dec 11, 2018

@eddyb

This comment has been minimized.

Copy link
Member

eddyb commented Dec 11, 2018

@petrochenkov That's perhaps useful, but what's needed here is something tied to the CrateNum, not (just) the name of the crate, i.e. what this method records:

fn update_extern_crate(&mut self,

And its calls in that module, e.g.:
self.update_extern_crate(
cnum,
ExternCrate {
src: ExternCrateSource::Path,
span,
// to have the least priority in `update_extern_crate`
path_len: usize::max_value(),
direct: true,
},
&mut FxHashSet::default(),
);

Right now we track what caused a crate to be loaded, which used to always be an extern crate item, but I think with Rust 2018, that's the wrong perspective.

We should instead record whether the crate can be referred to by name (and specifically by which name), or that we have to spell out the path to an extern crate that loaded the crate.

ExternCrateSource appears to be used only in ty::item_path, so we don't have to consider other usecases if we change how this "simplest way to refer to a crate" tracking system works.

(random aside, ExternCrateSource::Use is dead, we should remove it, I might do it later)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 11, 2018

something tied to the CrateNum, not (just) the name of the crate

Then ExternCrate/ExternCrateSource tied to the CrateNum probably need to keep a flag similar to introduced_by_item in ExternPreludeEntry tied to the name.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 19, 2018

@eddyb @petrochenkov -- I can't quite tell what's the status of this PR. I wrote r=me above, modulo testing. Do we think we want to alter the behavior in this case, though?

I guess that this means that if you have an extern crate and something in the prelude, you will get .. what?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 29, 2018

Relevant issue - #55779 (infinite recursion in fn push_item_path).

While printing the trait helper::Trait the inner extern crate helper item is (incorrectly) considered "the introducer" of helper crate (ExternCrateSource::Extern), and to print the introducer we need to print its parent - the trait impl including helper::Trait, and so on.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 3, 2019

I'm just going to change this to r? @petrochenkov =)

@stokhos

This comment has been minimized.

Copy link

stokhos commented Jan 14, 2019

Ping from triage @petrochenkov : This PR requires your review.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 14, 2019

This PR requires eddyb's return.

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @eddyb: What is the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment