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

restore TyCtxt::push_impl_path() logic for checking whether types are available #41983

Closed
wants to merge 1 commit into from

Conversation

dwrensha
Copy link
Contributor

Fixes #41697 by restoring some logic that got removed in 2cca256.

Note that I don't have a super strong grasp of what exactly is happening is this code. It's entirely possible that @nikomatsakis had a good reason for removing these lines, in which case maybe there is a better fix.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2017
@Mark-Simulacrum
Copy link
Member

Thanks for the PR! We'll periodically check in to make sure that @nikomatsakis or someone else from the team reviews this soon.

@oli-obk
Copy link
Contributor

oli-obk commented May 15, 2017

This should probably have a regression test so it doesn't regress again. Adding

// dump-mir as regression test for #41697
// compile-flags:-Zdump-mir=all

to src/test/run-pass/issue-33387.rs should to it.

@dwrensha
Copy link
Contributor Author

@oli-obk If I set that flag (on issue-33387.rs or issue-41697.rs) then running the test dumps a bunch of files like rustc.node15.000-000.SimplifyCfg-initial.after.mir in the project root directory. Maybe we could avoid that by also adding a -Zdump-mir-dir=DIR flag, but I'm unsure what directory would make sense to put there.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 15, 2017

Sadly, this fix isn't quite right. We shouldn't be poking directly at the fields of the maps structure (and, in fact, I'm surprised this even compiles, since they are supposed to be private). I have a (midly) better fix for #41697 specifically, though I think the best fix is to rewrite the item-path code more thoroughly. I was trying to decide if I could come up with a better unit test though and am not sure how best to do it.

@nikomatsakis
Copy link
Contributor

I'm going to close in favor of #42017 -- thanks @dwrensha !

@dwrensha dwrensha deleted the fix-41697 branch May 17, 2017 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in -Z dump-mir=all
5 participants