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 a MustUse trait to complement #[must_use] #71816

Closed
wants to merge 3 commits into from
Closed

Add a MustUse trait to complement #[must_use] #71816

wants to merge 3 commits into from

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented May 2, 2020

This is intended to solve a couple of diagnostics issues, particularly around async/await (for example, #67387 and #71368).

Currently, this will expand #[must_use] on type definitions to MustUse impls, but due to coherence it is not yet possible to make #[must_use] on a trait expand to a blanket MustUse impl for all type implementing the trait.

I'm not quite sure what the solution for that would be, but it likely involves punching a hole in the coherence rules, similar to what's done for #[marker] traits, and possibly rejecting MustUse in trait bounds to guard against potential soundness issues introduced by that. I'm not sure if the trait system can give us all impls that could apply (so that the lint can collect all REASON strings), but even just getting an arbitrary impl should be enough to solve the diagnostics problem here.

Before I go ahead with that, I'd like to gather some feedback on the approach I've taken so far though.

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2020
@jonas-schievink
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 2, 2020

⌛ Trying commit 6c8c6bc06fe088345cc4bd00b4e5e56d97e31f48 with merge 88cbb409307e88eb10e428631ecaa67178e52435...

@bors
Copy link
Contributor

bors commented May 3, 2020

☀️ Try build successful - checks-azure
Build commit: 88cbb409307e88eb10e428631ecaa67178e52435 (88cbb409307e88eb10e428631ecaa67178e52435)

@rust-timer
Copy link
Collaborator

Queued 88cbb409307e88eb10e428631ecaa67178e52435 with parent f05a524, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 88cbb409307e88eb10e428631ecaa67178e52435, comparison URL.

The macro just expands to its input and replaces itself with
`#[rustc_must_use]` for now.

Technically this is a breaking change since it will now error when
used as a crate-level attribute. This does nothing, so it should just
be removed when that happens.
@jonas-schievink jonas-schievink changed the title [WIP] Add a MustUse trait to complement #[must_use] Add a MustUse trait to complement #[must_use] May 4, 2020
@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label May 5, 2020
@nikomatsakis
Copy link
Contributor

So, we discussed this once before in the meeting, and I was fairly skeptical of it then. I think I remain sort of skeptical, but I clearly see the advantages, as well, and I think the request is well-motivated, so I'm trying to keep an open mind. The alternatives I can come up with are basically extending #[must_use] with more options and maybe that's just worse.

Regardless, I think this sort of change also probably requires an RFC -- per our new process, that means we ought to see if there is a lang team liaison who wants to carry it through, I think.

I'm nominating for the upcoming triage meeting.

@nikomatsakis
Copy link
Contributor

Ah, right, I also remember that this introduced an asymmetry, in that #[must_use] on a function can't be "reified" into the trait system.

@nikomatsakis
Copy link
Contributor

(I haven't looked at the implementation details yet, I will say that this would be nicer given a chalk-like split :) but oh well.)

@nikomatsakis
Copy link
Contributor

For completeness, I think the alternative would be to extend #[must_use] with something like #[must_use(conditional)], which says that, for some generic type Foo<P0..PN>, it is "must use" if any of P0..Pn are must-use. That would suffice for all known use cases (Pin, Option). We could even make it an error if there is more than one parameter.

That reminds me of another limitation of using the trait system for this. I guess if we make it a marker trait, it's ok, but otherwise you can't express the "any" in the statement above.

@Nemo157
Copy link
Member

Nemo157 commented May 11, 2020

#[must_use(conditional)] would work for the newly mentioned case as well. But having something like impl<F: Future> MustUse for Foo<F> where F::Output: MustUse seems like it could potentially be useful somewhere.

@bors
Copy link
Contributor

bors commented May 12, 2020

☔ The latest upstream changes (presumably #72120) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor

@Nemo157 that's an interesting example, yeah, although I'm not sure if it will really come up or not.

I guess I agree that it's better to (mildly) abuse the trait system than to invent a new specification language. I think my resistance to this idea of using a "magic trait" is weakening.

However, there is still the question of whether we want this extension to be opt-in or whether some kind of "opt-out". Opt-in would definitely be the more conservative route.

(I also tend to think an RFC is appropriate for this level of change, though I'm good with experimentation before that.)

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2020
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 30, 2020
@jonas-schievink
Copy link
Contributor Author

Closing this as I don't really have enough time to go through the RFC process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants