remove let this = self; in compiler

[Kivooeo]

I still kept this as name for closure argument

I don't know original story why this was let this = self but I don't see any problems to remove it

by the way, there is one place, where it's actually necessary and provides a good solution because of macro https://github.com/rust-lang/rust/blob/main/compiler/rustc_parse/src/parser/expr.rs#L495

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[rustbot]

r? @saethlin

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

[bors]

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

[bjorn3]

For at least one case the method used to be a closure passed to self.in_scope(), with the closure argument being this: 066d44b?w=1#diff-e63c37c3db7192043330a40f97b301858cdba917d19dd968334f30ffdae34f9eR49

[Zalathar]

See how the code inside this closure uses this, but the code outside the closure uses self?

Being able to consistently use this in both places, regardless of whether they're nested in a closure, is the whole point of doing let this = self. So getting rid of it would make the code harder to read and harder to modify.

[Zalathar]

let this = self is a widely-used idiom throughout the MIR building code, because there are a lot of calls that need to borrow self and then re-expose it through a closure argument |this|.

Switching back to self at the top level creates inconsistency between code that happens to be outside or inside one of those closures. And it creates more churn when moving code in or out of a closure, which is a reasonably common thing to need to do.

So I don't think this PR should be merged.

[Kivooeo]

I'd agree if it wasn't like that in 7 places, using |this| to pass self in closure is a common practice overall in compiler and does not requires let this = self for this

for inconsistency part I don't agree because as I said places where self passed to closure like |this| way more common than let this = self

Here I want to show how inconsistent current version is (for info, there is no reason to use let this = self here, it does not affect anything, I assume CI passed means that I rewrote it correctly and logic wasn't touched)

This function uses self on top level in MIR building

rust/compiler/rustc_mir_build/src/builder/block.rs

Lines 12 to 31 in e1a2ec6

	impl<'a, 'tcx> Builder<'a, 'tcx> {
	pub(crate) fn ast_block(
	&mut self,
	destination: Place<'tcx>,
	block: BasicBlock,
	ast_block: BlockId,
	source_info: SourceInfo,
	) -> BlockAnd<()> {
	let Block { region_scope, span, ref stmts, expr, targeted_by_break, safety_mode: _ } =
	self.thir[ast_block];
	self.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| {
	if targeted_by_break {
	this.in_breakable_scope(None, destination, span, |this| {
	Some(this.ast_block_stmts(destination, block, span, stmts, expr, region_scope))
	})
	} else {
	this.ast_block_stmts(destination, block, span, stmts, expr, region_scope)
	}
	})
	}

And this function uses let this = self right down below in the same file

rust/compiler/rustc_mir_build/src/builder/block.rs

Lines 33 to 42 in e1a2ec6

	fn ast_block_stmts(
	&mut self,
	destination: Place<'tcx>,
	mut block: BasicBlock,
	span: Span,
	stmts: &[StmtId],
	expr: Option<ExprId>,
	region_scope: Scope,
	) -> BlockAnd<()> {
	let this = self;

And here is some more examples that not rename self for this

rust/compiler/rustc_mir_build/src/check_unsafety.rs

Line 291 in e1a2ec6

fn visit_block(&mut self, block: &'a Block) {

rust/compiler/rustc_mir_build/src/builder/matches/mod.rs

Line 413 in e1a2ec6

pub(crate) fn lower_match_arms(

rust/compiler/rustc_mir_build/src/thir/pattern/check_match.rs

Line 113 in e1a2ec6

fn visit_arm(&mut self, arm: &'p Arm<'tcx>) {

(And all other function in check_match.rs)

Moreover, if I remember it correctly, we are not allowing renaming self in arguments like for this reason? Just to keep code more readable and consistent, but this is opposite

The original idea for this PR comes from that I was reading this code that renaming self and it was hard to read

tldr; around 500 usages of |this| without let this = self and 7 with it does not make sense