-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Fix #23808: dead_code lint: visit types in paths #27565
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (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. 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. |
So, I had a look at the travis logs and it looks like the pretty test failed because pretty forces a What would be the best solution to fix this? Disable the test for pretty? How do I do that? |
955c0f9
to
797acca
Compare
Ok, so I think I fixed the issue by making |
It would be nice if we didn't need |
I have no clue, this is the first time I have worked on the rust compiler. The only other alternative I can think of is to disable pretty for this test. Or is it possible to use |
797acca
to
9759fcb
Compare
Alright, I removed the pub again, this now has to wait for #27567 |
9759fcb
to
88881e9
Compare
Alright, now that #27571 is merged this should build. I just rebased & pushed again to get travis to test it again. |
Alright, this doesn't compile at the moment due to some recent rust changes, I'll fix this now. |
88881e9
to
ab22af6
Compare
Please don't r+ yet, I need to add another test case which came up on reddit: |
ab22af6
to
2d4e07e
Compare
Alright, I added the new test cases. They would probably not have been necessary, but I wanted to be on the safe side. |
@nrc: could you review again? |
📌 Commit 2d4e07e has been approved by |
⌛ Testing commit 2d4e07e with merge 3c35d55... |
💔 Test failed - auto-mac-32-opt |
@nrc could you retry? this passes make check locally (on linux) and I don't think there is any target dependent code in that part of the compiler. |
That doesn't look like it's spurious (may be #27583), but I don't think a retry will fix it. Can you reproduce locally? Perhaps on a 32-bit build? |
@alexcrichton I'm trying a local 32bit build now but I can't imagine how my changed could cause a test failure there, except if they just expose another bug. If this turns out to be #27583, how would I go about fixing this? Just wait until the issue is fixed or add one of the workarounds from the issue to |
Unfortunately I'm not sure if there's a fix, but #27856 is also block on this and @nikomatsakis indicates that #27892 may fix it. |
Thanks for the additional links! I'll see if the local build will result in anything and wait for #27892 otherwise. I never imagined that this rather small pull request would cause so much trouble, even if it did take some (rather enjoyable) hours to figuring this fix out. |
Alright, so the local 32bit linux build failed as well, so I'll wait on #27892 and then try again |
Since #27892 has landed now, could someone retry please? |
@bors: retry |
Fixes #23808, passes `make check-stage1` `run-pass` and `run-fail` locally.
This code was introduced in rust-lang#27565 to mark types in paths alive. It is now unnecessary since rust-lang#37676.
Fixes #23808, passes
make check-stage1
run-pass
andrun-fail
locally.