[yashaggarwal85d]

View all comments

let _ = y; where y is a local guard doesn't move or drop y, but
let_underscore_lock (deny by default) says the lock is released immediately and suggests
drop(y), which would change behaviour. Same for let_underscore_drop.

This skips the lint when the let initializer is a place expression. Temporaries still lint,
including fields of temporaries like (m.lock(), 1).0.

Fixes #164746
cc @theemathas @traviscross

[rustbot]

r? @mu001999

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 19 candidates

[mu001999]

@rustbot author

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[mu001999]

@rustbot author

View changes since this review

[oli-obk]

Reopening after a conversation with the author

[mu001999]

@bors try

[yashaggarwal85d]

Two more false positives are still left, and I’m not sure they’re worth the extra code in this PR:

let (_, &_) = (1, &m.lock().unwrap()); // borrow inside a tuple
let S { ref a, b: _ } = temp(); // `ref a` extends the temporary

In both cases the lock stays held, but it still lints. Should I add them to the FIXME on is_local_or_static_place, or try to handle them here?

[rust-bors]

☀️ Try build successful (CI)
Build commit: c3821d7 (c3821d702a38d0d602d3913d7b1c7994ff40a615)
Base parent: 96a18bf (96a18bf5ffa2b24f9cf3d1c731a9eb121c1acad9)

[mu001999]

@rust-timer queue

Let's check the perf first.

[mu001999]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 240909c (240909cad68106ddfbf201b18110c2696a26319f)
Base parent: 96a18bf (96a18bf5ffa2b24f9cf3d1c731a9eb121c1acad9)

[rust-timer]

Finished benchmarking commit (240909c): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.3%, 3.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.4%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -3.1%, secondary 1.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.7%	[1.4%, 2.0%]	2
Improvements ✅ (primary)	-3.1%	[-3.6%, -2.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.1%	[-3.6%, -2.6%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.353s -> 486.474s (-0.18%)
Artifact size: 406.68 MiB -> 406.87 MiB (0.05%)

[mu001999]

Could we achieve this by simply checking the pattern is ref and the ty needs drop or is lock guard? 🤔

View changes since the review

[yashaggarwal85d]

I thought about that, but only checking for a ref pattern would stop linting cases where the lock really is dropped, like let &_ = identity(&m.lock()) or let &&_ = &identity(&m.lock()). Happy to switch to the simpler check if you think those false negatives are fine though

[mu001999]

Thanks for the trying! IMO, borrows_extended_temporary is a little complicated and not complete enough, I tend to remove it and keep the check simple.

I'll approve this after the removal, but I'd like to refer it to another reviewer for a second opinion. So you don't need to remove it for now.

r? estebank, since you approved #97739 :)