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

Check uniqueness of impl items by trait item when applicable. #100387

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Aug 10, 2022

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 #71614.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 10, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2022
@petrochenkov
Copy link
Contributor

I need to refresh hygiene details in my memory before reviewing this PR and #99445, I'll try to do it some time during the next couple of weeks.

@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor

Let's focus on trait impls first (as opposed to inherent impls).

Names in a trait impl (trait impl items) are considered uses referring to their definitions in the trait.
So they are resolved as uses - code in fn check_trait_item does it manually, but it should be equivalent to resolve_ident_in_module (with the trait being the module).

This PR doesn't change that scheme.
So I guess for traits we can check both completeness of trait items (that should consider specialization?) and the lack of duplicates in the same place, using only their DefIds obtained early during name resolution and not names.

@petrochenkov
Copy link
Contributor

As for inherent items, I don't think something like enforce_impl_items_are_distinct makes sense for them.
We need an additional overlap check to catch cases like

impl S { fn f() {} }
impl S { fn f() {} }

as well, so the same overlap check should be able to report

impl S {
    fn f() {}
    fn f() {}
}

too.

What should be considered a duplicate in this case (in presence of macros 2.0), I'm honestly not sure.
Inherent impl items are themselves definitions unlike trait impl items, so they should use the normalize_to_macros_2_0 approach to duplicates, but inherent impls for the same type can also be defined in totally different contexts (parent modules and expansions), how that fact affects the comparison?
I suggest just using whatever the overlap check uses now.
(And also splitting the changes for trait impls and inherent impls to 2 different PRs.)

@rustbot author

@rustbot rustbot 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 Oct 7, 2022
@cjgillot cjgillot force-pushed the hygiene-trait-impl branch 2 times, most recently from ea7d1b2 to ec19fcc Compare October 9, 2022 19:16
@cjgillot
Copy link
Contributor Author

cjgillot commented Oct 9, 2022

@petrochenkov I split the PR in 2 parts.
This PR keeps the change for trait impl items.
#102856 moves the check for duplicate inherent impl items work with overlap check.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2022
@petrochenkov
Copy link
Contributor

PR description also needs an update.
@rustbot author

@rustbot rustbot 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 Oct 10, 2022
@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 10, 2022
@petrochenkov
Copy link
Contributor

r=me after squashing commits.
@rustbot author

@rustbot rustbot 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 Oct 11, 2022
@cjgillot
Copy link
Contributor Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit 152cd63 has been approved by petrochenkov

It is now in the queue for this repository.

@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 Oct 11, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request 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 added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2022
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#100387 (Check uniqueness of impl items by trait item when applicable.)
 - rust-lang#101727 (Stabilize map_first_last)
 - rust-lang#101774 (Warn about safety of `fetch_update`)
 - rust-lang#102227 (fs::get_path solarish version.)
 - rust-lang#102445 (Add `is_empty()` method to `core::ffi::CStr`.)
 - rust-lang#102612 (Migrate `codegen_ssa` to diagnostics structs - [Part 1])
 - rust-lang#102685 (Interpret EH actions properly)
 - rust-lang#102869 (Add basename and dirname aliases)
 - rust-lang#102889 (rustc_hir: Less error-prone methods for accessing `PartialRes` resolution)
 - rust-lang#102893 (Fix ICE rust-lang#102878)
 - rust-lang#102912 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6d58ff7 into rust-lang:master Oct 11, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 11, 2022
@cjgillot cjgillot deleted the hygiene-trait-impl branch October 13, 2022 16:55
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 14, 2022
…ochenkov

Only test duplicate inherent impl items in a single place

Based on rust-lang#100387

r? `@petrochenkov`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 14, 2022
…ochenkov

Only test duplicate inherent impl items in a single place

Based on rust-lang#100387

r? ``@petrochenkov``
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…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.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#100387 (Check uniqueness of impl items by trait item when applicable.)
 - rust-lang#101727 (Stabilize map_first_last)
 - rust-lang#101774 (Warn about safety of `fetch_update`)
 - rust-lang#102227 (fs::get_path solarish version.)
 - rust-lang#102445 (Add `is_empty()` method to `core::ffi::CStr`.)
 - rust-lang#102612 (Migrate `codegen_ssa` to diagnostics structs - [Part 1])
 - rust-lang#102685 (Interpret EH actions properly)
 - rust-lang#102869 (Add basename and dirname aliases)
 - rust-lang#102889 (rustc_hir: Less error-prone methods for accessing `PartialRes` resolution)
 - rust-lang#102893 (Fix ICE rust-lang#102878)
 - rust-lang#102912 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
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.

Declarative macros: no error about duplicate trait items
6 participants