Streamline quoting

[Expurple]

I was very suprised and confused when I noticed the following in the codebase:

• A comment on Iden::quoted, saying that it doesn't actually return a quoted string (!). In fact, the quoting is done later by the (DB-specific) backend.
• fn is_static_iden that's not actually about knowing the identifier at compile time. In fact, it's about having special characters in the identifier (and the need to escape the identifier in the backend).
• The expensive use of Cow::Owned to indicate the presense of special characters (rather than to simply store a runtime-provided string).

It's so weird and non-obvious.

Here, I streamline things:

• Removed the Iden::quoted method that isn't actually quoted.

  I understand that this is a breaking change. But if the users can't actually get a properly-quoted string without a specific DB backend, then we should help them notice this reality.

• Made Iden::unquoted return a Cow, as that's the method that actually constructs a DynIden (Cow).

  This is a breaking change.

• Replaced IdenStatic: Iden with impl<T> Iden for T where T: IdenStatic.

  Technically a breaking change for manual implementors, but most people probably use derives.

• Removed all other extra supertraits from IdenStatic. All tests pass without those, and it's not clear why those were needed. Those are probably leftowers from the old Iden system. If those are actually needed, we should test and document that.

  Technically a breaking change, although I doubt that many people worked with generic IdenStatic and relied on those implied bounds.

• Replaced impl Iden with impl IdenStatic everywhere where the string is static (e.g., in all derives).

• Removed unnecessary allocations of Cow::Owned.

• Removed is_static_iden.

  This isn't a breaking change because we didn't have that in 1.1.x.

• Updated the docs.

To be fair, I understand the intended meaning and benefit of is_static_iden and indicating it with Cow::Borrowed. When we know at compile time that the identifier doesn't have any special characters, the backend can write_str it as-is, in one go. Because that's no longer the case, my PR may regress the performance a little.

However:

• The correctness of that approach is hard to verify. I'm not sure that we construct Cow::Borrowed only when the string doesn't have special characters. It's easy to choose Cow::Borrowed simply because you have a static string (whether or not it contains special characters). That's the most obvious thing to do.
• My PR wins back performance by avoiding allocations and using Cow::Borrowed more often.
• If we really want to keep a fast write_str path in the backend, we can put the is_static_iden check right there before the write. But I wouldn't worry about that.

The main thing that I'm going for here is conceptual clarity, rather than performance:

• All static identifiers now implement IdenStatic.
• No more qouted that isn't actually quoted.
• No more valid static identifiers that return false from is_static_iden.

[Expurple]

Sorry, missed the workspace tests locally. Marking as draft until I fix those

[Expurple]

Potential optimization idea for the future:

• Restore is_static_iden logic and rename it to something appropriate like may_need_iden_escaping.
• Add a default method to Iden:

  /// Whether the identifier may need to be escaped in the generated SQL.
  ///
  /// By default, we expect that and check the identifier at runtime.
  ///
  /// `#[derive(Iden)]` can statically analyze identifiers, override this method and disable these runtime checks.
  ///
  /// Override with caution. You may encounter SQL errors at runtime.
  fn may_need_escaping(&self) -> bool {
      true
  }

• Where possible, derive specialized implementations that returns false.
• Restore the fast path in the backend for that case.

This is entirely backwards-compatible and can be done later in 2.x.x

[tyt2y3]

can you keep the example in README using impl Iden?

[tyt2y3]

The reason IdenStatic has extra Copy and 'static requirements is because of SeaORM. but right now SeaORM has it's own IdenStatic trait and not actually using this. if possible (without orphan rule constraint), we should reuse this trait in SeaORM

[tyt2y3]

I am fine with this breaking change as this only affects users who implemented Iden manually

However, I'd like to clarify do we still recommend impl Iden for users with existing impl Iden? IdenStatic is not a full replacement of Iden, if users need to choose between Iden / IdenStatic, can you add some docs regarding this in readme?

Before or after merging this, can you prepare changes needed in SeaORM as well?

[tyt2y3]

this is equivalent to the following right?

impl Iden for Character {
    fn unquoted(&self) -> Cow<'static, str> {
        match self {
            Self::Table => "character",
            Self::Id => "id",
            Self::Character => "character",
            Self::FontSize => "font_size",
            Self::SizeW => "size_w",
            Self::SizeH => "size_h",
            Self::FontId => "font_id",
            Self::Ascii => "ascii",
            Self::CreatedAt => "created_at",
            Self::UserData => "user_data",
        }.into()
    }
}

if so we should at least keep one

[Expurple]

Apart from not implementing IdenStatic, this is equivalent, yes. I used IdenStatic where possible because it's less verbose and conveys more information in the type system.

If you want to keep a direct impl Iden somewhere in the docs/examples, I'd choose an example that makes sense (a runtime-allocated, non-static identifier). We have a few such synthetic examples in trybuild tests

[Expurple]

I am fine with this breaking change as this only affects users who implemented Iden manually

Not exactly. I also removed Iden::quoted and changed the return type of Iden::unquoted. This affects their callers.

IdenStatic is not a full replacement of Iden

Why? Any IdenStatic gets a blanket impl Iden. My idea is that we should implement IdenStatic instead of directly implementing Iden, where possible.

if users need to choose between Iden / IdenStatic, can you add some docs regarding this in readme?

Ok, I'll add this a bit later.

Before or after merging this, can you prepare changes needed in SeaORM as well?

Yeah, makes sense to check such breaking changes before merging.

[tyt2y3]

My idea is that we should implement IdenStatic instead of directly implementing Iden, where possible.

that's what I want to clarify. keep doing impl Iden for Character { fn unquoted(&self) -> Cow<'static, str> { seems to be a lesser breaking change, and don't have to look around when the str is non-static (although rare)?

the confusion is that sea-orm has it's own IdenStatic trait which has the same name and interface, but with extra Send bounds. if you'd like to recommend IdenStatic then we should unify the two, presumably reusing the one in sea-query.

[Expurple]

Ok, I'll try to unify those when I prepare SeaORM