Open fmease opened 4 months ago
This change affects diesel in a major feature breaking way :disappointed:
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.
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
.
I'm curious as to why diesel
didn't show up in the various crater runs we did, let me double-check.
@fmease Did you already had some time to investigate that. I would like to resolve this before cutting a new diesel release.
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.
@fmease I've filled https://github.com/diesel-rs/diesel/pull/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.
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
This will produce: