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

Things to fix once specialization is in stable #130

Open
sgrif opened this Issue Jan 23, 2016 · 3 comments

Comments

Projects
None yet
1 participant
@sgrif
Member

sgrif commented Jan 23, 2016

This list is probably non-exhaustive, but we should try and add to it as we remember issues or new ones come up

  • We are currently brute forcing the impl of FromSqlRow. This should entirely be covered by FromSql implies FromSqlRow and our tuple impls. The addition of the DB parameter made this invalid under coherence, but we can hopefully go back to our old system w/ specialization
  • We should be able to have FromSqlRow imply Queryable
  • We should be able to remove the selectable_column_workaround! macro, and be able to write impl<Left, Right, T, ST> SelectableColumn<JoinSource<Left, Right>, ST> for T where T: SelectableColumn<Left> and impl<Left, Right, T, ST> SelectableColumn<JoinSource<Left, Right>, ST> for T where T: SelectableColumn<Right>. This would have required lattice
  • Replace SupportsReturningClause and SupportsDefaultKeyword with a blanket impl and a refinement for SQLite. This will likely mean that we can simplify several where clauses as well. (As far as I'm aware, SQLite is the only case where these aren't supported. Other backends should be able to specialize if this is not the case). The where clauses about Insertable in the migrations mod can go away after this. What we really want here is negative reasoning. Most of these cases we don't want to specialize for SQLite, we want to implement a trait for everything except SQLite.
  • Specialize the QueryFragment<Sqlite> impl for Now -- SQLite does not have a now function. While the string 'now' is technically valid on all platforms, I'd prefer to write something a little bit more "correct"

sgrif added a commit that referenced this issue Jan 23, 2016

Make `ToSql` and `FromSql` generic over the backend
This was a mostly mechanical change, but there were several points of
interest.

I've done my best to audit impls which I consider generic, vs impls that
are PG specific. This implies that an impl even can be considered
generic. This mostly doesn't apply to SQLite, since due to the way its
bind parameters are handled I don't expect that types will go through
`ToSql`. `FromSql` I'm still unclear on, and that might change further.
However, after reading through the `MySQL` docs for bind params, I'm
reasonably confident that for string types, binary types, and numeric
types which have an equivalent C primitive, they will always be
represented this way. SQL standard types which I believe will generally
very by backend are date/time types, and decimal.

As part of this change, our blanket impl causing `FromSql` implying
`FromSqlRow` becomes invalid. I won't go into too much detail here, as
I've written up a pre-RFC on a solution that goes into more detail at
https://internals.rust-lang.org/t/pre-rfc-sealed-traits/3108/1, and I
think this issue goes away once specialization is stable (which is
tracked at #130).

With this change, I believe the only remaining trait which needs to be
made generic is `NativeSqlType` (though I'm legitimately unsure if we
actually want to do that, due to the complications it adds with our
expression heirarchy related to `Bool`, and I do not think I want
`Expression` to be generic over the backend)
@sgrif

This comment has been minimized.

Member

sgrif commented Apr 17, 2016

We can probably use default impl/partial impl to make it easier to add new AST passes. We have a lot of AST nodes that are composed of exactly 1 or 2 elements, we can add a BinaryQueryFragment trait and do something like:

default impl<DB, T> QueryFragment<DB> for T where
    DB: Backend,
    T: BinaryQueryFragment<DB>,
{
    fn is_safe_to_cache_prepared(&self) -> bool {
        self.left().is_safe_to_cache_prepared() &&
            self.right().is_safe_to_cache_prepared()
    }
}

Right now I'm having to manually add these for a ton of nodes, and it's quite painful

@sgrif

This comment has been minimized.

Member

sgrif commented Apr 19, 2016

We should see if we can remove the need to ever specify QueryId as a constraint by adding a blanket impl for all T, maybe with the constraint of T: QueryFragment<Debug> or something. If I could, I would make QueryId a supertrait of QueryFragment but that would mean that I have to specify the query id type when it is boxed, which we don't want to do.

But right now if a type forgets to implement it, the error message is really hard to track down. It's similar to if QueryFragment wasn't implemented, but that's much less likely.

rogeruiz added a commit to rogeruiz/tick that referenced this issue Nov 21, 2016

Add save functionality; 👏
This helps put a major dent in diesel-rs/diesel#376. I was getting stuck
here most likely because of what's remaining on diesel-rs/diesel#130.
I'm not sure what differences SQLite has to Postgresql in terms of not
being about to use `get_result`, but the guide for diesel-rs/diesel#376
should either address the differences by suggesting you use `execute`
instead. Although, I'm not sure how to reproduce the functionality for
`get_result` using SQLite? Maybe a quick call to the database to
retrieve the latest one? We wouldn't know the id, so finding items in
the database would be almost impossible or at best super hacky.
@sgrif

This comment has been minimized.

Member

sgrif commented Sep 21, 2017

I'd like to have

for<'a> &'a NewMigration<'a>: Insertable<__diesel_schema_migrations, T::Backend>,
just be for<'a> &'a str: ToSql<VarChar, T::Backend>, and use an ad-hoc insert in that file. However, that's going to fail because ColumnInsertValue requires that DB: SupportsDefaultKeyword or DB == Sqlite. With specialization we can make the SQLite impl a blanket impl, and then specialize it for DB: SupportsDefaultKeyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment