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

Implement #[rustc_must_implement_one_of] attribute #92164

Merged
merged 9 commits into from
Jan 18, 2022

Conversation

WaffleLapkin
Copy link
Member

@WaffleLapkin WaffleLapkin commented Dec 21, 2021

This PR adds a new attribute — #[rustc_must_implement_one_of] that allows changing the "minimal complete definition" of a trait. It's similar to GHC's minimal {-# MINIMAL #-} pragma, though #[rustc_must_implement_one_of] is weaker atm.

Such attribute was long wanted. It can be, for example, used in Read trait to make transitions to recently added read_buf easier:

#[rustc_must_implement_one_of(read, read_buf)]
pub trait Read {
    fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
        let mut buf = ReadBuf::new(buf);
        self.read_buf(&mut buf)?;
        Ok(buf.filled_len())
    }

    fn read_buf(&mut self, buf: &mut ReadBuf<'_>) -> Result<()> {
        default_read_buf(|b| self.read(b), buf)
    }
}

impl Read for Ty0 {} 
//^ This will fail to compile even though all `Read` methods have default implementations

// Both of these will compile just fine
impl Read for Ty1 {
    fn read(&mut self, buf: &mut [u8]) -> Result<usize> { /* ... */ }
}
impl Read for Ty2 {
    fn read_buf(&mut self, buf: &mut ReadBuf<'_>) -> Result<()> { /* ... */ }
}

For now, this is implemented as an internal attribute to start experimenting on the design of this feature. In the future we may want to extend it:

  • Allow arbitrary requirements like a | (b & c)
  • Allow multiple requirements like
    • #[rustc_must_implement_one_of(a, b)]
      #[rustc_must_implement_one_of(c, d)]
  • Make it appear in rustdoc documentation
  • Change the syntax?
  • Etc

Eventually, we should make an RFC and make this (or rather similar) attribute public.


I'm fairly new to compiler development and not at all sure if the implementation makes sense, but at least it passes tests :)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 21, 2021
@WaffleLapkin WaffleLapkin marked this pull request as draft December 21, 2021 15:42
@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin WaffleLapkin force-pushed the rustc_must_implement_one_of_attr branch from e86dc5a to db76b9e Compare January 3, 2022 14:52
@WaffleLapkin WaffleLapkin marked this pull request as ready for review January 3, 2022 15:40
@WaffleLapkin WaffleLapkin changed the title [WIP] implement #[rustc_must_implement_one_of] attribute Implement #[rustc_must_implement_one_of] attribute Jan 3, 2022
@WaffleLapkin
Copy link
Member Author

r? @Aaron1011

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Quite glad to see the eq / ne etc. situation fixed that nicely 👌

Just a nit about favoring "(associated) function" terminology to "methods".

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/trait_def.rs Outdated Show resolved Hide resolved
compiler/rustc_feature/src/builtin_attrs.rs Outdated Show resolved Hide resolved
@WaffleLapkin WaffleLapkin force-pushed the rustc_must_implement_one_of_attr branch from e4a0af2 to 105ae9c Compare January 4, 2022 10:33
@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin WaffleLapkin force-pushed the rustc_must_implement_one_of_attr branch from 105ae9c to f524d58 Compare January 4, 2022 10:49
@bors
Copy link
Contributor

bors commented Jan 8, 2022

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 8, 2022
hir::ItemKind::Trait(is_auto, unsafety, .., items) => {
(is_auto == hir::IsAuto::Yes, unsafety, items)
}
hir::ItemKind::TraitAlias(..) => (false, hir::Unsafety::Normal, &[][..]),
Copy link
Member

Choose a reason for hiding this comment

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

I think this can just be &[] instead of &[][..]

Copy link
Member Author

Choose a reason for hiding this comment

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

screenshot of a compilation error that happens without [..] - in short, expected &[TraitItemRef], found &[_; 0]

Copy link
Contributor

Choose a reason for hiding this comment

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

Very disappointing; minimal repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=875b8d707eb70e293fc06d51eb6f26e0 — a type annotation at line 1201 does fix it, but without that a "wide pointer" field on one branch is not sufficient for a coercion to happen on other branches.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, even a one element tuple is enough to demonstrate this: (play)

@Aaron1011
Copy link
Member

Can you rebase this against master?

@WaffleLapkin WaffleLapkin force-pushed the rustc_must_implement_one_of_attr branch from f524d58 to 96b2f8a Compare January 9, 2022 09:21
@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member

It looks like you need to mark the test with // check-pass

@WaffleLapkin
Copy link
Member Author

This PR is currently broken because #90639 changed what values does is_implemented have. So the test correctly fails (by passing compilation). I haven't yet figured how to fix this.

This adds the old, pre 90639 `is_implemented` that previously only was
true if the implementation of the item was from the given impl block and
not from the trait default.
@WaffleLapkin
Copy link
Member Author

@Aaron1011 I rebased & fixed the problem, now it should work as expected again :')

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 13, 2022
@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Jan 13, 2022

The test failed because order of items in a FxHashSet<Ident> changed. Isn't FxHashSet supposed to be deterministic?

And what is the right way to get items from it for error reporting then? We can access the original list there, instead of the hash set I think.

@WaffleLapkin
Copy link
Member Author

@Aaron1011 I removed the use of HashSet so CI should spuriously fail anymore. I've also added a check that all function names are unique.

Copy link
Contributor

@danielhenrymantilla danielhenrymantilla left a comment

Choose a reason for hiding this comment

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

Modulo that last tiny nit, the new improvements look good to me! (EDIT: I still can't r stuff 😅)

compiler/rustc_typeck/src/collect.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 14, 2022

@danielhenrymantilla: 🔑 Insufficient privileges: Not in reviewers

@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2022

📌 Commit 28edd7a has been approved by Aaron1011

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 16, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2022
…askrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#90498 (Clarifications in the target tier policy)
 - rust-lang#92164 (Implement `#[rustc_must_implement_one_of]` attribute)
 - rust-lang#92729 (rustc_codegen_llvm: Remove (almost) unused span parameter from many functions in metadata.rs)
 - rust-lang#92752 (Correct minor typos in some long error code explanations)
 - rust-lang#92801 (Enable wrapping words by default)
 - rust-lang#92825 (Rename environment variable for overriding rustc version)
 - rust-lang#92877 (Remove LLVMRustMarkAllFunctionsNounwind)
 - rust-lang#92936 (rustdoc: Remove `collect` in `html::markdown::parse`)
 - rust-lang#92956 (Add `log2` and `log10` to `NonZeroU*`)
 - rust-lang#92960 (Use `carrying_{mul|add}` in `num::bignum`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 32d85c0 into rust-lang:master Jan 18, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 18, 2022
@WaffleLapkin WaffleLapkin deleted the rustc_must_implement_one_of_attr branch January 18, 2022 08:34
@oxalica
Copy link
Contributor

oxalica commented Jan 30, 2022

This reminds me the #[cfg()] syntax, which already defines a set of boolean operators.
Would it be more clear to be #[rustc_must_implement(any(a, b))] and can be extended to #[rustc_must_implement(all(any(a, b), any(c, d)))]?

@WaffleLapkin
Copy link
Member Author

@oxalica personally I'd say that cfg syntax is quite hard to read. I've seen a proposal to change it to use && and || and IMO it's a good idea.

rustc_must_implement can be extended to use the old cfg syntax or a new one, though I think we should at least explore other possibilities. For example, since rustc_must_implement can only make the requirement stricter and it seems to be mostly needed when you have recursive default implementation, maybe something like #[requires] attribute is better suited for this:

trait Eq {
    #[requires(neq)]
    fn eq(&self, other: &Self) -> bool { !self.neq(other) }

    #[requires(eq)]
    fn neq(&self, other: &Self) -> bool { !self.eq(other) }
}

@oxalica
Copy link
Contributor

oxalica commented Jan 31, 2022

@WaffleLapkin

@oxalica personally I'd say that cfg syntax is quite hard to read. I've seen a proposal to change it to use && and || and IMO it's a good idea.

I'm not really care about the detail of cfg syntax or if it need to be changed. I just think if we have two similar situations, it's better to always keep them consistent, and maybe changing them together, instead of inventing a new one in the new place.

trait Eq {
    #[requires(neq)]
    fn eq(&self, other: &Self) -> bool { !self.neq(other) }

    #[requires(eq)]
    fn neq(&self, other: &Self) -> bool { !self.eq(other) }
}

I don't like the word requires since it sounds like the semantic of "use/import neq" instead of "must impl", but well it's a bike-shedding issue.
As you said, this solution is specific for recursive default impls and cannot express something like #[must_implement(any(not(foo), bar))] trait Foo { .. } (defining foo implies defining bar). But I'm not sure if this complex case ever exists in real world.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 26, 2022
…ort_for_must_implement, r=GuillaumeGomez

rustdoc: Add support for `#[rustc_must_implement_one_of]`

This PR adds support for `#[rustc_must_implement_one_of]` attribute added in rust-lang#92164. There is a desire to eventually use this attribute of `Read`, so making it show up in docs is a good thing.

I "stole" the styling from cfg notes, not sure what would be a proper styling. Currently it looks like this:
![2022-07-14_15-00](https://user-images.githubusercontent.com/38225716/178968170-913c1dd5-8875-4a95-9848-b075a0bb8998.png)

<details><summary>Code to reproduce</summary>
<p>

```rust
#![feature(rustc_attrs)]

#[rustc_must_implement_one_of(a, b)]
pub trait Trait {
    fn req();
    fn a(){ Self::b() }
    fn b(){ Self::a() }
}
```

</p>
</details>
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 9, 2022
…table, r=Aaron1011

Implement `#[rustc_default_body_unstable]`

This PR implements a new stability attribute — `#[rustc_default_body_unstable]`.

`#[rustc_default_body_unstable]` controls the stability of default bodies in traits.
For example:
```rust
pub trait Trait {
    #[rustc_default_body_unstable(feature = "feat", isssue = "none")]
    fn item() {}
}
```
In order to implement `Trait` user needs to either
- implement `item` (even though it has a default implementation)
- enable `#![feature(feat)]`

This is useful in conjunction with [`#[rustc_must_implement_one_of]`](rust-lang#92164), we may want to relax requirements for a trait, for example allowing implementing either of `PartialEq::{eq, ne}`, but do so in a safe way — making implementation of only `PartialEq::ne` unstable.

r? `@Aaron1011`
cc `@nrc` (iirc you were interested in this wrt `read_buf`), `@danielhenrymantilla` (you were interested in the related `#[rustc_must_implement_one_of]`)
P.S. This is my first time working with stability attributes, so I'm not sure if I did everything right 😅
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants