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.

Sign up for GitHub

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

Jump to bottom

Include ForeignItem when visiting types for WF check #98159

Merged
merged 1 commit into from
Jun 20, 2022
Merged

Include ForeignItem when visiting types for WF check #98159

merged 1 commit into from
Jun 20, 2022

Conversation

PrestonFrom
Copy link
Contributor

Addresses Issue 95665 by including hir::Node::ForeignItem as a valid
type to visit in diagnostic_hir_wf_check.

Fixes #95665

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 16, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 16, 2022
@PrestonFrom
Copy link
Contributor Author

This fixes the issue originally reported, but I'm wondering if the other variants in hir::Node should be valid to visit. The comment on line 120 seemed like it would justify visiting any Ty that could be extracted, but I definitely don't have a complete understanding of what is happening. Any guidance would be appreciated!

@petrochenkov
Copy link
Contributor

The full list can be found by searching for places where WellFormedLoc::Ty is created.
Looks like all nodes except for foreign statics were already processed correctly.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2022
@cjgillot
Copy link
Contributor

HIR WF check is just about getting a more precise diagnostic. Can't we just let the match be None instead of panicking? This would give us a sub-optimal diagnostic, without any chance of ICE.

@PrestonFrom
Copy link
Contributor Author

@cjgillot Apologies if this should be obvious, but I want to confirm that you mean the match on the ForeignItem kind and not the "parent" match on the WellFormedLoc.

@cjgillot
Copy link
Contributor

@PrestonFrom in the cases the inner match returns None, the outer one will return the same result, won't it? In doubt: I mean replacing the bug! into None, not changing the other cases that already return Some.

@PrestonFrom
Copy link
Contributor Author

@cjgillot The outer match currently uses bug! for the non-matching case, which I think is why it was suggested to use bug! here as well. So, I wasn't sure if you wanted to update both to return None or just the new addition.

@petrochenkov
Copy link
Contributor

r=me after squashing commits.

@PrestonFrom
Include ForeignItem when visiting types for WF check
f725b97 
Addresses Issue 95665 by including `hir::Node::ForeignItem` as a valid
type to visit in `diagnostic_hir_wf_check`.

Fixes rust-lang#95665
@PrestonFrom
Copy link
Contributor Author

r? @petrochenkov
Commits squashed and pushed

@PrestonFrom
Copy link
Contributor Author

r=@petrochenkov

@petrochenkov
Copy link
Contributor

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 20, 2022

📌 Commit f725b97 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 20, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request Jun 20, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 20, 2022
@bors
Auto merge of rust-lang#98292 - Dylan-DPC:rollup-hueb8tm, r=Dylan-DPC
b12708f 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#93080 (Implement `core::slice::IterMut::as_mut_slice` and `impl<T> AsMut<[T]> for IterMut<'_, T>`)
 - rust-lang#94855 (Panic when advance_slices()'ing too far and update docs.)
 - rust-lang#96609 (Add `{Arc, Rc}::downcast_unchecked`)
 - rust-lang#96719 (Fix the generator example for `pin!()`)
 - rust-lang#97149 (Windows: `CommandExt::async_pipes`)
 - rust-lang#97150 (`Stdio::makes_pipe`)
 - rust-lang#97837 (Document Rust's stance on `/proc/self/mem`)
 - rust-lang#98159 (Include ForeignItem when visiting types for WF check)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7bde23b into rust-lang:master Jun 20, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
Development

Successfully merging this pull request may close these issues.

Unexpected node ForeignItem with extern generic
6 participants
@PrestonFrom @rust-highfive @petrochenkov @cjgillot @bors @rustbot