Proposal: in `new` methods on intern structs, leverage `Borrow` or `ToOwned` traits or something

[nikomatsakis]

I often wind up with an Identifier interned struct:

#[salsa::interned]
struct Identifier<'db> { text: String }

and then I want to create an instance from an &str. It's silly that I have to allocate an owned string for that. The hashmap methods already support the ability to start with a borrowed value and "promote" to an owned value if needed, we should leverage that.

[nikomatsakis]

Mentoring instructions

This would make for a good first issue. The idea is to modify the intern method:

https://github.com/salsa-rs/salsa/blob/a20b894cc2318c5a6077659e2d8aef7c784c38e7/src/interned.rs#L125-L130

so that instead of an owned C::Data<'db> instance, it takes a impl ToOwned<C::Data>:

    pub fn intern<'db, D>(
        &'db self,
        db: &'db dyn crate::Database,
        data: impl ToOwned<C::Data<'db>>,
    ) -> C::Struct<'db>

This will also require tweaking the setup_interned_rules! macro definition in a corresponding way:

https://github.com/salsa-rs/salsa/blob/a20b894cc2318c5a6077659e2d8aef7c784c38e7/components/salsa-macro-rules/src/setup_interned_struct.rs#L135

As you can see, the Data<'db> field is currently a tuple:

https://github.com/salsa-rs/salsa/blob/a20b894cc2318c5a6077659e2d8aef7c784c38e7/components/salsa-macro-rules/src/setup_interned_struct.rs#L67

This should be changed to a newtype'd struct that we generate for each #[salsa::interned], something like

struct InternedData<'db, F1, F2, ..., Fn>(F1, F2, ..., Fn, PhantomData<'db>);

The value of C::Data<'db> then becomes something like

type Data<$db_lt> = InternedData<$db_lt, $($field_ty,)* PhantomData<$db_lt>>;

Using a newtype'd struct lets us implement ToOwned and Borrow:

impl<'db, F1..Fn> ToOwned for InternedData<'db, F1..Fn, PhantomData<'db>>
where
    F1: ToOwned<Owned = DeclaredType1>,
    ...,
    FN: ToOwned<Owned = DeclaredTypeN>,
{
    type Owned = InternedData<'db, DeclaredType1..DeclaredTypeN, PhantomData<'db>>;
}

// Borrow left us an exercise for the mentee

and then in turn modify new to accept impl ToOwned of each argument:

https://github.com/salsa-rs/salsa/blob/a20b894cc2318c5a6077659e2d8aef7c784c38e7/components/salsa-macro-rules/src/setup_interned_struct.rs#L129-L136

[puuuuh]

Then we have to do a unnecessary clone of any new already-owned value

[nikomatsakis]

That's true, sometimes you would, though not (in my experience) in the common case. Ideal would be a trait that accepts owned or borrowed, I have to look at the available traits to see if we have one like that. =)

[puuuuh]

We can use separate methods for owned and non-owned keys and Cow in intern method, i dont think there is something like IntoOwned trait in stdlib

[nikomatsakis]

Separate methods + cow would work but seems clumsy.

We could however make our own trait that captures the most important cases:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a0ee6a9dee9aaa0ab7a1810602b22dd5

[puuuuh]

Not so general solution, but lgtm

[puuuuh]

Also i can't impl Borrow for Data struct with many fields, so only way to get id from map is to use fxhashmap's raw-api, ig

[nikomatsakis]

Also i can't impl Borrow for Data struct with many fields

Oh, that is annoying.