Add support for feature-gated columns in table! macro#4961
Add support for feature-gated columns in table! macro#4961LucaCappelletti94 wants to merge 3 commits intodiesel-rs:mainfrom
table! macro#4961Conversation
|
This PR should be ready for review. The Windows 2025 + PostgreSQL CI failure is unrelated to these changes - it's a linker error ( |
|
I have just slammed against the case of I guess that while it is somewhat reasonable to feature gate the single columns, I suppose that if the primary key changes such as in this case, one really needs to feature gate the entire table. Do you have any better idea? |
weiznich
left a comment
There was a problem hiding this comment.
I'm not really in favour introducing a feature that allows users to generate significantly more code in the expansion of a table macro like this.
I've left a few remarks how that could be stripped down + what I would rather like to prefer doing instead even if that's blocked on future language development.
This is not a full review yet, just a dump of related thoughs
|
|
||
| /// Generates all combinatorial variants of aggregate types for feature-gated columns. | ||
| /// | ||
| /// This function generates 2^n variants of `all_columns`, `AllColumns`, and `SqlType` |
There was a problem hiding this comment.
I would rather like to avoid generating an exponential amount of instances to any user input. The cost of compiling large schemas is already large enough, so I rather do not want to introduce another feature to make this even worse. Either we find a different solution to the underlying problem or we need to accept that certain things require a bit more code (e.g. require the user to put a cfg on the table itself and not only on a specific column.)
For reference there is an open RFC to possible allow putting #[cfg] on tuple types like required to solve this in a better way: rust-lang/rfcs#3532 There are no concerns listed there and there seems to be a slight push to accept that (at least a few months back). So it might be worth to rather push for accepting this RFC and then just implementing the relevant feature.
Other than that, given that this is already allowed for tuple expressions it should be rather simple to get the lang team to also accept it for types. Also it shouldn't be too hard to stabilize this, as there is not much that could go wrong there as far as I can tell.
| let mut conditions: Vec<TokenStream> = Vec::new(); | ||
|
|
||
| for (i, group) in groups.iter().enumerate() { | ||
| let is_enabled = (enabled_mask & (1 << i)) != 0; |
There was a problem hiding this comment.
It's probably not worth to use a bitmask here compared to just a Vec<boo>. The later is better readable and the code shouldn't be that performance critical to need to safe that few bytes of memory.
| impl diesel::Table for table { | ||
| type PrimaryKey = #primary_key; | ||
| type AllColumns = (#(#column_names,)*); | ||
|
|
||
| fn primary_key(&self) -> Self::PrimaryKey { | ||
| #primary_key | ||
| } | ||
|
|
||
| fn all_columns() -> Self::AllColumns { | ||
| (#(#column_names,)*) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is no reason to duplicate al of that. Only the two associated types need to be put behind a cfg flag. For tuple expressions it's allowed to put a field behind a cfg flag.
|
To illustrate concretely how I am using this, here are a few
I didn't notice a particular increase to the (already rather long) compile times, and this schema I think is rather large for my standards. While I agree that it is always desirable to avoid increase of compile time imposed by default in general, here I feel it is more of a pay-as-you-use-it type of feature. Tables without feature gates should not experience any added cost, and the code that a person needs to edit is exponentially less (before it was 2^n where n is the number of feature gates). Of course, it is very regrettable that the language at this time does not allow for a cleaner solution. I will apply the suggested changes and see if I can further trim it down. |
This PR adds support for placing
#[cfg(...)]attributes on columns within thetable!macro. This allows users to conditionally include columns based on feature flags, which is useful when certain columns are variable, like when the underlying schema changes with certain versions and we want to support several versions (today's PR triggering example for me was Postgres 18 and 17).Resolves discussion #4922
Example Usage
Implementation Details
Rust doesn't allow
#[cfg]attributes on tuple type elements directly. This means we can't simply write:For
ndistinct cfg groups, we generate2^nvariants of all aggregate types, each guarded by the appropriate combination of#[cfg(all(...))]conditions:The following are generated with combinatorial variants:
all_columnsconstantSqlTypetype aliasimpl diesel::Table for table(specificallyAllColumnsassociated type)impl ValidGrouping<__GB> for starAll trait implementations for individual columns (e.g.,
Expression,QueryFragment,SelectableExpression,AppearsOnTable, operator impls) are wrapped with the column's#[cfg(...)]attributes so they only exist when the feature is enabled.The
IsContainedInGroupBycross-product impls between column pairs include cfg attributes from both columns.New Helper Functions
Added and documented:
CfgGroup- Groups columns by their cfg conditioncollect_cfg_groups()- Organizes columns into cfg groupsgenerate_combined_cfg_condition()- Creates#[cfg(all(...))]with enabled/disabled conditionsgenerate_aggregate_variants()- Generates 2^n variants ofall_columns,SqlType,ValidGroupinggenerate_kind_specific_impls_variants()- Generates 2^n variants ofTable/QueryRelationimplsTests Added
all_columnsfor each variantSqlTypefor each variantAllColumnsassociated type for each variantValidGroupingbounds forstarLimitations
With many distinct cfg groups, (feature-gated) code size grows exponentially (2^n). This is an inherent limitation of Rust's cfg system. In practice, most tables will have at most 1-2 distinct feature groups, keeping the generated code manageable.