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

Remove HasLocalDecls. #129681

Closed
wants to merge 1 commit into from
Closed

Conversation

nnethercote
Copy link
Contributor

It's a trait that lets you pass any of Body, LocalDecls, or IndexVec<Local, LocalDecl> to various functions. A minor convenience, and a minor obfuscation, that not actually needed.

This commit removes the trait and changes those functions to receive &LocalDecls instead of &D where D: HasLocalDecls. This makes lots of call sites slightly more verbose, but I think it's worth it for the overall simplicity.

r? @scottmcm

It's a trait that lets you pass any of `Body`, `LocalDecls`, or
`IndexVec<Local, LocalDecl>` to various functions. A minor convenience,
and a minor obfuscation, that not actually needed.

This commit removes the trait and changes those functions to receive
`&LocalDecls` instead of `&D where D: HasLocalDecls`. This makes lots of
call sites slightly more verbose, but I think it's worth it for the
overall simplicity.
@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 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 28, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

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.

This makes lots of call sites slightly more verbose, but I think it's worth it for the overall simplicity.

I disagree that this is worthwhile, and I think this existing abstraction improves code readability significantly in dense MIR like MIR typeck.

If you think this is making folks confused about what to pass in the argument, then I'd suggest leaving a doc comment on HasLocalDecls saying what implements it.

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Aug 28, 2024
@scottmcm
Copy link
Member

I'm happy to review this, but since I'm not actually on compiler contributors I think I'm poorly positioned to make a policy decision here on whether this is desirable, so I'll leave this to the team to talk about approach first.

@nnethercote
Copy link
Contributor Author

My original motivation was I had a chang (not in this PR) where a call to ty(body) didn't borrowck because another field of body was also being borrowed. Changing it to ty(&body.local_decls) fixed that borrowck error. Then I felt that HasLocalDecls was unnecessary cleverness, that often the simple obvious way to do things is the best.

@scottmcm: I asked you for review because you modified some of this code in #109819, though I guess that was part of a larger change.

@wesleywiser
Copy link
Member

We discussed in the weekly triage meeting but did not come to a consensus that the changes in this PR were more understandable than the current code. There seemed to be strong support for documenting how to deal with the borrowck error @nnethercote originally ran into in the doc comments for HasLocalDecls.

@scottmcm
Copy link
Member

scottmcm commented Aug 29, 2024

@nnethercote TBH I'm generally personally opposed to things like the "into trick", and thus have no issues with this change, I just feel like deciding that for rustc is outside my purview.

I only added the new HasLocalDecls in that PR because taking impl HasLocalDecls disables deref coercion, so the added impl is basically doing that coercion manually -- and losing coercions like this is one of the reasons that I tend to dislike passing parameters this way.

It does seem like perhaps not all the implementations make it into https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/trait.HasLocalDecls.html#implementors today? That seems suboptimal... No, I don't know what I'm thinking -- they're all there.

@nnethercote nnethercote deleted the rm-HasLocalDecls branch August 30, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-compiler-nominated Nominated for discussion during a compiler team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). 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.

5 participants