construct NamedFields from owned Strings

[hawkw]

Issue #62 describes a need to construct Fields::Named from an owned
set of strings, rather than borrowed ones. This branch adds a
NamedField::from_string constructor that returns a 'static
NamedField constructed from an owned String value.

The proposed solution in #62 is to use Cows (presumably constructing
NamedField from T: Into<Cow<'a, str>>). However, we can't easily do
this, as Cow is not available without alloc, so we'd need two
totally separate internal representations based on whether alloc is
enabled. Additionally, we can't have a single new constructor taking
T: Into<Cow<'a, str>>, for a couple reasons: first, the new
constructor is a const fn, and const fns can't currently have trait
obunds on generic arguments; second, we can't have the new constructor
expose Cow, since the constructor would have to have a different type
signature when the alloc feature is not enabled.

Instead, I've added an internal enum that's roughly Cow-like, and a
NamedField::from_string constructor taking a String by value. Rather
than directly mimicking Cow, though, the owned string case is wrapped
in an Arc. This is because NamedField currently implements Clone
and Copy; it's intended to be cheaply clonable. It seems quite bad
to now have a Clone that's either a basically free pointer copy or,
sometimes, allocating a string and copying all the bytes in it. Instead,
when a NamedField is constructed from a string, we reference count it
so that it can be cloned inexpensively.

I did still remove the Copy impl for NamedField, since Arcs are
not Copy. However, if we think it's acceptable for an owned
NamedField to perform an arc refcount bump and still implement Copy,
I could put it back.

Hopefully, this solves some of the problem from #62. I think we might
also need to do something similar for Fields::Named, so that the array
can either be a borrowed slice or an owned
Vec<NamedField>/Box<[NamedField]>. We can do this with a similar
approach as the one used here, but I wanted to put this PR up first
before starting on that, to make sure it's basically the right
direction.

[hawkw]

hmm...this CI failure seems concerning: https://github.com/tokio-rs/valuable/runs/4291553819?check_suite_focus=true#step:4:566

This approach may end up not being viable. But, I'm also uncertain about the error in that case; AFAICT the value should not be dropped inside the static? I think we may end up having to put the named fields in another static to fix this, and reference it in the variant static...which kinda sucks. I'm not sure if there's a great way to support owned string values here without Arcing them, though, given the constraint that NamedFields should be inexpensive to clone...

@carllerche & @taiki-e, any thoughts?

[hawkw]

hmm...this CI failure seems concerning: https://github.com/tokio-rs/valuable/runs/4291553819?check_suite_focus=true#step:4:566

This approach may end up not being viable. But, I'm also uncertain about the error in that case; AFAICT the value should not be dropped inside the static? I think we may end up having to put the named fields in another static to fix this, and reference it in the variant static...which kinda sucks. I'm not sure if there's a great way to support owned string values here without Arcing them, though, given the constraint that NamedFields should be inexpensive to clone...

@carllerche & @taiki-e, any thoughts?

Continuing to look into this, it looks like the issue occurs with Box<str> or String, as well, since those types also have Drop impls; it's not specifically a limitation with Arcs. So that's either good or bad. :)

It seems like the cause of the issue is that the compiler cannot tell that, when the &[NamedFields] is an argument to a constructor (rather than a field value as part of a literal initializer) that it is owned by the constructed type --- it may be dropped at the end of the constructor call. And if the values in the array have a Drop impl (which Arc<str>, Box<str> and String all do, of course), it can't be used in a static. So, this appears to be an issue that will exist with any approach that allows using any type of owned string as a NamedField, as long as the &[NamedField] is passed to a constructor rather than used as a value for a literal struct/enum initializer.

However, the problem can be trivially solved by putting the &[NamedField] array in a separate static, and passing that to the constructor, instead. Commit 6c9141b changes the existing doctests to do that, and they compile again. So I guess the only real question here is...are we okay with that? It will perhaps make the API for constructing StructDef/VariantDef in statics a little bit more annoying, but if we're okay with requiring a second static to be declared, we can have owned NamedField values.

The other solution to #62 would be to have the owned variant not use NamedField, and instead add some kind of enum that's either a &'a [NamedField<'a>] or a Box<[OwnedNamedField]>. However, this has the significant downside that we can't borrow that as a &[NamedField<'a>], since the array contains values of one of two types...we could provide an iterator over NamedFields by borrowing them from the hypothetical OwnedNamedField type, but we can't borrow a slice.

Therefore, IMO, requiring &[NamedField]s to go in a separate static when manually constructing StructDef/VariantDef statics seems like the better option...or, we just give up on this whole thing, and say that only statically-defined Structables can have O(1) field access. We should probably choose one of those options.

I'd love to hear what @taiki-e and @carllerche think is the right way to move forward here.

[carllerche]

This is a solid PR, but the wrong direction IMO. The definition() method is intended to be as fast as possible and only include statically known fields.

To better handle the dynamic case, I think we can, instead, provide additional field accessor methods that take &str (or other type). So, Structable could get something like.

fn visit_field(&self, name: &str, visit: impl Visit);

I don't know if we can have the fn return a Value<'_> directly as that isn't always possible. We could possibly have a get fn that returns Option<Value<'_>> or a custom enum that lets the caller know that visit has to be called..

Mappable could get similar methods.

[carllerche]

Going to close as I don't think we will move forward w/ this direction. Thanks for exploring 👍