Replace "DynIden" with "IdenImpl" by Huliiiiii · Pull Request #892 · SeaQL/sea-query

Huliiiiii · 2025-05-29T21:08:43Z

Background: #795 (comment)

PR Info

Closes

Dependencies:

Dependents:

New Features

Bug Fixes

Breaking Changes

Replaced DynIden with concrete struct IdenImpl.
- How to migrate:
  - If you use the derive macro Iden, you don't need to do anything.
  - If you implemented trait Iden manually, you need to replace it with impl From<YourType> for IdenImpl.
Unlike DynIden, IdemImpl is compared purely by string. Same strings rendered from different Rust identifier types will now compare as equal.
Remove SeaRc.
- How to migrate:
  - In SeaORM and SeaQuery, SeaRc was only used for DynIden. So, after you fix the errors around DynIden, you shouldn't have any errors left.
  - But if you used SeaRc for your own purposes outside of DynIden, there can be more errors. There, you can try replacing SeaRc with RcOrArc (which was the underlying implementation of SeaRc anyway).

Changes

Expurple · 2025-06-01T05:45:22Z

If you don't mind, I'll review when the CI passes. But feel free to ask if you need my review earlier

Huliiiiii · 2025-06-01T20:37:29Z

Because it is too troublesome to fix the tests and examples, I also implemented the corresponding behavior for the derive macro Iden in this PR.

Huliiiiii · 2025-06-07T10:19:02Z

It seems that all errors are caused by deprecation.

tyt2y3 · 2025-06-08T19:30:59Z

sorry, it passed CI already. then there's a conflict

Expurple

I love this!

src/types.rs

Expurple · 2025-06-09T05:09:43Z

src/extension/mysql/column.rs

+impl From<MySqlType> for IdenImpl {
+    fn from(value: MySqlType) -> Self {
+        Self::from(match value {
+            MySqlType::TinyBlob => "tinyblob",
+            MySqlType::MediumBlob => "mediumblob",
+            MySqlType::LongBlob => "longblob",
+        })
+    }
+}


This duplicated match (with impl Iden right above) doesn't look right. I think, in this iteration (this PR that doesn't delete trait Iden yet) we should blanket impl<T> Iden for T where T: Into<IdenImpl>.

Also, your PR description should mention the plan to delete the trait, so that it's easier for Chris to review and understand the whole picture (there will be nothing called IdenImpl eventually)

After this PR, the Iden trait is no longer used in the actual code. I think we can remove these impls?

If the trait is already unused (there's no meaningful "intermediate" state of this trait in this PR), then I'd say we should just delete it and rename IdenImpl -> Iden right in this PR

You're right. My current choice is mainly for compatibility, but since users will have to re-implement Into<IdenImpl> anyway, it makes sense to replace Iden.

Hmm. As I think about it, there's another, more compatibible option. The blanket impl could go the other way around: impl<T> From<T> for IdenImpl where T: Iden. That way, if users have manual Iden impls, these types will continue to work where IntoIden is accepted.

But the downside is that the struct will be called IdenImpl or something like that (Iden is still taken by the trait). And there's more complexity overall because the trait is still around.

I'd say, we should clean things up and delete the trait.

I'm having some issues while cleaning up the code. Keeping IdenStatic requires a lot of (dirty) fixes. But since the current Iden is already static, I think we can remove IdenStatic as well?

Yeah, right. Nuke it. The lifetime of Rust identifier types no longer matters, because we no longer keep them inside DynIden. Instead, we eagerly "render" them into an owned struct Iden, and then deal only with that.

I really like your new design and cleanups. Less traits and lifetimes; more simple, concrete, owned values.

src/types.rs

Expurple · 2025-06-09T11:15:54Z

sea-query-derive/tests/pass/renamed_unit_struct.rs


 fn main() {
-    assert_eq!(CustomName.to_string(), "another_name");
+    assert_eq!(Iden::from(CustomName).to_string(), "another_name");


Hmm. Is this pattern common in the actual user code? I see a lot of this in the diff. Perhaps, we should find a way to reduce this breakage and verbosity. E.g., add a helper trait ToIdenSting that's blanket-implemented for Into<Iden> and provides this to_string method from the old Iden trait?

I think, it won't be as bad as the old Iden trait, because SeaQuery interfaces wouldn't depend on ToIdenSting for anything. It's just a helper "extension trait", and we can document that

I don't think that's very common. But we could add a trait to provide the corresponding Iden methods for any T: Into.

Yeah, I think we can provide that extension trait as IdenTrait or something like that.

On one hand, it's a little messy because we keep much of the old API surface.

On the other hand, the diff indicates that the trait was actually used outside of DynIden. So, it's probably worth keeping. And we still improve the API by making it an optional extension trait and no longer depending on it via DynIden.

Oh, I have an even crazier (or even more boring and conservative?) idea. What if we keep even the name of trait Iden the same, and keep your Cow struct called DynIden? This naming even makes sense, because struct DynIden is a "raw" identifier string that's no longer "type safe" and could refer to any SQL entity

This even preserves backward-compatibility with user code that uses the actual dyn Iden. We'll just no longer use it in SeaQuery

For even more compatibility, we can:

Reverse the blanket impl: impl<T> From<T> for DynIden where T: Iden.

Keep generating impl Iden in #[derive(Iden)].

Keep the users' manual impl Iden working

🤯🤯🤯

That's the issues with this implementation:

The Iden trait doesn't provide a method to get a &'static str, so we can only implement
impl<T> From<T> for DynIden where T: IdenStatic.

Therefore, we cannot solve the problem of derive(Iden) because this trait can be implemented in a non-static way.

One option is to add a method to Iden trait that returns a Cow, but in practice, I think this isn't much different from implementing the Iden trait for types that implement Into<IdenImpl>.

There are still more issues: users can implement the Iden trait themselves to override the default behavior—for example, to ignore quote as a way to bypass escaping. The current idenimpl cannot provide the same behavior.

impl<T> From<T> for DynIden where T: Iden we can, but we have to always return Owned, which is not ideal.
yeah, IdenStatic is somehow added later than Iden that made things complicated.

for example, to ignore quote as a way to bypass escaping

I think we ultimately have to reject this use case as part of the breaking change

I think we ultimately have to reject this use case as part of the breaking change

Ok, that means that we must delete the provided Iden methods, so that they can not be overridden [1]. We can keep their calls working [2] by adding a blanket-implemented trait IdenExt. This solution makes sense to me.

[1]: Whether we decide to keep trait Iden for some compatibility, or not.
[2]: Actually, the calls will break initially, but the users only need to use sea_query::IdenExt as the compiler suggests.

Expurple · 2025-06-09T11:24:47Z

sea-query-derive/tests/pass-static/3_level_flatten.rs

-use sea_query::{Iden, IdenStatic};
-use strum::{EnumIter, IntoEnumIterator};
-
-#[derive(Copy, Clone, IdenStatic, EnumIter)]


Sorry for noticing this only now, but perhaps we should keep IdenStatic derive macro for compatibility? Just as an alias that does the same thing as #[derive(Iden)].

Hmm, perhaps it shouldn't even be an alias, and we should keep the trait IdenStatic and blaknet-impl<T> From<T> for Iden where T: IdenStatic? That would be nice for compatibility, and also for providing a compile-time guarantee that you can cheaply get a &static str

We could make the derive(IdenStatic) do nothing and implement as_str and AsRef for Iden.

On this line of code that I have reviewed, #[derive(IdenStatic)] is used without #[derive(Iden)]. It used to implicitly generate impl Iden in addition to impl IdenStatic.

If we make #[derive(IdenStatic)] do nothing, then code like this breaks completely (no longer gives us an IntoIden type), instead of generating a partially-compatible impl From<_> for Iden that at least still allows the type to be passed into methods that accept IntoIden

Oh, I see — IdenStatic actually implements both Iden and IdenStatic, so maybe we just need to re-export the derive(Iden) macro as derive(IdenStatic)?

That's what I've initially suggested here, yeah. That's the minimal viable solution, IMO.

Whether we should additionally preserve trait IdenStatic, is a more debatable design tradeoff

tyt2y3 · 2025-06-16T20:41:03Z

sorry for the late reply. I think this is in the right direction. however, I am afraid there's still too much breaking change.

having said that, I think most of the changes can be polyfilled:

Iden::to_string we can have a struct Iden and a generic function to_string?
SeaRc::new we can again have a helper struct SeaRc and a method new?
.into_iden we can add this method to Alias?

Huliiiiii · 2025-06-17T01:33:59Z

First, I don't think it's a good practice to name this method to_string as it conflicts with the Display trait method, which violates Rust conventions. I suggest renaming it to to_iden_string or to_sql_string instead.
Second, the current issue is that most existing code calls the trait method Iden::to_string on enums or zero size types. There's no clean migration path for the existing code with this approach.
SeaRc can be kept, but it will be unused.
Yes, we can add into_iden for Alias, but I think a better approach would be to remove Alias (Or keep it for compatibility), since the current changes implement Into<IdenImpl> for both &'static str and String.

Expurple · 2025-06-17T06:15:34Z

I don't think it's a good practice to name this method to_string as it conflicts with the Display trait method, which violates Rust conventions. I suggest renaming it

I agree with you in principle, but we're faced with a peculiar task of balancing between APIs that make more sense and APIs that are more compatible. I think, the right way is keeping to_string in 2.0.0 and perhaps deprecating it sometime later in 2.x.x, giving plenty of time to migrate before 3.0.0. 2.0.0 has plenty of other, absolutely necessary breaking changes that have to be fixed in one go.

There's no clean migration path for the existing code with this approach.

There is one that I proposed here: blanket impl<T> OldIdenTrait for T where T: Into<NewIdenStruct> (OldIdenTrait and NewIdenStruct is for clarity, it's not the actual proposed naming). It will keep all Iden method calls working.

This will still break manual impl Iden (if the type also implements Into<NewIdenStruct>) and the code calling methods that accept NewIdenStruct (if the type does not implement Into<NewIdenStruct>). But that breakage is almost inherent to your design.

The only really-non-breaking alternative is reversing the blanket impl as impl<T> From<T> for NewIdenStruct where T: OldIdenTrait. As you've noted, this implies that the Cow will be Owned more often than we'd like.

SeaRc::new we can again have a helper struct SeaRc and a method new?

Yeah, we can keep if for compatibility and later deprecate in 2.x.x.

.into_iden we can add this method to Alias?

This PR already keeps impl<T> IntoIden for T where T: Into<NewIdenStruct> as an extension trait for compatibility. The method calls will keep working.

This still breaks manual impl IntoIden when the type also implements Into<NewIdenStruct>, but that's pretty much by design.

If the type manually implements IntoIden without implementing Into<NewIdenStruct>, then it can't be passed into methods that accept Into<NewIdenStruct>. But we can solve this by making trait IntoIden: Into<NewIdenStruct>. I think, this makes a lot of sense. That's what I plan doing with all Into* traits.

I think a better approach would be to remove Alias (Or keep it for compatibility), since the current changes implement Into<IdenImpl> for both &'static str and String.

I'm also strongly in favor of adding an impl for String and keeping Alias only for compatibility: #882. And two more users share their frustrations and hacks in that thread.

tyt2y3 · 2025-06-19T09:32:06Z

I would probably even not put in the deprecated notice to avoid causing tons of compiler warnings in the first release.

I'm also strongly in favor of adding an impl for String and keeping Alias only for compatibility: #882. And two more users share their frustrations and hacks in that thread.

well, I guess my argument is that String is data and I don't want it to be confused as identifier. example:

let a = String::new("a");
let b = String::new("b");

`a.eq(b)` would result in `"a" = 'b'`
`b.eq(a)` would result in `"b" = 'a'`

one is quoted as identifier, one is quoted as literal. am I right that it will cause this confusion ultimately?

Expurple · 2025-06-19T10:04:36Z

I would probably even not put in the deprecated notice to avoid causing tons of compiler warnings in the first release.

Yeah, agree. Definitely not in 1.0.0.

I guess my argument is that String is data and I don't want it to be confused as identifier

That's an interesting argument and we should explore it. But it needs a convincing example. Your pseudocode example is incorrect. The non-pseudocode

// [dependencies]
// sea-query = { version = "0.32.3", features = ["tests-cfg"] }

use sea_query::{ExprTrait, PostgresQueryBuilder, QueryBuilder};

fn main() {
    let a = String::from("a");
    let b = String::from("b");

    let mut sql = String::new();
    PostgresQueryBuilder.prepare_simple_expr(&a.clone().eq(&b), &mut sql);
    println!("{}", sql);

    let mut sql = String::new();
    PostgresQueryBuilder.prepare_simple_expr(&b.eq(&a), &mut sql);
    println!("{}", sql);
}

prints

'a' = 'b'
'b' = 'a'

And to me, that's obvious and expected. Identifiers don't even implement ExprTrait. It's impossible to call eq directly on an identifier string. "a" = 'b' can be produced only from something like Expr::col(Alias::new(a)).eq(b). Or Expr::col(a).eq(b), after we impl Iden for String. The latter looks great to me.

Expurple · 2025-07-01T09:41:39Z

@Huliiiiii, we've chatted with Chris and decided to split the PR in order to proceed.

Initially, we want to merge these two small changes as separate PRs:

impl Iden for String
Alter the PartialEq impl to be safe, ignore the types and compare DynIdens as strings

That's a blocker for now.

Huliiiiii · 2025-07-01T18:40:04Z

@Huliiiiii, we've chatted with Chris and decided to split the PR in order to proceed.

Initially, we want to merge these two small changes as separate PRs:

impl Iden for String

Alter the PartialEq impl to be safe, ignore the types and compare DynIdens as strings

That's a blocker for now.

I haven’t had time to work on this lately. I’m totally fine if you wants to take over or use this as a base for further changes.

tyt2y3 · 2025-07-10T13:37:05Z

I’m totally fine if you wants to take over or use this as a base for further changes.

I will try some experiments

tyt2y3 · 2025-07-10T13:52:58Z

thank you so much for the initiative!

Huliiiiii added 4 commits May 30, 2025 00:53

Type Id

5495252

No type id

60ff624

no opt

d7b7ec5

Clean up and fix doc test

e04a09c

Huliiiiii added 5 commits June 2, 2025 02:50

Refactor

3395c1a

Fmt

6e0ee0f

Fix derive macro

4ec47c1

Fix doc tests and derive macro

11e82c5

Fmt

93a390e

Fix bench

896b2b5

Huliiiiii marked this pull request as ready for review June 1, 2025 20:48

Deprecate Iden and DynIden

4d9f98f

Huliiiiii added 2 commits June 7, 2025 18:21

Comment out deprecated attrs

67b8ec0

Fmt

dbdb644

Merge remote-tracking branch 'upstream' into no-opt

8cf046b

Expurple requested changes Jun 9, 2025

View reviewed changes

Expurple reviewed Jun 9, 2025

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

Huliiiiii added 4 commits June 9, 2025 14:39

Update

b40d108

Remove iden trait with dirty fix

a26e9e8

Remove IdenStatic

f26534f

Fmt

ece6ac4

Expurple reviewed Jun 9, 2025

View reviewed changes

Huliiiiii closed this Jul 1, 2025

tyt2y3 reopened this Jul 10, 2025

Merge branch 'master' into no-opt

d633ca0

tyt2y3 changed the base branch from master to new-iden July 10, 2025 13:36

tyt2y3 merged commit 483de91 into SeaQL:new-iden Jul 10, 2025
19 of 20 checks passed

Uh oh!

Conversation

Huliiiiii commented May 29, 2025 • edited by Expurple Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Info

New Features

Bug Fixes

Breaking Changes

Changes

Uh oh!

Expurple commented Jun 1, 2025

Uh oh!

Huliiiiii commented Jun 1, 2025

Uh oh!

Huliiiiii commented Jun 7, 2025

Uh oh!

tyt2y3 commented Jun 8, 2025

Uh oh!

Expurple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Huliiiiii Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tyt2y3 Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Expurple Jun 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Expurple Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Huliiiiii commented May 29, 2025 •

edited by Expurple

Loading

Expurple Jun 9, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

Huliiiiii Jun 11, 2025 •

edited

Loading

tyt2y3 Jun 29, 2025 •

edited

Loading

Expurple Jun 29, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

Expurple Jun 9, 2025 •

edited

Loading

tyt2y3 commented Jun 16, 2025 •

edited

Loading

Huliiiiii commented Jun 17, 2025 •

edited

Loading

Expurple commented Jun 17, 2025 •

edited

Loading

tyt2y3 commented Jun 19, 2025 •

edited

Loading

Expurple commented Jul 1, 2025 •

edited

Loading