Replace Body::basic_blocks() with field access

[tmiasko]

Since the refactoring in #98930, it is possible to borrow the basic blocks
independently from other parts of MIR by accessing the basic_blocks field
directly.

Replace unnecessary Body::basic_blocks() method with a direct field access,
which has an additional benefit of borrowing the basic blocks only.

[rustbot]

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

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[JakobDegen]

It's not super clear to me that we want this. DerefMut is really easy to call accidentally, so this might be a footgun for people intending to use .as_mut_no_invalidate() or whatever it's called. Don't feel super strongly either way though

[pnkfelix]

I also do not see why we would do this.

(Are accessor methods like .basic_blocks_mut() considered an anti-pattern in Rust fashion today?)

[jackh726]

In fairness, Deref is already implemented. It seems a bit silly to have one but not the other?

[tmiasko]

As to the question why change this at all, I would like to encourage borrowing individual fields instead of the whole body and reduce the number of different ways to borrow the basic blocks.

I also think that it might be confusing to leave basic_blocks_mut(). If so far you only used basic_blocks_mut() to mutably borrow the basic blocks, you probably wouldn't guess how to say borrow mutably basic_blocks and local_decls both (note that the answer isn't basic_blocks_and_local_decls_mut() because that method is already gone).

[JakobDegen]

@jackh726 yeah, but the important difference is that DerefMut, like the .as_mut() that it calls, has a side-effect of invalidating the CFG cache. That's the only reason that this was ever behind a method anyway. I wouldn't mind having this if that wasn't the case.

@tmiasko I'm on board with getting rid of .basic_blocks_mut(). I just think that .as_mut() suffices and we don't need DerefMut

[tmiasko]

@JakobDegen informed by this refactoring, I think that if one knows about cache invalidation DerefMut is clear enough. If one isn't aware of cache invalidation aspect yet, then as_mut on its own offers no improvement.

I suspect that to make further improvements toward preserving cache validity we would need to go one step further. Avoid DerefMut, IndexMut (body still implements IndexMut<BasicBlock>), and as_mut. Instead using as_mut_preserves_cfg and as_mut_invalidates_cfg or so.

[JakobDegen]

The thing I'm more worried about is accidental calls from someone intending to use the no-cfg-invalidation stuff

[bors]

☔ The latest upstream changes (presumably #99592) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #99667) made this pull request unmergeable. Please resolve the merge conflicts.

[tmiasko]

r? rust-lang/mir-opt

[bors]

☔ The latest upstream changes (presumably #100087) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #100089) made this pull request unmergeable. Please resolve the merge conflicts.

[oli-obk]

Instead using as_mut_preserves_cfg and as_mut_invalidates_cfg or so.

I like this, as it makes it clear that there's something possibly surprising going on

[tmiasko]

I limited changes to basic_blocks() only and left out mutable variant for another pull request.

[oli-obk]

@bors r+

[bors]

📌 Commit b48870b has been approved by oli-obk

It is now in the queue for this repository.