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.

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

Fix wrong argument for get_fn_decl #129223

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Fix wrong argument for get_fn_decl #129223

merged 1 commit into from
Aug 19, 2024

Conversation

wafarm
Copy link
Contributor

@wafarm wafarm commented Aug 18, 2024

Closes #129215 (seems to be introduced in #129168)

@rustbot
Copy link
Collaborator

rustbot commented Aug 18, 2024

r? @fmease

rustbot has assigned @fmease.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2024
@@ -554,7 +562,9 @@ impl<'hir> Map<'hir> {
/// }
/// ```
pub fn get_fn_id_for_return_block(self, id: HirId) -> Option<HirId> {
let enclosing_body_owner = self.tcx.local_def_id_to_hir_id(self.enclosing_body_owner(id));
// Id may not have a owner if it's a fn itself
Copy link
Member

Choose a reason for hiding this comment

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

Remark (not-blocking, just thinking): I almost wonder if this indicates a logic problem in whichever analysis requires get_fn_id_for_return_block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/// Given a `HirId`, return the `HirId` of the enclosing function, its `FnDecl`, and whether a
/// suggestion can be made, `None` otherwise.
pub fn get_fn_decl(
&self,
blk_id: HirId,
) -> Option<(LocalDefId, &'tcx hir::FnDecl<'tcx>, bool)> {
// Get enclosing Fn, if it is a function or a trait method, unless there's a `loop` or
// `while` before reaching it, as block tail returns are not available in them.
self.tcx.hir().get_fn_id_for_return_block(blk_id).and_then(|item_id| {

I see this function calls get_fn_id_for_return_block directly, but I guess from the name that this function should handle fn HirId inputs. Should I add an extra check here and keep get_fn_id_for_return_block unchanged instead?

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a super strong opinion on this myself, I would wait for @fmease to discuss what we feel like these helpers should or should not be doing.

@Noratrieb
Copy link
Member

I haven't bisected or tested it but it seems likely that #123812 regressed it. Even if not, @compiler-errors touched this recently and can probably judge what it should be.

@compiler-errors
Copy link
Member

@Noratrieb: #129168 regressed this more likely, given the timing. cc @BoxyUwU

I'll review this anyways, but there's likely a better place to put the check.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

I actually think that this ICE fix is kinda over-engineered. I think it's much simpler to simply pass the right HIR id to get_fn_decl:

if let Some((fn_id, fn_decl, can_suggest)) = fcx.get_fn_decl(parent_id)

Instead of passing parent_id, pass block_or_return_id.

This will regress the suggestions for a single test, which you're welcome to fix, but if you don't want to, I can work on it as a follow-up.

Well won't this fix just cause us to fall back into this issue next time someone uses get_fn_decl incorrectly?

Well, so, I don't really have a lot of interest in us making these functions less ICEy, since them being ICEy is often a good signal for the code needing to be reworked in general (like #123812 tried to do) that I'd rather not paper over with better checks that cause these functions to silently fail.

@rustbot rustbot 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 Aug 18, 2024
@wafarm wafarm changed the title Check if enclosing body owner exists in get_fn_id_for_return_block Fix wrong argument for get_fn_decl Aug 19, 2024
@wafarm
Copy link
Contributor Author

wafarm commented Aug 19, 2024

Thank you! I will open another pull request to fix the regression of add_semicolon_non_block_closure.rs later.

@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 Aug 19, 2024
@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2024

📌 Commit da7dd43 has been approved by compiler-errors

It is now in the queue for this repository.

@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 Aug 19, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#129194 (Fix bootstrap test `detect_src_and_out` on Windows)
 - rust-lang#129217 (safe transmute: check lifetimes)
 - rust-lang#129223 ( Fix wrong argument for `get_fn_decl`)
 - rust-lang#129235 (Check that `#[may_dangle]` is properly applied)
 - rust-lang#129245 (Fix a typo in `rustc_hir` doc comment)
 - rust-lang#129271 (Prevent double panic in query system, improve diagnostics)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5ba877c into rust-lang:master Aug 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2024
Rollup merge of rust-lang#129223 - wafarm:fix-129215, r=compiler-errors

 Fix wrong argument for `get_fn_decl`

Closes rust-lang#129215 (seems to be introduced in rust-lang#129168)
@wafarm wafarm deleted the fix-129215 branch August 28, 2024 08:28
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…mpiler-errors

Don't suggest adding return type for closures with default return type

Follow up of rust-lang#129223

r? `@compiler-errors`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 11, 2024
…mpiler-errors

Don't suggest adding return type for closures with default return type

Follow up of rust-lang#129223

r? ``@compiler-errors``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
Rollup merge of rust-lang#129260 - wafarm:dont-suggest-closures, r=compiler-errors

Don't suggest adding return type for closures with default return type

Follow up of rust-lang#129223

r? ``@compiler-errors``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

ICE: no enclosing_body_owner for hir_id
7 participants