Skip to content
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

Auto type binary operators #4272

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Sep 20, 2024

This adds support for type inference of e.g. some_timestamptz_column - some_pginterval_expression.

Also adds support for update(...) expression (I found myself needing it for the test).

@Ten0 Ten0 force-pushed the auto_type_binary_operators branch 3 times, most recently from 3c3b8f7 to c93e588 Compare September 20, 2024 11:11
@Ten0
Copy link
Member Author

Ten0 commented Sep 20, 2024

The ubuntu mysql fails seem unrelated.

@Ten0 Ten0 requested a review from a team September 20, 2024 12:01
@weiznich
Copy link
Member

Yes the test failures are unrelated. I hadn't the time to investigate them yet. What seems to be really strange is that the test itself passes, but nextest then reports a sigsev

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 52 filtered out; finished in 0.00s

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this change looks good to me. Thanks for submitting.

I think PartialEq is not handled correctly yet, we might just need to mark it as unsupported for now?

syn::BinOp::BitOr(_) => "BitOr",
syn::BinOp::Shl(_) => "Shl",
syn::BinOp::Shr(_) => "Shr",
syn::BinOp::Eq(_) => "PartialEq",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that case won't work. PartialEq is neither a member of std::ops nor has it an associated type Output. Maybe we can just mark it as unsupported for now?

Copy link
Member Author

@Ten0 Ten0 Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right this one should have been among the "always boolean". 👍
fixed.

@Ten0 Ten0 force-pushed the auto_type_binary_operators branch 2 times, most recently from 22a954c to 33cfe72 Compare September 25, 2024 12:57
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, I just added a correction to remove the now wrong comment about the fact that we implement all the ops as PartialEq is now missing.

@Ten0
Copy link
Member Author

Ten0 commented Sep 25, 2024

Thanks for the update, I just added a correction to remove the now wrong comment about the fact that we implement all the ops as PartialEq is now missing.

It's not missing: there is no PartialEq BinOp: there's only an Eq BinOp (which Rust calls PartialEq for, which is mapped, to bool, and was incorrectly mapped to <Lhs as std::ops::PartialEq<Rhs>>::Output before).
What I mean by this comment is that our match is in fact exhaustive at the time of writing, and will likely stay so unless Rust adds new operators, despite what the _ => branch might suggest.

@weiznich
Copy link
Member

You are correct, I just search for PartialEq and missed that it's only Eq 🤦
I've reverted the removal, so everything should be fine now.

@weiznich weiznich added this pull request to the merge queue Sep 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2024
@weiznich weiznich force-pushed the auto_type_binary_operators branch 2 times, most recently from 8edd17b to 33cfe72 Compare September 26, 2024 07:12
This adds support for type inference of e.g. `some_timestamptz_column - some_pginterval_expression`
@weiznich weiznich added this pull request to the merge queue Sep 27, 2024
Merged via the queue into diesel-rs:master with commit ce6fa0b Sep 27, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants