[theemathas]

View all comments

This lint is similar to the existing let_underscore_lock lint, but applies to DropGuard instead of MutexGuard and similar types. It is deny-by-default, like let_underscore_lock. (Note that the related let_underscore_drop lint is allow-by-default.)

Relevant discussion: #t-libs > Linting `let _ = DropGuard::new(....);`

Note that DropGuard is a newly stabilized type, currently in 1.100 beta, and will be on the 1.100 stable release on November 12.

Tracking issue for DropGuard: #144426

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
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 18 candidates

[theemathas]

Lang-nominating to ask for approval for adding a new lint. Also, does it make sense to beta-backport this so that we have the lint at the same time as DropGuard being stabilized? (Although based on zulip discussion, it seems to me like T-libs doesn't want this to be backported, which I find a bit surprising.)

[Jules-Bertholet]

As discussed in the Zulip thread, perhaps we want a more general mechanism here?

[scottmcm]

I agree it makes sense to have a lint to catch this kind of problem with drop guards for the same reason as mutex guards.

@rfcbot fcp merge lang

I see the note that a more general mechanism might be nice, and I don't disagree, but we can always remove any behaviour from this lint in future should the general mechanism be created, so I don't see a need to reject this one. If we end up with another dead name in the removed-lints list eventually, so be it.

---

EDIT, speaking just for me not as part of the FCP: I wish we had a way to approve the general intent here -- the drop is meaningful so not doing it early is important and thus linting is fruitful -- but leave the details of how that's accomplished out of FCPing so that others could tune the details over time independently. I don't think lang needs to decide "one lint or multiple", for example.

[rust-rfcbot]

@scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[programmerjake]

Although based on zulip discussion, it seems to me like T-libs doesn't want this to be backported, which I find a bit surprising.

I think this should be backported. Do you have a link to where some of T-libs disagrees? (I happen to be on T-libs but not on the FCP list.)
Edit: the link #t-libs > Linting `let _ = DropGuard::new(....);` @ 💬

[petrochenkov]

As discussed in the Zulip thread, perhaps we want a more general mechanism here?

I agree with this, MutexGuard previously, DropGuard now, some other type next, but they are all the same pattern.

[traviscross]

Suggested change

	pub LET_UNDERSCORE_DROP_GUARD,
	pub LET_UNDERSCORE_DROP_GUARDS,

Given our RFC 0344 guidance, this should be named in the plural. (I.e., it reads more naturally as "allow let underscore drop guards".)

Though, here, annoyingly, that puts it at odds with let_underscore_lock. Still, I'd err on the side of following our convention and treating the other one as a miss.

View changes since the review

[theemathas]

I thought about this already and I disagree. Adding an s there implies that there are multiple drop guards. The thing that's logically "plural" is "let statements", but I don't think we should needlessly add the word "statements" either.

[traviscross]

Thanks. Here's how I'd frame what I understand to be your reading and, conversely, how I read it.

You're, I think, reading this as a transcription of the statement form: let _: DropGuard. In this framing, the part on the right names a token-like part of one construct rather than the class. We see this also in while_true, dyn_drop, unused_unsafe, break_with_label_and_loop, etc. So there's good precedent for it. And it's correct that those should not be pluralized.

The way I was reading it is as follows: "drop guard" is the head noun (i.e., the main thing) and "let underscore" is the attributive modifier (i.e., the adjective, essentially). (In unused_variables, "variables" is the head noun and "unused" is the attributive modifier.) So I read "drop guards (in let underscore position)" as the thing that we're allowing, which leads to my sense of why, in this reading, it should be plural, since it does name the class.

But I buy your reading here too, so I'll check a box.

[traviscross]

Thanks @theemathas.

One thing I wanted to check here is that this doesn't fire on let _ = &DropGuard::new(…);, since then the guard isn't actually dropped at the semicolon due to temporary lifetime extension. It's not, in my testing. It might be good to add a test for this.

And I notice, in my testing, that it does fire, as a false positive, when _ refers to a place with a DropGuard (where the guard also isn't dropped). Do we expect any problems from that?

@rfcbot reviewed

[nikomatsakis]

@rfcbot reviewed

[tmandry]

Happy to have this lint. For backport: Sure, but it would be fine not to. It's not lang's call ultimately. I also don't think we need to delay stabilization of DropGuard simply because of this lint not being merged.

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[theemathas]

@traviscross

One thing I wanted to check here is that this doesn't fire on let _ = &DropGuard::new(…);, since then the guard isn't actually dropped at the semicolon due to temporary lifetime extension. It's not, in my testing. It might be good to add a test for this.

I'm not particularly convinced that this is a useful test. The lint fires when the type of the _ pattern is equal to DropGuard<_, _>. It doesn't fire here because &DropGuard<_, _> is not DropGuard<_, _>, regardless of whether there's lifetime extension or not. For the same reason, it won't fire on an Option<DropGuard<_, _>> even though there's an argument to be made that maybe it should lint there.

And I notice, in my testing, that it does fire, as a false positive, when _ refers to a place with a DropGuard (where the guard also isn't dropped). Do we expect any problems from that?

This is a preexisting issue. I've filed #164746. I don't think such cases are likely to occur in real code, and the suggestion given by the lint isn't actively bad in some subtle way.

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit 73d8ca8 has been approved by petrochenkov

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #165283 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#165283), which was unapproved.

View changes since this unapproval

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[theemathas]

I've added a cfg_attr(bootstrap, ....) thing that hopefully fixes the test. There's some existing similar code, such as

rust/compiler/rustc_lint/src/unmarked_unchecked_type_aliases.rs

Lines 14 to 15 in 4c46afa

	#[cfg_attr(bootstrap, doc = "```rust,ignore")]
	#[cfg_attr(not(bootstrap), doc = "```rust")]

. Let's see if that fixes the issue.

@bors try jobs=test-x86_64-gnu-llvm-22-3

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2b30994 (2b309946eb7d60e31fd15585bcb6f610673a2d06)
Base parent: 4c46afa (4c46afa63688fb473618f9b3553d70b0c15a88eb)

[theemathas]

@rustbot ready