Skip to content

Clarify let_underscore documentation #10908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Jun 8, 2023

Fixes #10867
changelog: [let_underscore]: Clarify its documentation

@rustbot
Copy link
Collaborator

rustbot commented Jun 8, 2023

r? @llogiq

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 8, 2023
@@ -122,7 +122,7 @@ declare_clippy_lint! {
/// }
/// // Either provide a type annotation:
/// let _: Result<u32, ()> = foo();
/// // …or drop the let keyword:
/// // …or drop the `let` keyword to fully ignore the type no matter what:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assign-to-underscore pattern doesn't so much ignore the type as the binding. That said, I'm unsure how to best formulate this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this phrasing because the original PR did it.
#10356

[...] Alternatively, they can drop the let keyword to truly just ignore the value no matter what.

@Fuuzetsu
Copy link

I made the original ticket. I'm still unsure about why we'd warn that let _ = foo() is not fine because type can change at any time but _ = foo() is allowed... Why is former preferred to latter, even if we intentionally want to ignore the type? Neither stops or flags a type change of foo().

The only difference that I know of is that you can say let _x = foo() for a while to remove a binding from play for a little bit, the type changes (unknowingly) and you try to bring it back with let x = foo() and get a nasty surprise. _x = foo() is not allowed so it's much more obvious that you're ignoring the result completely rather than temporarily hiding a binding. However I don't think that's what the clippy lint is about, right? If it is, it should probably be much more explicit.

@Centri3
Copy link
Member

Centri3 commented Jun 10, 2023

let _ = foo() is not fine because type can change at any time but _ = foo() is allowed... Why is former preferred to latter

It's not preferred. let _ = and _ = function the same afaik, _ = is likely allowed as you're explicitly opting out of specifying the type (as you physically cannot give a type annotation); as blyxyas said, it's essentially the same as just allowing the lint altogether, but is a ton more ergonomic

@blyxyas
Copy link
Member Author

blyxyas commented Jul 5, 2023

AFAIK, both let _ = ... and _ = ... drop the value when used (meaning that the type is irrelevant, as it's immediately dropped). So, I think the original lint gave the option to use _ = ... instead of a let as a way to "allow" the lint.

I'm not sure it should have been implemented in the lint in the first place, but now that's here, I also don't see a reason to remove it. It's less verbose than an #[allow] and the lint is restriction, so it isn't very important.

@Fuuzetsu
Copy link

Fuuzetsu commented Jul 6, 2023

If we could add a line like "to allow this anyway, use _ = ..." then it'd be clearer. It's just that it currently sounds like a solution to the lroblem rather than a way to disable the hint and ignore it.

@blyxyas blyxyas force-pushed the fix-let_underscore_untyped_help_message branch from 167840e to 9199fed Compare July 11, 2023 10:42
@blyxyas
Copy link
Member Author

blyxyas commented Jul 24, 2023

@Fuuzetsu what do you think about the new commit? This has been in the middle of my notifications for 3 weeks now, I think it's ready 😅

@Fuuzetsu
Copy link

@Fuuzetsu what do you think about the new commit? This has been in the middle of my notifications for 3 weeks now, I think it's ready 😅

Looks better to me. I suppose there are other ways of saying the same thing such as or drop the `let` keyword to ignore this lint. but we could be here forever. Let's take what we have. Thank you.

@blyxyas
Copy link
Member Author

blyxyas commented Aug 8, 2023

@llogiq What do you think about the current phrasing?

@hikari-no-yume
Copy link

I think it is very unintuitive that let _ = would be be forbidden, but _ = wouldn't. What if there were separate lints for these two, and by default the former enables the latter, or something like this? You could then choose to allow the former but deny the latter if you want to use this pattern, but otherwise you'd get the intuitive behaviour where a type must be specified.

@blyxyas
Copy link
Member Author

blyxyas commented Sep 22, 2023

I'll just close this, 107 days for 4 words of documentation is kinda ridiculous.

@blyxyas blyxyas closed this Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs for let_underscore_untyped unclear
6 participants