Skip to content
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

Add more #[must_use] attributes for appropriate functions #457

Merged
merged 2 commits into from Jul 22, 2022

Conversation

WaffleLapkin
Copy link
Member

See #453

This PR marks all functions warned by clippy::must_use_candidate as #[must_use]

@p0lunin
Copy link
Collaborator

p0lunin commented Oct 14, 2021

Let's just disable this lint: it forces to add too much boilerplate attributes which is unusable in most cases

@Hirrolot
Copy link
Collaborator

Hirrolot commented Oct 14, 2021

I don't think that they're useless -- at least, they reduce the risk of bugs. @WaffleLapkin, can we wrap this #[must_use = "..."] into a macro?

@WaffleLapkin
Copy link
Member Author

@p0lunin we can't disable this lint, because it isn't enabled in the first place.

@Hirrolot

I don't think that they're useless -- at least, they reduce the risk of bugs.

The methods are not useless, but calling them and then not using their output is useless since it does nothing.

WaffleLapkin, can we wrap this #[must_use = "..."] into a macro?

We probably could, but I don't think we should, IMO it will create more problems than it'll solve.

@Hirrolot
Copy link
Collaborator

That's right, so maybe we should only annotate builder-related functionality?

@WaffleLapkin
Copy link
Member Author

Any pure function is just a transformation of data, so it's kind of builder-related in some sense? 🤔

IMO it would be better if Rust supported the opposite - marking functions as #[may_discard] (ie the default is function result must be used). But at this point, it can't be helped.

@Hirrolot
Copy link
Collaborator

Any pure function is just a transformation of data, so it's kind of builder-related in some sense? 🤔

I mean request builders specifically, in teloxide-core. Or they are already marked with #[must_use]?

@WaffleLapkin
Copy link
Member Author

@Hirrolot the request builders are not fully marked yet, but I'm currently working on a PR that resolves that.

I don't see though why we should only mark as #[must_use] the builder methods. It seems like the HTML/markdown methods, for example, are just as easy to misuse:

let input: &str = ...;
escape(input); // user may have expected that this would change `input`

bot.send_message(..., format!("Input: {}", input)).await?; // Oops

@Hirrolot
Copy link
Collaborator

It's fair. However, I don't see much sense in #[must_use] descriptions because users can reasonably infer this information from the signatures. If a function accepts &str and returns String, it's crystal clear that it returns a new string rather than mutating its argument, so I'd suggest avoiding those descriptions.

@WaffleLapkin
Copy link
Member Author

Telegram bots are a common thing to do for beginners, which can struggle to understand that a &str -> String function returns an edited copy of the string and can't possible mutate it's argument. With that in mind, I think that the descriptions are useful.

@WaffleLapkin
Copy link
Member Author

(rebased to update CI)

src/utils/markdown.rs Outdated Show resolved Hide resolved
@Hirrolot Hirrolot mentioned this pull request Jul 22, 2022
@Hirrolot Hirrolot self-requested a review July 22, 2022 11:54
@WaffleLapkin WaffleLapkin merged commit a36f2b0 into dev Jul 22, 2022
@WaffleLapkin WaffleLapkin deleted the more_must_uses branch July 22, 2022 11:56
WaffleLapkin added a commit that referenced this pull request Nov 1, 2022
Add more `#[must_use]` attributes for appropriate functions

Former-commit-id: a36f2b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants