Create const blocks on the fly during typeck instead of waiting until writeback. by oli-obk · Pull Request #125806 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits2 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This allows us to spawn a nested FnCtxt, creating a barrier that control flow statements cannot cross.
fixes #125670
The issues were caused by #124650 where I failed to account for the lack of changing scopes for const blocks
… writeback.
This allows us to spawn a nested FnCtxt, creating a barrier that control flow statements cannot cross.
r? @fee1-dead
rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
Comment on lines -906 to +912
| let encl_body_owner_id = self.tcx.hir().enclosing_body_owner(expr.hir_id); |
|---|
| // If this didn't hold, we would not have to report an error in |
| // the first place. |
| assert_ne!(encl_item_id.def_id, encl_body_owner_id); |
| assert_ne!(encl_item_id.def_id, self.body_id); |
| let encl_body = self.tcx.hir().body_owned_by(encl_body_owner_id); |
| err.encl_body_span = Some(encl_body.value.span); |
| err.encl_body_span = Some(self.tcx.def_span(self.body_id)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is because enclosing_body_owner does not yield const blocks anymore. I'm addressing this separately to remove this footgun, but the code here could have been written simpler anyway, so I just did that.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enclosing_body_owner being out of sync is somewhat sketch 💀 why doesn't it? it should be as much of a body as a closure is, for example.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change definitely improves things, but I'm still somewhat skeptical that we can't improve it more... Thoughts?
| @@ -76,7 +76,12 @@ impl<'hir> LoweringContext<'_, 'hir> { |
|---|
| ExprKind::Array(exprs) => hir::ExprKind::Array(self.lower_exprs(exprs)), |
| ExprKind::ConstBlock(c) => { |
| self.has_inline_consts = true; |
| hir::ExprKind::ConstBlock(self.lower_expr(c)) |
| self.with_new_scopes(e.span, |this |
| let coroutine_kind = this.coroutine_kind.take(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of strange that coroutine_kind is not handled in with_new_scopes. Is there some other with_body_*-like function that does the coroutine kind handling too?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been looking into this in a branch, but didn't want to add more stuff to this PR.
we can get it all working by merging with_new_scopes into lower_body, but it needs some managing of the right spans to maintain diagnostic quality.
Comment on lines -906 to +912
| let encl_body_owner_id = self.tcx.hir().enclosing_body_owner(expr.hir_id); |
|---|
| // If this didn't hold, we would not have to report an error in |
| // the first place. |
| assert_ne!(encl_item_id.def_id, encl_body_owner_id); |
| assert_ne!(encl_item_id.def_id, self.body_id); |
| let encl_body = self.tcx.hir().body_owned_by(encl_body_owner_id); |
| err.encl_body_span = Some(encl_body.value.span); |
| err.encl_body_span = Some(self.tcx.def_span(self.body_id)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enclosing_body_owner being out of sync is somewhat sketch 💀 why doesn't it? it should be as much of a body as a closure is, for example.
Comment on lines +1466 to +1468
| let feed = self.tcx().create_def(self.body_id, kw::Empty, DefKind::InlineConst); |
|---|
| feed.def_span(span); |
| feed.local_def_id_to_hir_id(hir_id); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like const blocks should be first-class nested bodies (liek closures) all the way from the AST-level (i.e. having a def id, being treated as a hir owner, etc) instead of having to synthesize a def-id here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's what they used to be (before #124650), but it meant we couldn't feed the type_of query. Turns out I couldn't do that anyway, so now I think I should probably just revert #124650 and try again from scratch
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps yeah
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though we'll def need to lazily create the DefId of anon consts of method calls' const generic params, so that we can feed type_of. That's why I'm trying to figure out how to do this better.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right now we are breaking query system invariants and are feeding anon const type_of from typeck, even though typeck did not create the DefId.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why do we need to feed the type_of query at all?
The type_of for a const block should operate like a type_of for a closure -- for closures, it calls typeck_results on the parent and extracts it from the expr_ty:
| Node::Expr(&Expr { kind: ExprKind::Closure { .. }, .. }) => { |
|---|
| tcx.typeck(def_id).node_type(hir_id) |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though we'll def need to lazily create the DefId of anon consts of method calls' const generic params, so that we can feed type_of. That's why I'm trying to figure out how to do this better.
I don't understand why this needs to be entangled with inline const exprs. Perhaps we should just distinguish these two rather than treating them the same.
Just gonna revert #124650 and start over
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…r-errors
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang#125846
r? @compiler-errors
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…r-errors
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang#125846
r? @compiler-errors
fmease added a commit to fmease/rust that referenced this pull request
…r-errors
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang#125846
r? @compiler-errors
bors added a commit to rust-lang-ci/rust that referenced this pull request
…errors
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang#125846
r? @compiler-errors
bors added a commit to rust-lang-ci/rust that referenced this pull request
…errors
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang#125846
r? @compiler-errors
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang/rust#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang/rust#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang/rust#125846
r? @compiler-errors
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request
Revert: create const block bodies in typeck via query feeding
as per the discussion in rust-lang/rust#125806 (comment)
It was a mistake to try to shoehorn const blocks and some specific anon consts into the same box and feed them during typeck. It turned out not simplifying anything (my hope was that we could feed type_of to start avoiding the huge HIR matcher, but that didn't work out), but instead making a few things more fragile.
reverts the const-block-specific parts of rust-lang/rust#124650
@bors rollup=never had a small perf impact previously
fixes rust-lang/rust#125846
r? @compiler-errors
Labels
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.