Fix nested imports not included in the save_analysis output #47081
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (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. |
|
r? @nrc |
|
Thanks for the PR! We’ll periodically check in on it to make sure that @nrc or someone else from the team reviews it soon. |
kennytm
added
the
S-waiting-on-review
|
review ping @nrc ! pinging you on IRC too! |
|
@bors: r+ |
|
📌 Commit 9991fdf has been approved by |
|
@pietroalbini we don't really have tests for save-analysis beyond a smoke test to check we don't crash when running with -Zsave-analysis. I have some ideas for implementing tests, but it's kind of a lot of work. Let me know if you happen to be interested in implementing such a project. |
|
@nrc I'll see what I can do. Can you open an issue with your ideas? |
…r=nrc Fix nested imports not included in the save_analysis output This PR fixes rust-lang#46823. The bug was caused by the old access level checking code, which checked against the root UseTree even for nested trees. The problem with that is, for nested trees the root is lowered as an empty `ListStem`, which is not reachable by definition. The new code computes the access level with each tree's own ID, and with the root tree's visibility. I tested this manually and it works, but I'm not really satisfied with that. I looked at the existing tests though, and no one checked for the save_analysis output as far as I can see. How should I proceed with that? I think having a test about this would be really nice.
|
@pietroalbini sorry it took a while, notes are at #34481 (comment) |
This PR fixes #46823.
The bug was caused by the old access level checking code, which checked against the root UseTree even for nested trees. The problem with that is, for nested trees the root is lowered as an empty
ListStem, which is not reachable by definition. The new code computes the access level with each tree's own ID, and with the root tree's visibility.I tested this manually and it works, but I'm not really satisfied with that. I looked at the existing tests though, and no one checked for the save_analysis output as far as I can see. How should I proceed with that? I think having a test about this would be really nice.