rust-lang / rust-lang/rust

`refining_impl_trait` only fires on public traits

Open
#119,535 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

A-lints A-trait-system F-refine F-return_position_impl_trait_in_trait T-lang
Dominant language
Rust
Stars
119k
Forks
16.2k
PR merge metrics
PR metrics pending

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reproducing the first Rust code sample with and without pub and observe when the refining_impl_trait lint fires. Read the compiler's implementation of that lint and its existing tests, then make the behavior consistent for private and public traits. Done means the lint reports the refinement mismatch in both cases without changing the intended abstraction-boundary behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.