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

Update non-local-defns regarding parameterized traits and types #3581

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dhardy
Copy link
Contributor

@dhardy dhardy commented Mar 3, 2024

The existing RFC does not give sufficient attention to parametrized types and traits.

@ehuss ehuss added the not-rfc For PRs that fix things like spelling mistakes, wrong file names, etc. label Mar 3, 2024
@matklad
Copy link
Member

matklad commented Mar 8, 2024

Agree that this is under specified. I am not sure that spelling this out explicitly in the RFC text works --- its not clear whether the specified rules have any loopholes.

My current belief is that the rule we want here is exactly equivalent to coherence. So the RFC should say something along to the effect of "function bodies are treated as downstream crates for the purposes of trait checking and type inference".

As this (most excellent) example by QuineDot demonstrates

#3373 (comment)

it is important to restrict both:

  • which impls are allowed inside
  • which inside impls are visible from the outside, for the purpose of type inference reasoning that "I know of a single impl for this trait, so _ must be that type", where "that type" might end up being a type from a function's body.

@matklad
Copy link
Member

matklad commented Mar 8, 2024

Ah, I missed this extra bit of context: rust-lang/rust#121621, where it is suggested that we want more restritive rules than coherence here specifically to avoid fixind point two from above.

@ehuss
Copy link
Contributor

ehuss commented Apr 1, 2024

@tmandry Is this something you can review (or direct to someone who can)?

I can't tell if this is changing the RFC in a meaningful way, or just clarifying it. If it is a change from before, it would probably be best to start a new RFC.

@tmandry
Copy link
Member

tmandry commented Apr 2, 2024

From a quick look this is in line with the original RFC, and if @joshtriplett is willing to consider it a "friendly amendment" I think it can be merged.

fn main() {
Foo.method();
}
```
Copy link
Member

@joshtriplett joshtriplett Apr 2, 2024

Choose a reason for hiding this comment

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

I'd prefer to leave this example out of the motivation section. Could you move it to the explanation section?

@joshtriplett
Copy link
Member

Made a few editorial changes, and made one comment about moving the example out of the motivation section. With that change made, LGTM; no objections.

@Urgau
Copy link
Member

Urgau commented Apr 2, 2024

Isn't this going against T-types solution ? (which is being implemented in rust-lang/rust#122747, @lcnr)
It was even approved by T-lang.

@joshtriplett
Copy link
Member

@Urgau Thanks for catching that. @lcnr, can you help resolve the discrepancy here?

@lcnr
Copy link
Contributor

lcnr commented Apr 6, 2024

I'd personally just keep the RFC as "we lint if the existance of the impl is observable outside of its containing item", I don't think there is much value in explaining the exact algorithm used for that in an RFC. I also thought we generally expect the reference/guidelevel explanation of RFCs to get out of date and do not bother updating them, instead referring to the reference/docs.

The idea implemented in rust-lang/rust#122747 is as follows:

Lint impls inside of bodies unless:

  • the body is of an unnamed const, or
  • the self type is also local to the body, or
  • for trait impls: after replacing all generic params and types in the local to the body with inference variables in the "impl trait ref", there are at least 2 global impls which could also prove that trait bound

This should mean that in all cases where the local impl may apply outside of the body, we already fail with ambiguity even when ignoring the impl

@joshtriplett
Copy link
Member

@lcnr I'm deferring entirely to the types team here for whether you want to make changes/suggestions to this or close it.

It'd be nice if the rules we have are documented somewhere. Updating the RFC is one possible option; others could work as well.

@dhardy
Copy link
Contributor Author

dhardy commented Apr 27, 2024

whether you want to make changes/suggestions to this or close it

My only feelings is that it is often hard to tell the status and intended behaviour of in-development features. Whether or not updating RFCs is appropriate I don't know... and now we're getting off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-rfc For PRs that fix things like spelling mistakes, wrong file names, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants