Tracking issue for future-incompatibility lint uncovered_param_in_projection

[fmease]

This is a tracking issue for the lint uncovered_param_in_projection which was added in #117164.

The lint detects a violation of one of Rust's orphan rules for foreign trait implementations that concerns the use of type parameters inside trait associated type paths ("projections") whose output may not be a local type that is mistakenly considered to "cover" said parameters which is unsound and which will be rejected by a future version of the compiler.

Originally reported in #99554 (kept open) and tracked in rust-lang/types-team#104.

Example

// dependency.rs

#![crate_type = "lib"]

pub trait Trait<T, U> {}

// dependent.rs

trait Identity {
    type Output;
}

impl<T> Identity for T {
    type Output = T;
}

struct Local;

impl<T> dependency::Trait<Local, T> for <T as Identity>::Output {}

fn main() {}

This will produce:

warning[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
  --> dependent.rs:11:6
   |
11 | impl<T> dependency::Trait<Local, T> for <T as Identity>::Output {}
   |      ^ type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #124559 <https://github.com/rust-lang/rust/issues/124559>
   = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
   = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last
   = note: `#[warn(uncovered_param_in_projection)]` on by default

[weiznich]

This change affects diesel in a major feature breaking way 😞

You can reproduce this by using the following steps:

git clone https://github.com/diesel-rs/diesel
cd diesel
cargo +nightly check -p diesel_cli

Which produces the following warning:

warning[E0210]: type parameter `Col` must be covered by another type when it appears before the first local type (`database::multi_connection_impl::backend::MultiBackend`)
   --> diesel_cli/src/database.rs:109:10
    |
109 | #[derive(diesel::MultiConnection)]
    |          ^^^^^^^^^^^^^^^^^^^^^^^ type parameter `Col` must be covered by another type when it appears before the first local type (`database::multi_connection_impl::backend::MultiBackend`)
    |
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #124559 <https://github.com/rust-lang/rust/issues/124559>
    = note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
    = note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last
    = note: `#[warn(uncovered_param_in_projection)]` on by default
    = note: this warning originates in the derive macro `diesel::MultiConnection` (in Nightly builds, run with -Z macro-backtrace for more info)

The relevant impl generated by the proc macro looks like this:

    impl<Col, Expr>
            diesel::insertable::InsertValues<
                Col::Table,
                super::multi_connection_impl::backend::MultiBackend,
            >
            for diesel::insertable::DefaultableColumnInsertValue<
                diesel::insertable::ColumnInsertValue<Col, Expr>,
            >
        where
            Col: diesel::prelude::Column,
            Expr: diesel::prelude::Expression<SqlType = Col::SqlType>,
            Expr: diesel::prelude::AppearsOnTable<
                diesel::internal::derives::multiconnection::NoFromClause,
            >,
            Self: diesel::query_builder::QueryFragment<
                super::multi_connection_impl::backend::MultiBackend,
            >,
            diesel::insertable::DefaultableColumnInsertValue<
                diesel::insertable::ColumnInsertValue<Col, Expr>,
            >: diesel::insertable::InsertValues<
                Col::Table,
                <PgConnection as diesel::connection::Connection>::Backend,
            >,
            diesel::insertable::DefaultableColumnInsertValue<
                diesel::insertable::ColumnInsertValue<Col, Expr>,
            >: diesel::insertable::InsertValues<
                Col::Table,
                <SqliteConnection as diesel::connection::Connection>::Backend,
            >,
        {
            fn column_names(
                &self,
                mut out: diesel::query_builder::AstPass<
                    '_,
                    '_,
                    super::multi_connection_impl::backend::MultiBackend,
                >,
            ) -> QueryResult<()> {
                use diesel::internal::derives::multiconnection::AstPassHelper;
                match out.backend(){
                    super::backend::MultiBackend::Pg(_) => {
                        <Self as diesel::insertable::InsertValues<Col::Table, <PgConnection as diesel::connection::Connection> ::Backend>> ::column_names(&self,out.cast_database(super::bind_collector::MultiBindCollector::pg,super::query_builder::MultiQueryBuilder::pg,super::backend::MultiBackend::pg, |l|{
                            <PgConnection as diesel::internal::derives::multiconnection::MultiConnectionHelper> ::from_any(l).expect("It's possible to downcast the metadata lookup type to the correct type")
                        },),)
                    },
                    super::backend::MultiBackend::Sqlite(_) => {
                        <Self as diesel::insertable::InsertValues<Col::Table, <SqliteConnection as diesel::connection::Connection> ::Backend>> ::column_names(&self,out.cast_database(super::bind_collector::MultiBindCollector::sqlite,super::query_builder::MultiQueryBuilder::sqlite,super::backend::MultiBackend::sqlite, |l|{
                            <SqliteConnection as diesel::internal::derives::multiconnection::MultiConnectionHelper> ::from_any(l).expect("It's possible to downcast the metadata lookup type to the correct type")
                        },),)
                    },

                    }
            }
        }

As far as I understand this issue this is the desired behavior that these impls are no longer valid . Removing this behavior is highly problematic for diesel as I do not see any other way to write that impl without major breaking changes to diesel itself. It's even likely that this would force us to remove a large popular feature from diesel itself in response to this breaking change.

[fmease]

I will look into this later. Note that the current orphan rules are unsound.

It's possible that the warning emitted for diesel are false positives (FPs) because the fix / lint for this unsoundness that is implemented on nightly (normalizing alias types in a certain way) is constrained by some known limitations of both trait solvers and that may lead to FPs.

Let me see what is the case for diesel.

[fmease]

I'm curious as to why diesel didn't show up in the various crater runs we did, let me double-check.

[weiznich]

@fmease Did you already had some time to investigate that. I would like to resolve this before cutting a new diesel release.

[fmease]

Did you already had some time to investigate that. I would like to resolve this before cutting a new diesel release.

I've pm'ed you.

[weiznich]

@fmease I've filled diesel-rs/diesel#4050 on diesels side to fix this. This was likely not picked up by crates as it is a change that only landed on the master branch yet. It's not in any released diesel version yet.