Skip to content

Include ForeignItem when visiting types for WF check#98159

Merged
bors merged 1 commit intorust-lang:masterfrom
PrestonFrom:issue_95665
Jun 20, 2022
Merged

Include ForeignItem when visiting types for WF check#98159
bors merged 1 commit intorust-lang:masterfrom
PrestonFrom:issue_95665

Conversation

@PrestonFrom
Copy link
Copy Markdown
Contributor

@PrestonFrom PrestonFrom commented Jun 16, 2022

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
Copy Markdown
Contributor

rust-highfive commented Jun 16, 2022

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
Copy Markdown
Contributor Author

PrestonFrom commented Jun 16, 2022

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
Copy Markdown
Contributor

petrochenkov commented Jun 16, 2022

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 reviewed Jun 16, 2022
View reviewed changes
@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
Copy Markdown
Contributor

cjgillot commented Jun 16, 2022

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
Copy Markdown
Contributor Author

PrestonFrom commented Jun 16, 2022

@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
Copy Markdown
Contributor

cjgillot commented Jun 16, 2022

@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
Copy Markdown
Contributor Author

PrestonFrom commented Jun 16, 2022

@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
Copy Markdown
Contributor

petrochenkov commented Jun 18, 2022

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
Copy Markdown
Contributor Author

PrestonFrom commented Jun 20, 2022

r? @petrochenkov
Commits squashed and pushed

@PrestonFrom
Copy link
Copy Markdown
Contributor Author

PrestonFrom commented Jun 20, 2022

r=@petrochenkov

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented Jun 20, 2022

Thanks!
@bors r+ rollup

@bors
Copy link
Copy Markdown
Collaborator

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