Add #[must_bind] attribute and lint #78715

vi · 2020-11-03T23:59:08Z

Prototype implementation of https://internals.rust-lang.org/t/pre-rfc-must-bind/12658

Like #[must_use], but also catches let _ =.

Intended to be used on semaphore guards and similar things where RAII guard is detached from the protected resource.
Beginner users often think that _ is just a funny variable name and the guard will be dropped at the end of block instead of immediately. This may introduce silent logic error into the code.

TODO:

Require #[feature] instead of insta-stable
Proper test
Add section to reference
Probably refactor the code
Decide on a specific lint text and attribute name
Detecting #[must_bind] without #[must_use] as error.
Automatically suggest an edit for cargo fix

Is this idea worth pursuing further or it's unlikely to go through even with the TODOs resolved?

rust-highfive · 2020-11-03T23:59:12Z

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

jyn514 · 2020-11-04T01:49:18Z

compiler/rustc_lint/src/unused.rs

+    /// [the reference]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_bind-attribute
+    pub UNUSED_MUST_BIND,
+    Warn,
+    "Bogus `_` binding of a type flagged as `#[must_bind]`. Use something like `_guard`.",


Suggested change

"Bogus `_` binding of a type flagged as `#[must_bind]`. Use something like `_guard`.",

"types marked as `#[must_bind]` should be assigned to a variable, not dropped immediately",

ghost · 2020-11-06T13:01:47Z

Did not know _ and _guard are different things. 💡
Would be nice to warn the user if they used the wrong variant.

I general (not necessarily with #[must_bind]) I think people should be made aware of this difference. Is this even mentioned in the book?

Mark-Simulacrum · 2020-11-06T16:30:58Z

I'm going to nominate for lang team because while I tend to think this is not the right fix (it's too similar to must_use, IMO), I think it may be worth trying to get a brief discussion at least on the _ vs _guard difference; I think I frequently see users (including myself) forgetting that these two are different.

vi · 2020-11-06T18:30:50Z

I think people should be made aware of this difference.

#[must_bind] attribute allows telling it to users at the moment where they need it the most, i.e. when dealing with content-less drop-only RAII guards (which some may consider an anti-pattern per se).

davidtwco

As Mark as noted, needs lang team involvement but w/r/t implementation:

In addition to the changes you've noted in the MR description, I've added some nitpicks below - but the implementation looks broadly correct to me.

davidtwco · 2020-11-08T12:17:25Z

compiler/rustc_lint/src/unused.rs

+                                descr_post_path
+                            ),
+                            WhatAttrToCheck::MustBind => format!(
+                                "Bogus `let _ =` binding of {}`{}`{}.  Use something like `let _guard =`.",


This should be changed to the same message suggested by this comment.

davidtwco · 2020-11-08T12:19:21Z

compiler/rustc_lint/src/unused.rs

+        let (expr, attr_to_check) = match s.kind {
+            hir::StmtKind::Semi(ref expr) => (&**expr, WhatAttrToCheck::MustUse),
+            hir::StmtKind::Local(ref local) => {
+                match local.pat.kind {


I'd prefer a guard in the match arm: if !matches!(local.pat.kind, hir::PatKind::Wild)

davidtwco · 2020-11-08T12:20:25Z

compiler/rustc_lint/src/unused.rs

-            });
-            op_warned = true;
-        }
+        match attr_to_check {


if matches!(attr_to_check, WhatAttrToCheck::MustUse) would be preferable

davidtwco · 2020-11-08T12:21:20Z

compiler/rustc_lint/src/unused.rs

-                    });
-                    true
-                }
+                ty::Closure(..) => match attr_to_check {


I'd prefer a guard in the match arm: if matches!(attr_to_check, WhatAttrToCheck::MustUse)

davidtwco · 2020-11-08T12:21:30Z

compiler/rustc_lint/src/unused.rs

+                    }
+                    _ => false,
+                },
+                ty::Generator(..) => match attr_to_check {


Likewise, guard in match arm.

davidtwco · 2020-11-08T12:23:46Z

compiler/rustc_lint/src/unused.rs

-        let expr = match s.kind {
-            hir::StmtKind::Semi(ref expr) => &**expr,
+        #[derive(Clone, Copy)]
+        enum WhatAttrToCheck {


I'd rename this to something like UnusedResultsAttr (bikeshedding welcome) and I prefer it declared outside of the function and impl.

vi · 2020-11-12T18:29:57Z

Note: #[must_not_await] implementation may also touch this code.
Shall this pull request somehow prepare for addition of the third checking mode?

jyn514 · 2020-11-12T20:08:09Z

@vi I think this change is small enough you don't have to worry about it, the author of must_not_await can take care of conflicts when they open their PR.

Mark-Simulacrum · 2020-11-23T22:48:54Z

We discussed this change in the last language team meeting. I think the general feeling is that we agree that this is a problem worth solving, but this does not quite feel like the right solution, or at least a broader exploration of the design space would be warranted first.

The next step here is likely to close this PR in favor of a future project group, which would explore the solution space a bit more before proposing a concrete solution. If you'd be interested in driving this project group, please start a thread in #t-lang on Zulip and we can coordinate on who will liaison the group and the charter drafting. The project group scope can be adjusted for how much time interested parties have to invest, but likely at least a solution space of fixing confusion around _ makes sense; an extension might be to look at temporary lifetime rules in general.

I'm leaving this PR open and nominated as an informed "Final comment period" on the suggested path above, and unless there's further discussion taking us in a different path the lang team will likely close this during next triage.

Thanks @vi!

Mark-Simulacrum · 2020-12-08T18:38:31Z

Thanks! We've discussed a bit more over Zulip since then, and the current consensus remains that we would like to see an exploration of the design space some more perhaps targeted at solving the underlying problems rather than patching over it. Going to go ahead and close this PR for the time being, though.

Add #[must_bind] attribute and lint

648aad3

rust-highfive assigned davidtwco Nov 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2020

jyn514 reviewed Nov 4, 2020

View reviewed changes

Mark-Simulacrum added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 6, 2020

davidtwco requested changes Nov 8, 2020

View reviewed changes

Mark-Simulacrum removed the I-nominated label Dec 8, 2020

Mark-Simulacrum closed this Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add #[must_bind] attribute and lint #78715

Add #[must_bind] attribute and lint #78715

vi commented Nov 3, 2020 •

edited

Loading

rust-highfive commented Nov 3, 2020

jyn514 Nov 4, 2020

ghost commented Nov 6, 2020

Mark-Simulacrum commented Nov 6, 2020

vi commented Nov 6, 2020 •

edited

Loading

davidtwco left a comment

davidtwco Nov 8, 2020

davidtwco Nov 8, 2020

davidtwco Nov 8, 2020

davidtwco Nov 8, 2020

davidtwco Nov 8, 2020

davidtwco Nov 8, 2020

vi commented Nov 12, 2020

jyn514 commented Nov 12, 2020

Mark-Simulacrum commented Nov 23, 2020

Mark-Simulacrum commented Dec 8, 2020

	"Bogus `_` binding of a type flagged as `#[must_bind]`. Use something like `_guard`.",
	"types marked as `#[must_bind]` should be assigned to a variable, not dropped immediately",

Add #[must_bind] attribute and lint #78715

Add #[must_bind] attribute and lint #78715

Conversation

vi commented Nov 3, 2020 • edited Loading

rust-highfive commented Nov 3, 2020

jyn514 Nov 4, 2020

Choose a reason for hiding this comment

ghost commented Nov 6, 2020

Mark-Simulacrum commented Nov 6, 2020

vi commented Nov 6, 2020 • edited Loading

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

davidtwco Nov 8, 2020

Choose a reason for hiding this comment

vi commented Nov 12, 2020

jyn514 commented Nov 12, 2020

Mark-Simulacrum commented Nov 23, 2020

Mark-Simulacrum commented Dec 8, 2020

vi commented Nov 3, 2020 •

edited

Loading

vi commented Nov 6, 2020 •

edited

Loading