Changed return_ref syntax to returns(as_ref) and returns(cloned)

[CheaterCodes]

As discussed in #719 , I figured I'd give this a go.
It was about as straightforward as I expected, with about as much problems as I anticipated.

Basically, I replaced the return_ref syntax with returns(as_ref). This allows other return modes to be specified as well, like the current default of returns(cloned).

I used returns instead of return, because using #[return(ref)] already fails pre-macro expansion, since return is a keyword. I used as_ref instead of ref, because it required less rewrite of the macros. It's possible to use ref, but I didn't bother with it for now. I used cloned instead of clone, since it felt more analogous to as_ref (compare with Option::as_ref and Option::cloned).

Since this was my first time browsing this code base, I wasn't entirely sure how hacky is too hacky. I think it's not too bad, other than the reuse of maybe_clone (which I renamed to return_mode), since I didn't want to duplicate the code.

I don't necessarily expect this to be merged, but I wanted to share the results of my experiment.

I also didn't change the defaults away from clone yet, since updating all the tests seems like an annoying chore that I'd rather only do once it's decided on.

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	e162825
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/681dab51794d600008ab5181

[codspeed-hq]

CodSpeed Performance Report

Merging #772 will not alter performance

_{Comparing CheaterCodes:return_ref (e162825) with master (af69cc1)}

Summary

✅ 12 untouched benchmarks

[MichaReiser]

The change itself looks good to me. However, I realised that it isn't entirely clear to me how we'd implement as_deref because it would require knowing the return type (Option<Name> where Name has a Deref implementation to str returns a &str and this isn't something that salsa can easily infer).

That makes me think that it might be good to add as_deref support as part of this PR, to ensure that the new syntax is flexible enough (I don't want to make this migration twice 😅)

A possible syntax could be returns(as_deref = &str)

[CheaterCodes]

I would have assumed it would work the same way Option::as_deref does; It would only deref once to &<T as Deref>::Target.

Honestly, I didn't even consider Option, but I realize now what you wanted to do. Currently as_ref would also return a &Option<String> and as_deref would fail, since Option doesn't implementDeref. So maybe returns(ref/deref) and returns(as_ref/as_deref) would be different?

Another option I considered is to instead implement something like returns(&str via |x| x.as_deref()), but that feels maybe a bit much?

[MichaReiser]

I would have assumed it would work the same way Option::as_deref does; It would only deref once to &::Target.

Yes, it should work the same as Option<Deref>.

Oh, I see, yes my example was incorrect. It should return Option<&str> given Name implements Deref<Target=str>. But also, yes, we may need more variants because we want to support returning a slice instead of a &Vec (deref) and also returning a Option<&T> instead of &Option<T> (as_deref)

Hmm, we might need to think a bit more about the desired API. It would be nice if we didn't have to special case Option::as_deref and could instead have a generic API for going from &T to the desired return type and cloned, deref, as_deref would just be well known cases. Either way. Supporting copied, cloned, as_ref, ref, deref and as_deref seem a good starting point. I'm still wondering if there's a way to not require an explicit return type for as_ref and as_deref (a custom salsa trait?)

[CheaterCodes]

Just to clarify on ref: it's slightly annoying because it isn't a valid identifier and half the macros assume that. but I think it's doable, I'll give it a shot.

From my quick browsing of the std-lib, Option and Result appear to be the only types with an as_ref and as_deref method that isn't covered by deref already.

So we could special case it, or, like you suggested, use a custom trait. That may also be able to cover other custom mappings that a user might want to provide.

pub trait AsDeref {
    type Target<'a>;

    fn as_deref(&self) -> T::Target<'a>;
}

should in principle cover all allowed transformations (not just as_deref).

[CheaterCodes]

This would be a lot easier with spezialitation 😅

[Veykril]

I think you both already agreed on it but I'll just put down my coarse view on it (partially rehashing what was already said)
We have the basic ones which are trivial to implement

• copy: just leave the return type verbatim and dereferencing the returned borrowed
• clone: likewise but with .clone()
• ref: put a &$db_lt in front of the type
  The following require traits to transform the return type
• deref: Change return type to &$db_lt <Ty as Deref>::Target, put call ::core::ops::Deref::deref(ret_val) on return
• as_ref: Here a question comes up, do we mean the AsRef trait? Or do we specifically mean functor like things like Option::as_ref, Result::as_ref? Probably the latter, the former seems not as useful.
  • in the case of the latter, I'd expect us to offer a custom trait

  trait SalsaAsRef {
  	type Out<'a>;
  	fn as_ref<'a>(&'a self) -> Out<'a>;
  }

  and do the transformation as with deref for the return type. The trait just delegates to Option/Result::as_ref as the provided impls of it,
• as_deref: Same as above really

Naming bikeshed: Should it be copy / clone or copied / cloned?

Hmm, we might need to think a bit more about the desired API. It would be nice if we didn't have to special case Option::as_deref and could instead have a generic API for going from &T to the desired return type

This sounds generally nice to have and I think can also just be modeled with a simple trait that we can provide a derive for which would basically look the same

    trait SalsaReturnTransform<'db> {
    	type Transformed;
    	fn as_ref(&'db self) -> Transformed;
    }

(I think a GAT would be less flexible here than tying the database lifetime to the trait impl, not 100% sure though)
then we could just have an opt-in for that like #[returns(transform)]/#[returns(transformed)] (with the right spans the IDE should even be able to jump to the trait impl on the transform ident there 👀)

[CheaterCodes]

Seems about right, yeah.

copy: just leave the return type verbatim and dereferencing the returned borrowed

I would rather wrap the value using something like copy(x) where fn copy<T: Copy>(x: &T) -> T, I think that gives better errors. Same with using Clone::clone(x), and like you did with Deref::deref as well.

as_ref: Here a question comes up, do we mean the AsRef trait? Or do we specifically mean functor like things like Option::as_ref, Result::as_ref? Probably the latter, the former seems not as useful.

I agree, it should be the latter, via a custom trait. AsRef doesn't make much sense because it is generic (we'd need to specify T), and it would leave us with missing Option::as_ref again, which I think is important. Also, most types where we'd want as_ref already have a Deref impl anyway. I suppose you could do something like as_ref([u8]), but that may be a bit confusing if as_ref alone means SalsaAsRef::as_ref.

Naming bikeshed: Should it be copy / clone or copied / cloned?
Probably copy/clone. I think the other option would be confusing with Option::cloned/Option::copied, which I guess would be a completely separate thing again.

Regarding transform, I think a GAT is the correct model here, but I'm also not sure. Would be great if someone researched this!
I also considered if something like transform([u8], |x| x.as_ref()) could make sense, but at that point it's probably better if the user just writes a getter for this.

Also there still needs to be a decision on the default: I believe that ref is the most sensible default, since it does the least magic under the hood, at least for struct fields. For tracked functions, I could be convinced to make the default copy instead to avoid implicitly rewriting the signature as a default. I could maybe be convinced that copy should be the default for both, but I'd really like ref for struct fields.

[CheaterCodes]

I have now implemented copy, clone, ref, deref, as_ref, as_deref.
There are two new traits salsa::SalsaAsRef and salsa::SalsaAsDeref for dealing with the last two cases. These can be implemented by crate users, so in principle that already deals with the custom transform use case.

Once we've decided on the default (I'm still undecided between copy and ref), I will go through and update the tests and examples, using copy, clone, ref where appropriate, probably deref for Vecs and Strings. It seems like as_ref and as_deref would require separate tests; There aren't really any Option<BigStruct> or Option<String> going around it seems.

[MichaReiser]

Thanks. This looks good to me. I'd prefer if someone else good quickly glance over it to confirm that they agree with the overall approach.

Could you add a test that demonstrates thtat the following doesn't compile: returns(not_a_return_mode).

I'd be okay if we switch the default in a separate PR

[CheaterCodes]

I was busy for a while, but now I went and added both pass and fail tests for the return mode, I hope they are fine the way they are, I'm not very used to writing tests.

I also did some minimal docs for the two new public API traits. I'm not sure if anything more need to be said?

Since @MichaReiser asked for a compile_fail test, I also went and added a nice error message. I think just aborting the proc-macro isn't ideal, but that's already happening in other cases, so I figured at least the error message is clearer this way.

I'm ok with updating the default in a separate PR if that is preferred.

[MichaReiser]

Thank you, this is great