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 reachability analysis for const methods #97716

Merged
merged 1 commit into from
Jun 4, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jun 3, 2022

Use method_might_be_inlined directly for ImplItemKind::Fn instead of duplicating the logic in def_id_represents_local_inlined_item.

This is parallel to how we use item_might_be_inlined for ItemKind::Fn in that same body.

Fixes #97708

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

r? @wesleywiser

(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 3, 2022
@rust-log-analyzer

This comment has been minimized.

@compiler-errors compiler-errors force-pushed the issue-97708 branch 2 times, most recently from 67d63c8 to dfc9294 Compare June 4, 2022 00:37
@compiler-errors compiler-errors changed the title Encode MIR for 'unreachable' non-generic fns Fix reachability analysis for const methods Jun 4, 2022
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Looks good to me! I like this approach better than the initial one and the results for should_codegen_locally and should_encode_mir look correct to me.

@wesleywiser
Copy link
Member

Thanks @compiler-errors!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2022

📌 Commit 15cccb9 has been approved by wesleywiser

@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 Jun 4, 2022
@compiler-errors
Copy link
Member Author

Yeah the first approach was kinda wonky. It only later realized to me that we should be using method_might_be_inlined everywhere we see a ImplItemKind::Fn parallel to using item_might_be_inlined.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 4, 2022
…eywiser

Fix reachability analysis for const methods

Use `method_might_be_inlined` directly for `ImplItemKind::Fn` instead of duplicating the logic in `def_id_represents_local_inlined_item`.

This is parallel to how we use `item_might_be_inlined` for `ItemKind::Fn` in that same body.

Fixes rust-lang#97708
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#96642 (Avoid zero-sized allocs in ThinBox if T and H are both ZSTs.)
 - rust-lang#97647 (Lazily allocate and initialize pthread locks.)
 - rust-lang#97715 (Support the `#[expect]` attribute on fn parameters (RFC-2383))
 - rust-lang#97716 (Fix reachability analysis for const methods)
 - rust-lang#97722 (Tighten spans for bad fields in struct deriving `Copy`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9917f38 into rust-lang:master Jun 4, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 4, 2022
@matthiaskrgr
Copy link
Member

Hm, i had hoped that this would also fix #96857 but it seems it didn't :/ :sweat_smile:

@compiler-errors compiler-errors deleted the issue-97708 branch August 11, 2023 20:05
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 MIR available for DefId(...)"
7 participants