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

Fix ProjectionElem validation #96856

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Fix ProjectionElem validation #96856

merged 1 commit into from
Jul 7, 2022

Conversation

beepster4096
Copy link
Contributor

TypeChecker::visit_projection_elem was not actually being called.

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

r? @lcnr

(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 May 9, 2022
@beepster4096
Copy link
Contributor Author

It looks like this issue happened in #95320. I'm concerned that this hasn't been caught for months, I think there should be tests for the validator.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@beepster4096
Copy link
Contributor Author

cc @JakobDegen, this touches your field validation logic

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
JakobDegen
JakobDegen reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@JakobDegen JakobDegen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @oli-obk who reviewed my PR

compiler/rustc_const_eval/src/transform/validate.rs Outdated
match parent_ty.ty.kind() {

let kind = match parent_ty.ty.kind() {
ty::Opaque(def_id, _) => self.tcx.type_of(def_id).kind(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this at the very least need a .subst() call with the substitutions from the Opaque type? In any case though, I'm also concerned that this can lead to query cycles. Probably the fact that this is necessary at all is a sign of a problem elsewhere, I don't think Opaque is supposed to be allowed to appear here. I'd run into this with SetDiscriminant too a while back

compiler/rustc_const_eval/src/transform/validate.rs Outdated
let Some(layout) = self.tcx.generator_layout(*def_id) else {
self.fail(location, format!("No generator layout for {:?}", parent_ty));
return;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a good point, the previous version of this was wrong, got some wires crossed there. That being said, this causing query cycles is not indicative of a bug I think, it's sort of to be expected I think. Probably this just means we can't do any checking in this branch and should just skip it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The query cycle is because I messed up here, instead of going through a query, it should be getting the generator layout directly through the MIR body. I'll fix it soon.

Copy link
Contributor

@JakobDegen JakobDegen May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how. There's no guarantee that the body gives GeneratorInfo for the generator type (and this can happen in practice if the generator resume call is inlined)

Although, maybe it's possible to do "if GeneratorLayout is unavailable from this body, get it via query." That still seems potentially subtle though...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented May 30, 2022

☔ The latest upstream changes (presumably #97025) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr
lcnr reviewed May 31, 2022
View reviewed changes
compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/transform/validate.rs Outdated

let kind = match parent_ty.ty.kind() {
ty::Opaque(def_id, substs) => {
self.tcx.type_of(def_id).subst(self.tcx, substs).kind()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that could cause query cycle if it were called before mir_borrowck afaict 🤔

It's surprising to me that this doesn't already happen 😅 Do you know why this is fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how or why this works. It just looked correct and happened to work without breaking anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk do you have an opinion about that?

@lcnr lcnr 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 8, 2022
@JohnCSimon
Copy link
Member

Ping from triage:
@drmeepster can you address the comments from the reviewer and fix the merge conflict?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@Dylan-DPC
Copy link
Member

@drmeepster any updates on this?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@beepster4096 beepster4096 force-pushed the fix_projection_validation branch 2 times, most recently from c46c41b to 03e0f5f Compare July 4, 2022 08:31
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@beepster4096
Copy link
Contributor Author

@rustbot ready

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

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some final nits, then r=me

thanks for the patience and good work 😅 ❤️

compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved
src/test/ui/mir/ssa-analysis-regression-50041.rs Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lcnr
Copy link
Contributor

lcnr commented Jul 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2022

📌 Commit 7ce85a4c14fad79f1734fcb311744089b7313ef7 has been approved by lcnr

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2022
@bors
Copy link
Contributor

bors commented Jul 6, 2022

☔ The latest upstream changes (presumably #98206) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2022
@lcnr
Copy link
Contributor

lcnr commented Jul 6, 2022

while rebasing, can you also squash some of the commits?

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 6, 2022

✌️ @drmeepster can now approve this pull request

@beepster4096
Copy link
Contributor Author

@bors r=Icnr

@bors
Copy link
Contributor

bors commented Jul 6, 2022

📌 Commit a2799b2 has been approved by Icnr

@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 Jul 6, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 7, 2022
@Dylan-DPC
Rollup merge of rust-lang#96856 - DrMeepster:fix_projection_validatio…
7ede77f 
…n, r=Icnr

Fix ProjectionElem validation

`TypeChecker::visit_projection_elem` was not actually being called.
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 7, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 7, 2022
@Dylan-DPC
Rollup merge of rust-lang#96856 - DrMeepster:fix_projection_validatio…
7a9ca42 
…n, r=Icnr

Fix ProjectionElem validation

`TypeChecker::visit_projection_elem` was not actually being called.
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 7, 2022
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 7, 2022
@Dylan-DPC
Rollup merge of rust-lang#96856 - DrMeepster:fix_projection_validatio…
cc0d421 
…n, r=Icnr

Fix ProjectionElem validation

`TypeChecker::visit_projection_elem` was not actually being called.
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 7, 2022
Closed
@Dylan-DPC Dylan-DPC mentioned this pull request Jul 7, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 7, 2022
@bors
Auto merge of rust-lang#99014 - Dylan-DPC:rollup-n84y0jk, r=Dylan-DPC
3e51277 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#96856 (Fix ProjectionElem validation)
 - rust-lang#97711 (Improve soundness of rustc_arena)
 - rust-lang#98507 (Finishing touches for `#[expect]` (RFC 2383))
 - rust-lang#98692 (rustdoc: Cleanup more FIXMEs)
 - rust-lang#98901 (incr: cache dwarf objects in work products)
 - rust-lang#98930 (Make MIR basic blocks field public)
 - rust-lang#98973 (Remove (unused) inherent impl anchors)
 - rust-lang#98981 ( Edit `rustc_mir_dataflow::framework` documentation )

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f6bbe28 into rust-lang:master Jul 7, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 7, 2022
This was referenced Jul 28, 2022
Closed
Closed
@ehuss ehuss mentioned this pull request Aug 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

@JakobDegen JakobDegen JakobDegen left review comments

Assignees

@lcnr lcnr

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.64.0
Development

Successfully merging this pull request may close these issues.
9 participants
@beepster4096 @rust-highfive @rust-log-analyzer @bors @JohnCSimon @Dylan-DPC @lcnr @JakobDegen @rustbot