Skip to content

Conversation

JohnTitor
Copy link
Member

@JohnTitor JohnTitor commented May 5, 2022

In the block we don't know if the method actually exists thus expect_local panics.
Fixes #96738
Fixes #96583

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

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 May 5, 2022
@SNCPlay42
Copy link
Contributor

I don't think this code should need a local ID, it should be rewritten to be able to handle non-local definitions.

@JohnTitor
Copy link
Member Author

Though it's more of an enhancement thing rather than bugfix, I guess.

@petrochenkov
Copy link
Contributor

I also think it would be better to support this for extern ids.
Once we know that the def_id is DefKind::Ctor we can take a tcx.parent(def_id) from it and then use tcx.associated_item_def_ids(parent_def_id).len() to obtain the number of fields.

@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 May 6, 2022
@JohnTitor
Copy link
Member Author

Hmm, I may be misunderstanding your suggestion.
This:

let suggest = if let ty::FnDef(def_id, _) = rcvr_ty.kind()
                            && let DefKind::Ctor(of, _) = tcx.def_kind(def_id)
                        {
                            let parent_def_id = tcx.parent(*def_id);
                            Some((tcx.associated_item_def_ids(parent_def_id).len(), of))
                        } else {
                            None
                        };

causes panic on tcx.hir().expect_item(def_id.expect_local()) of associated_item_def_ids (we would assume def_id is local anyway?). Any idea?

@petrochenkov
Copy link
Contributor

Sigh, looks like the local version of associated_item_def_ids doesn't work with fields as is.
Could you then use associated_item_def_ids for extern def-ids only, and use the old logic for local def-ids?

@JohnTitor
Copy link
Member Author

Added the implementation.

@Badel2
Copy link
Contributor

Badel2 commented May 6, 2022

This also fixes #96583, which is the same issue

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 6, 2022

📌 Commit 35d77c1 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 May 6, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 6, 2022
Fix an ICE on rust-lang#96738

In the block we don't know if the method actually exists thus `expect_local` panics.
Fixes rust-lang#96738
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2022
…piler-errors

Rollup of 8 pull requests

Successful merges:

 - rust-lang#96660 ([bootstrap] Give a better error when trying to run a path with no registered step)
 - rust-lang#96701 (update `jemallocator` example to use 2018 edition import syntax)
 - rust-lang#96746 (Fix an ICE on rust-lang#96738)
 - rust-lang#96758 (bootstrap: bsd platform flags for split debuginfo)
 - rust-lang#96778 (Remove closures on `expect_local` to apply `#[track_caller]`)
 - rust-lang#96781 (Fix an incorrect link in The Unstable Book)
 - rust-lang#96783 (Link to correct issue in issue-95034 known-bug)
 - rust-lang#96801 (Add regression test for rust-lang#96319)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 825dc80 into rust-lang:master May 7, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 7, 2022
@JohnTitor JohnTitor deleted the issue-96738 branch May 7, 2022 09:47
Badel2 added a commit to Badel2/rust that referenced this pull request May 8, 2022
PR rust-lang#96746 fixed a very similar bug, so the same logic is used in a
different place.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 9, 2022
…er-errors

Actually fix ICE from rust-lang#96583

PR rust-lang#96746 fixed a very similar bug, so the same logic is used in a different place.

I originally concluded that the two issues (rust-lang#96583 and rust-lang#96738) were identical by comparing the backtrace, but I didn't look close enough.
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
7 participants