Updated RFC #886 with lessons learned from #1812

[iopq]

See #886 and #1812

Rendered

Took out ok() parts

[mark-i-m]

I like the idea in general.

But i see two other alternatives:

1. Add must_use for types. That is, when a value of type T is ignored via a semicolon which is marked must_use a lint warning is triggered. This might make sense for types like Result, which may be used to indicate an error.

2. A stricter rule would be to emit warnings for all ignored values of types other than ! and (). Then, you could opt out of the warning explicitly.

Have these options been considered?

[Nemo157]

Those are both already implemented, Result (and a few other standard types) are must_use, and there's the unused_results lint (allowed by default) to let you get warnings/errors for ignoring any type (other than (), and I guess ! sorta since you can't actually reach it).

[mark-i-m]

Ah, so IIUC, the argument of this RFC is that there are types for which must_use is too strong? For example, what prevents us from simply must_useing bool and Option?

In reading the RFC, I didn't understand why existing solutions are insufficient...

[sfackler]

There are quite a lot of APIs that return things you don't always care about, e.g. HashMap::insert.

[mark-i-m]

Ah, ok. That's what I thought, but it's good to be sure 😄

Perhaps the RFC can explain this more explicitly.

[Nemo157]

One specific example of why making bool as a whole must_use would be really annoying is HashSet::insert; sometimes it's useful to know if you just inserted a new value, other times you just don't care.

A similar example against making Option must_use is Vec::pop; a lot of times when I end up using pop I don't actually want to use the resulting value, I might just be popping off a couple of known values.

What this RFC allows for are cases where a function is returning a more general type like those, but you still want to force the user to do something with the result. The example from #1812 is that by applying must_use to PartialEq::eq then code containing a line like foo == bar; on its own (a noop in any decent implementation of PartialEq) would trigger the must_use lint as it's likely intended to be foo = bar;.

[iopq]

https://github.com/zcoinofficial/zcoin/blob/81a667867b5d84893e4c225a8d73eddd78ac16c2/src/main.cpp#L963

zccoinSpend.denomination == libzerocoin::ZQ_LOVELACE;

this typo has enabled an attacker to create 548,000 Zcoins

this should motivate this RFC - right now Rust doesn't warn about this

[mark-i-m]

Well, actually, the amount of motivation is proportional to the cost of a zcoin 😜

Seriously, though, I does anyone know how common such mistakes actually are in the wild?

I already agree with the RFC, but I am curious.

[iopq]

It is estimated that the attacker made ~$750,000 when he sold his Zcoins. Those are some expensive bugs.

[withoutboats]

@iopq Rather than trying to wrangle merging two PR branches together, I think I want to propose we just merge this branch as RFC 886.

However, there are a few more amendments I think it needs to get it in line with the current consensus I'm going to list them as concerns and simultaneously propose a merge of this branch.

[withoutboats]

@rfcbot fcp merge

[withoutboats]

@rfcbot concern "partial_eq"

The detailed design doesn't mention attaching this attribute to PartialEq::partial_eq, which is something I think we know we want to do.

[withoutboats]

@rfcbot concern "Result::ok"

The drawback and motivation sections still contain a lot of the original commentary about Result::ok. Probably that should be trimmed down, since we aren't considering adding this attribute to Result::ok right now.

[withoutboats]

@rfcbot concern "traits"

One thing this RFC doesn't mention is how this plays out with traits and impls - clearly we want to support applying this attribute to trait declarations, but can you apply it to methods in a trait impl block, so that only that particular impl warns? I don't know what implementation concerns that would raise and I'd like to hear from someone about whether or not it would be hard to allow that.

[rfcbot]

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

Concerns:

• "partial_eq" resolved by Updated RFC #886 with lessons learned from #1812 #1940 (comment)
• "Result::ok" resolved by Updated RFC #886 with lessons learned from #1812 #1940 (comment)
• "traits" resolved by Updated RFC #886 with lessons learned from #1812 #1940 (comment)

Once these reviewers reach consensus, 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!

See this document for info about what commands tagged team members can give me.

[withoutboats]

@rfcbot resolved "Result::ok"

[withoutboats]

@rfcbot resolved "traits"

[liigo]

update rendered @iopq

[rfcbot]

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

[liigo]

Result::{ok, err} are mentioned in Summary, but not in Detailed design section.

[rfcbot]

The final comment period is now complete.

[aturon]

Huzzah! This RFC has been merged! Tracking issue.

Thanks, everyone who helped push this idea through!

[olson-dan]

I know the final comment period is complete but #[must_use] is a bad choice as opposed to #[maybe_not_used] because it implies the wrong default behavior.

(Sorry for wading into a pile of well-considered discussion and dropping this, but I feel it's very important for languages that value correctness to have the default behavior of warning on unused return value, which would make things like #[muse_use] unnecessary. Normally I would let it go but I think it's very important for Rust to make good choices going forward instead of drifting into programming language status quo.)

[ssokolow]

@olson-dan

#[maybe_not_used] is not at all intuitive unless changed to #[warn(maybe_not_used)] because it's a state of being rather than a command.

If it were changed, I'd recommend something like #[warn_if_unused] for that very reason.

[olson-dan]

@ssokolow

If I read your comment right I think it means I wasn't clear... I'm saying that #[must_use] should be the default behavior and that default behavior should not be opted-into but opted out of.

[ssokolow]

@olson-dan What you're arguing for is implementing full-blown linear typing as a lint and this is the only way that's really a viable option.

See The Pain Of Real Linear Types in Rust for details.

As for phrasing, my point about intuitiveness still stands. #[may_discard] would be a more intuitive name for what you proposed.

[withoutboats]

That would be a breaking change anyway & is just not on the table for that reason.

[olson-dan]

@withoutboats

By breaking you mean it would break 1.0 code? If it was not enabled by default it would not. Although it should definitely be default behavior a warning that can be opted-into globally is preferable to local, opt-ins.

Or more specifically: I can't rely on developers to locally opt-in to behavior that helps them write code correctly, but I can control compiler options across my organization. This is a major frustration in C++ because "Just add [[nodiscard]] to every function" is not an enforceable solution in that language.

Rust is going this route, it seems, and while #[must_use] is a valuable tool in the absence of proper warnings, it's not sufficient for my needs.

[Nemo157]

@olson-dan see the existing unused-results lint, this is allowed by default but you can enable it for your own codebase. I have attempted to use this in the past and found it to be too strict for real code.

[olson-dan]

@Nemo157

Thanks, that solves it for me. Sorry to derail the issue.