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

Declarative macros: no error about duplicate trait items #71614

Closed
RalfJung opened this issue Apr 27, 2020 · 9 comments · Fixed by #100387
Closed

Declarative macros: no error about duplicate trait items #71614

RalfJung opened this issue Apr 27, 2020 · 9 comments · Fixed by #100387
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-trait-system Area: Trait system C-bug Category: This is a bug. F-decl_macro `#![feature(decl_macro)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 27, 2020

I tried this code on the latest nightly:

#![feature(decl_macro)]

trait Trait {
    type T;
}

macro trait_impl {
    () => {
        type T = ();
    }
}

impl Trait for i32 {
    trait_impl!();
    type T = ();
}

I expected to see this happen: an error about type T being defined twice in the same impl. (This is what happens with macro_rules! trait_impl.)

Instead, this happened: the code just compiles. The same also works when considering a function rather than a type.

@RalfJung RalfJung added C-bug Category: This is a bug. F-decl_macro `#![feature(decl_macro)]` labels Apr 27, 2020
@jonas-schievink
Copy link
Contributor

They're seperate items because of hygiene though, right?

@petrochenkov
Copy link
Contributor

They're seperate items because of hygiene though, right?

Exactly.

This is still a bug though, because the trait has a single item, but the impl has two.

@jonas-schievink jonas-schievink added A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 27, 2020
@RalfJung
Copy link
Member Author

The names both still "work" for the instance though (in the sense that having one is enough to satisfy the check that the impl covers each item of the trait).

When both are present, it silently uses one of them, and I do not know which...

@cjgillot
Copy link
Contributor

What should be the correct diagnostic here?
I see two possible:

  1. report the macro output as not being in the trait;
  2. report the a conflict between the two names in the impl.

@RalfJung
Copy link
Member Author

The following code should work fine I think:

#![feature(decl_macro)]

trait Trait {
    type T;
}

macro trait_impl {
    () => {
        type T = ();
    }
}

impl Trait for i32 {
    trait_impl!();
}

I believe that means only the 2nd error makes sense?

@cjgillot
Copy link
Contributor

What about without the trait: should we err? That would be inconsistent with plain items.

#![feature(decl_macro)]

macro trait_impl {
    () => {
        fn foo() {}
    }
}

struct Type;

impl Type {
    trait_impl!();
    fn foo() {}
}

@RalfJung
Copy link
Member Author

What do you mean? This errors after "inlining" the macro, right?

@cjgillot
Copy link
Contributor

trait_impl!();
fn foo() {}

does not error. This is by design, the main difference with macro_rules.

Hence the question: should inherent impls follow the "plain function" behaviour, or the trait impl behaviour?

@RalfJung
Copy link
Member Author

Ah, so functions inside trait_impl can call the foo declared inside but are 'isolated' from the outside? That makes sense in terms of hygiene, I guess.

It would be nice to be able to have macros partially implement a trait though. (We do that in rustc's const-eval.) Conceptually I feel like the names there really refer to the trait, they are not newly declared names but 'resolve' to existing names. Arguably inside the macro it doesn't know which trait to resolve them to so a strict hygiene would entirely forbid this, but also then there'd be no way to around around this, right?

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 11, 2022
…rochenkov

Check uniqueness of impl items by trait item when applicable.

When checking uniqueness of item names in impl blocks, we currently use the same definition of hygiene as for toplevel items.  This means that a plain item and one generated by a macro 2.0 do not collide.

This hygiene rule does not match with how impl items resolve to associated trait items. As a consequence, we misdiagnose the trait impls.

This PR proposes to consider that trait impl items are uses of the corresponding trait items during resolution, instead of checking for duplicates later. An error is emitted when a trait impl item is used twice.

There should be no stable breakage, since macros 2.0 are still unstable.

r? `@petrochenkov`
cc `@RalfJung`

Fixes rust-lang#71614.
@bors bors closed this as completed in 6d58ff7 Oct 11, 2022
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 12, 2022
Check uniqueness of impl items by trait item when applicable.

When checking uniqueness of item names in impl blocks, we currently use the same definition of hygiene as for toplevel items.  This means that a plain item and one generated by a macro 2.0 do not collide.

This hygiene rule does not match with how impl items resolve to associated trait items. As a consequence, we misdiagnose the trait impls.

This PR proposes to consider that trait impl items are uses of the corresponding trait items during resolution, instead of checking for duplicates later. An error is emitted when a trait impl item is used twice.

There should be no stable breakage, since macros 2.0 are still unstable.

r? ``@petrochenkov``
cc ``@RalfJung``

Fixes rust-lang/rust#71614.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Check uniqueness of impl items by trait item when applicable.

When checking uniqueness of item names in impl blocks, we currently use the same definition of hygiene as for toplevel items.  This means that a plain item and one generated by a macro 2.0 do not collide.

This hygiene rule does not match with how impl items resolve to associated trait items. As a consequence, we misdiagnose the trait impls.

This PR proposes to consider that trait impl items are uses of the corresponding trait items during resolution, instead of checking for duplicates later. An error is emitted when a trait impl item is used twice.

There should be no stable breakage, since macros 2.0 are still unstable.

r? ``@petrochenkov``
cc ``@RalfJung``

Fixes rust-lang/rust#71614.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Check uniqueness of impl items by trait item when applicable.

When checking uniqueness of item names in impl blocks, we currently use the same definition of hygiene as for toplevel items.  This means that a plain item and one generated by a macro 2.0 do not collide.

This hygiene rule does not match with how impl items resolve to associated trait items. As a consequence, we misdiagnose the trait impls.

This PR proposes to consider that trait impl items are uses of the corresponding trait items during resolution, instead of checking for duplicates later. An error is emitted when a trait impl item is used twice.

There should be no stable breakage, since macros 2.0 are still unstable.

r? ``@petrochenkov``
cc ``@RalfJung``

Fixes rust-lang/rust#71614.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros-2.0 Area: Declarative macros 2.0 (#39412) A-trait-system Area: Trait system C-bug Category: This is a bug. F-decl_macro `#![feature(decl_macro)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants