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

diagnostics: make paths to external items more visible #32439

Merged
merged 2 commits into from
Mar 31, 2016

Conversation

jseyfried
Copy link
Contributor

This PR changes the reported path for an external item so that it is visible from at least one local module (i.e. it does not use any inaccessible external modules) if possible. If the external item's crate was declared with an extern crate, the path is guarenteed to use the extern crate.

Fixes #23224, fixes #23355, fixes #26635, fixes #27165.

r? @nrc

@alexcrichton
Copy link
Member

Holy cow I thought I'd never live to see the day this got implemented.

Thank you @jseyfried!

🍰

@jseyfried jseyfried force-pushed the visible_suggestions branch 2 times, most recently from 6dfa268 to 4f87af1 Compare March 23, 2016 06:44
@mitaa
Copy link
Contributor

mitaa commented Mar 23, 2016

Nice!

FYI, with #32293 there is also a refactoring going on here which lands soon. (8500e5b, ...)

@jseyfried jseyfried force-pushed the visible_suggestions branch from 4f87af1 to 62d6ea4 Compare March 23, 2016 07:51
@@ -244,6 +247,52 @@ impl CStore {
{
self.extern_mod_crate_map.borrow().get(&emod_id).cloned()
}

pub fn visible_parent_map<'a>(&'a self) -> ::std::cell::RefMut<'a, DefIdMap<DefId>> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the RefMut?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RefCell has to be borrowed mutably in this function and I didn't want to needlessly re-borrow it immutably before returning. This function is only used in librustc_metadata so I don't think it makes much difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document this function please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@jseyfried jseyfried force-pushed the visible_suggestions branch from 62d6ea4 to 82da520 Compare March 23, 2016 08:56
};

let croot = DefId { krate: cnum, index: CRATE_DEF_INDEX };
for child in self.crate_top_level_items(cnum) { add_child(bfs_queue, child, croot); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spread over 3 lines

@nrc
Copy link
Member

nrc commented Mar 24, 2016

r+ with the nits addressed

@jseyfried jseyfried force-pushed the visible_suggestions branch 3 times, most recently from ccc40dc to 17e8ce8 Compare March 25, 2016 06:53
@jseyfried
Copy link
Contributor Author

@nrc thanks for the feedback! I addressed your comments in the most recent commit.

We should probably wait for #32293 to land before merging this since there will be non-trivial conflicts (cc @nikomatsakis).

@bors
Copy link
Contributor

bors commented Mar 26, 2016

☔ The latest upstream changes (presumably #32293) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the visible_suggestions branch 6 times, most recently from e43422e to 6ac1799 Compare March 29, 2016 06:55
@eddyb
Copy link
Member

eddyb commented Mar 29, 2016

@jseyfried I think this can be r=nrc with src/test/pretty/issue-4264.pp updated to use the new paths (see travis failure).

@jseyfried jseyfried force-pushed the visible_suggestions branch 3 times, most recently from 8183ec4 to d5ea770 Compare March 30, 2016 01:05
@jseyfried jseyfried force-pushed the visible_suggestions branch from d5ea770 to da41e58 Compare March 30, 2016 22:03
@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Mar 30, 2016

📌 Commit da41e58 has been approved by nrc

@bors
Copy link
Contributor

bors commented Mar 31, 2016

⌛ Testing commit da41e58 with merge 4583dc9...

bors added a commit that referenced this pull request Mar 31, 2016
diagnostics: make paths to external items more visible

This PR changes the reported path for an external item so that it is visible from at least one local module (i.e. it does not use any inaccessible external modules) if possible. If the external item's crate was declared with an `extern crate`, the path is guarenteed to use the `extern crate`.

Fixes #23224, fixes #23355, fixes #26635, fixes #27165.

r? @nrc
@bors
Copy link
Contributor

bors commented Mar 31, 2016

💥 Test timed out

@eddyb
Copy link
Member

eddyb commented Mar 31, 2016

@bors force retry

@eddyb
Copy link
Member

eddyb commented Mar 31, 2016

@bors p=1

@alexcrichton
Copy link
Member

This ended up passing all tests on bors, not sure what happened here so I'm just gonna merge manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants