Documentation improvements

[weiznich]

• Introduce a feature to use the unstable rustdoc cfg_attr feature and
  classify all public items acoordinoly
• Introduce a
  i-implement-a-third-party-backend-and-opt-into-breaking-changes
  feature flag that is used to hide most of the previously hidden API
  behind this feature flag. The idea is that we can use this feature to
  communicate some documentation about implementing third party backends
  while making clear that we may break those API's potentially
• Change the any macro generated code to only use public API's or
  explicitly as diesel::internal exported API's
• Add a lot documentation in different places
• Make diesel_derives use the with-deprecated/without-deprecated
  feature flags as well
• Simplify the signature of Connection::load even more

Open points:

• go again over the documentation
• Inspect all ignored doc tests to see if they are building and if we could convert them to tests that are compiled
• Fix any test failures
• Maybe split up this commit into smaller ones
• Fix merge conflict
• Remove the doc building for the "special" branch

[weiznich]

A preview of the updated documentation can be found here: https://docs.diesel.rs/diesel_2.0_documentation_update/diesel/index.html

[weiznich]

For anyone coming here from "This week in rust": I want to use this chance to reach out to potential users to get answers to the following questions:

• Is there something that is missing from the API documentation in your opinion?
• Did you remember searching something longer than expected in our documentation? If yes do you remember details?
• Can something else be improved on the existing documentation?
• Anything else you want to tell us about the API documentation.

Please leave a comment here or in this users.rust-lang.org thread.

[mejrs]

Introduce a feature to use the unstable rustdoc cfg_attr feature and
classify all public items acoordinoly

I'm assuming you mean the doc_cfg feature? Sure I can do that.

[weiznich]

@mejrs Just to clarify that: This PR already implements that. Whats left is going over the generated documentation to check if the corresponding boxes appear in all locations.

[mejrs]

@mejrs Just to clarify that: This PR already implements that. Whats left is going over the generated documentation to check if the corresponding boxes appear in all locations.

Ah, okay, that wasn't clear to me. I didn't see that this is a pull request rather than an issue and it very much read like a wishlist to me.

[mejrs]

I've added some questions, remarks and suggestions here and there :)

FWIW, I would recommend using https://docs.rs/visibility rather than doing gymnastics with cfgs and cfg_attr's yourself. It's less likely to mess up and conveys intent better. But I have no idea whether it plays nice with doc_cfg.

[Darksonn]

This seems somewhat dangerous. If I depend on such a backend, then I can also access those unstable features, and might use one without realizing.

[weiznich]

The probably best other solution here is to use a raw cfg flag, right?
The other option that I see is to split up diesel itself into multiple crates and reexport only the stable api via a diesel crate. Everything else is then only exposed via a 0.x internal crate. I assume that this solution will cause problems as well...

[Darksonn]

Well, a raw cfg flag is how Tokio handles unstable features, but it does have the disadvantage that a crate cannot specify it, and that it must be specified by the person running cargo build. (But that's also its advantage and why Tokio uses it over a feature.)

[weiznich]

@diesel-rs/reviewers This is ready for a review now. Especially I would like to hear opinions on the following points:

• Introduction of the i-implement-a-third-party-backend-and-opt-into-breaking-changes feature + classification the corresponding API's?
  • Is that a good idea?
  • Are there API's missing?
  • Are there any API's that you would consider useful for the usage outside of implementing custom connections/backends? (So that we would guarantee stability for them as well)
  • Should that feature flag be a raw cfg flag instead, so noone could accidentally depend on it? (I think no, as it mostly hides stuff that's just useful for implementing a backend/connection, which is something almost no user will do. So there is only a minor chance that someone accidentally depends on a unstable item)
• What do you you think of DieselReserveSpecialization to reserve us the right to specialize certain QueryFragment impls later on? (That might be useful for allowing custom backends to provide custom implementations for stuff, or to specialize stuff in diesel itself)
• What do you think about passing a reference to the Backend type into the query builder? The idea there is to be able to use some runtime information as part of the query building process. Examples here are the version of the backend (so is it sqlite 3.27 or sqlite 3.37?) or possibly in the future even something like which backend at all is used there for a theoretical AnyConnection implementation (I have some rough ideas about that, but nothing written down or even implemented yet)
• What do you think about the added documentation how to implement a custom connection/backend?

[weiznich]

@diesel-rs/reviewers I would like to move on with this PR and plan to merge this at 21.2 if there are no fundamental issues raised till then.

[weiznich]

@diesel-rs/reviewers I would like to merge this tomorrow if there are no objectives.

[Ten0]

Introduction of the i-implement-a-third-party-backend-and-opt-into-breaking-changes feature

Looks good to me :)

• What do you think about passing a reference to the Backend type into the query builder? The idea there is to be able to use some runtime information as part of the query building process. Examples here are the version of the backend (so is it sqlite 3.27 or sqlite 3.37?) or possibly in the future even something like which backend at all is used there for a theoretical AnyConnection implementation (I have some rough ideas about that, but nothing written down or even implemented yet)

I feel like typically, new versions of backends may introduce new syntaxes, but do not modify behavior of old syntaxes.
Because we work as a query builder, we typically map to SQL syntax, and not to functionality.
So for this to be useful, we'd need a new backend version to be released in such a way that makes us want to change the mapping of one syntax to another without just introducing a new function name for that new syntax (e.g. if mysql or sqlite introduced an = ANY( syntax as well in a new version), and even then, if whether the query can be cached from its type is a const we probably won't get the true benefit from it, so we'd need to find a scenario where this is really useful.

I'm not sure to what extent this scenario is likely enough that we want to have the additional complexity of passing a ref to the typically-zst around.

So:

1. What would be a scenario where we need to update the to_sql impl based on backend version but not whether stuff is cacheable and how likely is it? (if super unlikely, solution 1 below is maybe not the best complexity/functionality compromise and I think we'd probably prefer 2 or 3)
2. How likely is the scenario where we need to update the to_sql impl based on backend version but also the cacheability (e.g. ANY) ?

Based on the answers to these questions, we can:

1. Pass the backend ref (current state of this PR) - side note: the ref is not zero-sized even if DB is a zero-sized type. (RFC - Zero-Sized References rust-lang/rfcs#2040)
2. Not pass the backend ref (current state of master)
3. Pass the backend ref, but also get rid of HAS_STATIC_QUERY_ID in trait QueryId and just implement the query_id function directly. This would enable cacheability updates based on the backend version. Does it actually need to be a const anywhere, or could we just rely on the optimizer to inline all the cacheability results? (I imagine we could measure the performance impact of this through the benchmarks?)

On the other topics I'll definitely trust you on writing the doc and moving stuff around to make it more private ^^

[weiznich]

On the other topics I'll definitely trust you on writing the doc and moving stuff around to make it more private ^^

❤️

• What do you think about passing a reference to the Backend type into the query builder? The idea there is to be able to use some runtime information as part of the query building process. Examples here are the version of the backend (so is it sqlite 3.27 or sqlite 3.37?) or possibly in the future even something like which backend at all is used there for a theoretical AnyConnection implementation (I have some rough ideas about that, but nothing written down or even implemented yet)

I feel like typically, new versions of backends may introduce new syntaxes, but do not modify behavior of old syntaxes.
Because we work as a query builder, we typically map to SQL syntax, and not to functionality.
So for this to be useful, we'd need a new backend version to be released in such a way that makes us want to change the mapping of one syntax to another without just introducing a new function name for that new syntax (e.g. if mysql or sqlite introduced an = ANY( syntax as well in a new version), and even then, if whether the query can be cached from its type is a const we probably won't get the true benefit from it, so we'd need to find a scenario where this is really useful.

I'm not sure to what extent this scenario is likely enough that we want to have the additional complexity of passing a ref to the typically-zst around.

Let me try to motivate the current change with some more details.

I see two use cases here:

1. Providing something like:

#[derive(MultiConnection)]
enum MultiConnection {
    Pg(PgConnection),
    Mysql(MysqlConnection),
    // …
}

where #[derive(MultiConnection)] should "magically" generate a Connection/Backend implementation that works for all of the provided connection types. This would require at some point providing QueryFragment<MyMultiConnectionBackend> impls, which need to know for what backend they need to generate SQL. That's probably my main motivation to include this at all and that's something I would like to try after releasing diesel 2.0.
(To be clear here: That's not something that I would see in diesel itself, but rather as third party helper crate.)
2. Providing fall back solutions for newer SQL features for some database system. That's the point you've discussed above. My motivating example here are sqlite returning clauses. Even if that turned out to be not feasible due to sqlite's limitations, there where some ideas to emulate support for returning clauses using CTE's. That would have required that we generate different queries based on the sqlite version. Again we would need to pass some argument to the querybuiding process there to customize it.

So now both motivating use cases are not directly relevant yet, but that's both something that I could imaging to happen while diesel 2.x is the current major diesel version. As that would be a breaking change we can do that either now, or later requiring another major version bump or later by finding another clever solution (for example by marking the corresponding functions as "private"/"i-implement-a-third-party-backend-and-opt-into-breaking-changes".

To address some on for concerns:

I'm not sure to what extent this scenario is likely enough that we want to have the additional complexity of passing a ref to the typically-zst around.

As of this change we wrap the reference into AstPass. That's already a non-zerosized type so putting a bit more information in there should not hurt anything. Additionally I would expect that this reference currently is just removed by the compiler as dead code (haven't checked that…).

What would be a scenario where we need to update the to_sql impl based on backend version but not whether stuff is cacheable and how likely is it? (if super unlikely, solution 1 below is maybe not the best complexity/functionality compromise and I think we'd probably prefer 2 or 3)

Caching already involves more than one layer of stuff. There is QueryId::HAS_STATIC_QUERY_ID and there is AstPass::unsafe_to_cache_prepared. The first one says how something can be cached (either by type id or by sql string), while the second one states if something could be checked. That means the second one (available inside of QueryFragment::walk_ask) always overwrites the first one. Let's go through several potential issues here:

1. Cacheability changes depending on the passed backend data -> That can be addressed by calling unsafe_to_cache_prepared conditionally, as it's already done by several query fragment impls (For example here).
2. The metadata change if a query could be cached by type id or by sql string. -> Just always cache by sql string and accept the minor performance hit which seems to be only relevant for sqlite in memory connections.

[Ten0]

That means the second one (available inside of QueryFragment::walk_ask) always overwrites the first one

Alright that looks good for me then :)

[weiznich]

Thanks for the feedback 👍. I will move on and merge this now.