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

Give return-position impl traits in trait a (synthetic) name to avoid name collisions with new lowering strategy #109455

Conversation

compiler-errors
Copy link
Member

We were previously lowering RPITITs to associated items with the name "kw::Empty", which as petrochenkov predicted, causes name collisions when being decoded from foreign crates.

Instead, synthesize a name by concatenating the def-path segments of the original RPITIT, something like foo::{opaque#1} or foo::{opaque#0}::{opaque#0}. This should keep the item unnameable, since we never want to name it as that would be problematic as we lower these items after the resolver is run.

Also, fix a few unrelated bugs with nested RPITITs, and beefen up the foreign encoding test.

cc @petrochenkov and the test case described here: https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/-Z.20lower-impl-trait-in-trait-to-assoc-ty.20summary.3F/near/343451927

@rustbot
Copy link
Collaborator

rustbot commented Mar 21, 2023

r? @Nilstrieb

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 21, 2023
@petrochenkov petrochenkov self-assigned this Mar 21, 2023
@@ -281,7 +281,7 @@ pub enum DefPathData {
/// An `impl Trait` type node.
ImplTrait,
/// `impl Trait` generated associated type node.
ImplTraitAssocTy,
ImplTraitAssocTy(Symbol),
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess there's no real reason why this needs to be a separate def path data. We could use TypeNs here now, (I guess) as long as we keep the actual name that the RPITIT is lowering to unnameable.

@spastorino
Copy link
Member

spastorino commented Mar 21, 2023

Btw, it looks good to me but I'd leave this up to @petrochenkov given that they've self-assigned it.

@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor

I think this is a big unnecessary complication.
See how macro_rules are ignored in build_reduced_graph_for_external_crate_res despite being in the children list.
Synthetic RPITITs can be ignored in the same way.

@petrochenkov
Copy link
Contributor

petrochenkov commented Mar 22, 2023

It's kind of a philosophical question what exactly should go into the children list.

Right now only named items that are actually planted into modules and visible to other modules/crates in this way are put into the children list, that it's main use case.

Also macro_rules are put into the same list. Why? For historical reasons. They have to be ignored in the main user of children (build_reduced_graph_for_external_crate_res ) and could easily go into a separate list. I actually planned to remove them from this list, just never got to it.

Are synthetic RPITITs similar to macro_rules in this case?
Yes, they are not named and not planted into trait "modules ", so they either need to go to a separate list, or be ignored by the main user of the children list.
Something like that.

(I think I have a weak preference for separate lists for both after all.)

@petrochenkov petrochenkov 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 Mar 22, 2023
@spastorino
Copy link
Member

I don't think this is really complicated, the meat of the change is 602cf24 and the change also help to make diagnostics be equal to master as seen in https://github.com/rust-lang/rust/pull/109455/files#diff-2245ba262b8ba2866c27f1ac3f76db96146870beb91a6ac7ce621ca2cbb7c501R9. Although I guess that could also be Trait::{opaque} instead of Trait::method::{opaque} and it would also be fine.

@bors
Copy link
Contributor

bors commented Mar 22, 2023

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

@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 Mar 23, 2023
@petrochenkov
Copy link
Contributor

It's not very complicated, it's just unnecessarily complicated (I'm only talking about the last commit, others seem unrelated).
If these types don't have a name then they shouldn't have a name.
Then you also won't have to invent some hygiene emulation scheme to synthesize names (which currently breaks if two RPITITs are defined by the same method?).

@petrochenkov petrochenkov 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 Mar 23, 2023
@spastorino
Copy link
Member

(which currently breaks if two RPITITs are defined by the same method?).

I don't think is going to break, you would end with Trait::method::{opaque#0}, Trait::method::{opaque#1} and so on.

Anyway, would leave this up to be answered by @compiler-errors

@Noratrieb Noratrieb removed their assignment Mar 24, 2023
@spastorino
Copy link
Member

Closing in favor of #109499

@spastorino spastorino closed this Mar 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2023
…-errors

Give return-position impl traits in trait a (synthetic) name to avoid name collisions with new lowering strategy

The only needed commit from this PR is the last one.

r? `@compiler-errors`

Needs rust-lang#109455.
@compiler-errors compiler-errors deleted the give-rpitits-real-names branch August 11, 2023 20:02
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-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.

6 participants