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

Mark function without side effects as must_use #228

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

WhyNotHugo
Copy link
Contributor

This hint will make the compiler warn consumers of this library if they ever call one of these functions without using its return value.

This hint will make the compiler warn consumers of this library if they
ever call one of these functions without using its return value.
@partim
Copy link
Member

partim commented Sep 26, 2023

Thank you for the PR!

Is this idiomatic use of the #[must_use] attribute? I so far only considered it for cases where ignoring the return type likely results in problems not merely waste.

@WhyNotHugo
Copy link
Contributor Author

I so far only considered it for cases where ignoring the return type likely results in problems not merely waste.

The docs aren't too specific:

https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute

The places where it's used here are functions that have no side effect but construct a new value. Calling them without using the return value is usually a programmer mistake (it should either be assigned or the call deleted entirely).

@partim
Copy link
Member

partim commented Sep 26, 2023

Hm. You are certainly correct. And I suppose this isn’t all that different from the unused variables warnings.

I’ll merge it. But, I think this is a breaking change, so I’ll stick the label on it and keep it open until we get closer to a breaking release. (I am trying to avoid the issues we had with having to forward port changes.)

@partim partim added the breaking A PR that includes a breaking change. label Sep 26, 2023
@WhyNotHugo
Copy link
Contributor Author

Exactly! It's more a hint that anything else.

I don't think that it's breaking; at most, consumers will see a warning. And this is in case they're doing something like CharStr::builder(); without assigning the builder to any variable.

@partim
Copy link
Member

partim commented Sep 27, 2023

It might not strictly be breaking, but it has the potential to break CI runs without any changes and I really detest that.

@WhyNotHugo
Copy link
Contributor Author

It's your call, this is trivial to rebase anyway.

@partim partim merged commit 98669b2 into NLnetLabs:main Oct 5, 2023
12 checks passed
@WhyNotHugo WhyNotHugo deleted the must_use branch October 6, 2023 01:37
partim added a commit that referenced this pull request Oct 18, 2023
Breaking changes

* Move the `flatten_into` method for converting domain names into a
  straight, flat form into a new `FlattenInto` trait. This trait is only
  implemented for types that actually are or contain domain names. ([#216])
* Marked various methods and functions that return values without side
  effects as `#[must_use]`. ([#228] by [@WhyNotHugo])
* Changed the signature of `FoundSrvs::merge` to use a non-mut `other`.
  ([#232])

New

* Added support for the ZONEMD record type. ([#229] by [@xofyarg])
* Re-exported the _octseq_ crate as `dep::octseq`. ([#230])
* Added a blanket impl for mut refs to `Composer`. ([#231] by [@xofyarg])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A PR that includes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants