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

refining_impl_trait only fires on public traits #119535

Open
tmandry opened this issue Jan 3, 2024 · 1 comment
Open

refining_impl_trait only fires on public traits #119535

tmandry opened this issue Jan 3, 2024 · 1 comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-traits Area: Trait system F-refine `#![feature(refine)]`; RFC #3245 F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@tmandry
Copy link
Member

tmandry commented Jan 3, 2024

The refining_impl_trait lint only fires for public traits. It does not fire without the pub keyword in the code sample below:

pub trait Foo {
//^ Required for lint to fire
    fn foo(self) -> impl Debug;
}

impl Foo for u32 {
    fn foo(self) -> String {
//                  ^^^^^^
//  warning: impl trait in impl method signature does not match trait method signature
        self.to_string()
    }
}

Difference from async_fn_in_trait lint

Apparently I was part of the discussion of this at one point (see also the zulip topic on this). I think it got lumped together with the discussion about the lint for async fn in traits, though, when there are some important distinctions:

  • The async fn lint is only temporary to help avoid footguns created by missing language features, and we want to make non-footgunny uses more convenient.
  • Refinement is a mechanism that will always exist and is fundamental to trait implementations.
  • Refinement's ability to "punch through" abstraction boundaries can happen accidentally, even within a crate.

The second point is important, because as a user I would expect such a fundamental mechanism to behave independently of whether the trait happens to be crate-public or not. This can lead to false expectations being created about the behavior in the other case.

Violating abstraction boundaries within a crate

As an example of the last point, let's say I as a user want to define a trait that my type implements ahead of actually generalizing my code:

trait Application {
    fn windows(&self) -> Vec<impl Window>;
}
trait Window {
    fn title(&self) -> Option<String>;
}

struct App;
struct Win;

impl Application for App {
    fn windows(&self) -> Vec<Win> { todo!() }
}
impl Window for Win {
    fn title(&self) -> Option<String> { todo!() }
}

fn all_windows(apps: &[App]) -> Vec<Win> {
    apps.iter().map(|a| a.windows()).flatten().collect()
}

Later on, I want to write a test for all_windows. But in order to do that, I have to change it to accept impl Application, which requires changing the output type to impl Window '_, and possibly changing all the users of all_windows as well. This can get unwieldy quick.

We can say that the user should have used impl Trait from the beginning, but that might be inconvenient when prototyping. If they are leaning on traits to provide the outlines of an abstraction boundary, we should let them opt in before punching through said boundary, IMO.

cc @compiler-errors

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 3, 2024
@tmandry tmandry added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-traits Area: Trait system T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` F-refine `#![feature(refine)]`; RFC #3245 and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 3, 2024
@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this today in triage and developed a consensus to:

  • Add a separate lint against impls that refine a return type defined with RPITIT even when the trait is not crate public.
  • Place that in a lint group along with the analogous crate public lint.
  • Create an issue to solicit feedback on these lints (or perhaps two separate ones).
  • Have the warnings displayed with each lint reference this issue in a similar manner to how we do that today with the required Self: '0' bound on GATs.
  • Make a note to review this feedback on 2-3 release cycles.

@rustbot rustbot removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Jan 25, 2024
fmease added a commit to fmease/rust that referenced this issue Mar 16, 2024
…-errors

Split refining_impl_trait lint into _reachable, _internal variants

As discussed in rust-lang#119535 (comment):

> We discussed this today in triage and developed a consensus to:
>
> * Add a separate lint against impls that refine a return type defined with RPITIT even when the trait is not crate public.
> * Place that in a lint group along with the analogous crate public lint.
> * Create an issue to solicit feedback on these lints (or perhaps two separate ones).
> * Have the warnings displayed with each lint reference this issue in a similar manner to how we do that today with the required `Self: '0'` bound on GATs.
> * Make a note to review this feedback on 2-3 release cycles.

This points users to rust-lang#121718 to leave feedback.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2024
Rollup merge of rust-lang#121720 - tmandry:split-refining, r=compiler-errors

Split refining_impl_trait lint into _reachable, _internal variants

As discussed in rust-lang#119535 (comment):

> We discussed this today in triage and developed a consensus to:
>
> * Add a separate lint against impls that refine a return type defined with RPITIT even when the trait is not crate public.
> * Place that in a lint group along with the analogous crate public lint.
> * Create an issue to solicit feedback on these lints (or perhaps two separate ones).
> * Have the warnings displayed with each lint reference this issue in a similar manner to how we do that today with the required `Self: '0'` bound on GATs.
> * Make a note to review this feedback on 2-3 release cycles.

This points users to rust-lang#121718 to leave feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-traits Area: Trait system F-refine `#![feature(refine)]`; RFC #3245 F-return_position_impl_trait_in_trait `#![feature(return_position_impl_trait_in_trait)]` T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants