Flip default for return_ref

[MichaReiser]

This has come up in #718.

It's very easy to forget a return_ref on a non-copy type. On the other hand, returning a ref for a copy type may be undesired, but the performance consequences are often negligible.

Should we change the default for return_ref to be true and instead introduce a return_cloned.

Related: Should we introduce a return_deref? There have been a few occasions where I had an Option<T> and I ended up writing a manual accessor to get the deref behavior. That might also suggest that the syntax should be return(cloned), return(deref) return(ref)?

This is a rather subtle breaking change. We should change/decide on this before the first "stable" release.

[Veykril]

For the Copy case I'd also expect the compiler to change the ref-passing to copy passing in some occasions on its own so I think the "risk" there of that being less efficient is negligible. The bigger annoyance is that you in fact now have a reference to your type which means you need to deref it manually, but that can be worked around by well putting the marker attribute on it.

Generalizing it via return(<action>) seems to generally be a good move for future extensions imo. Either way 👍 from me for flipping the default, it sounds like what one would expect in terms of default behavior.

In an ideal world with specialization we could work around this via a trait with an assoc type that marks the return type and specializing on Copy to reduce the annotation burden entirely (I think).

[Veykril]

#582 related

[davidbarsky]

I'm glad you opened this issue: I strongly think Salsa should default to #[return_ref] and I'm onboard with generalizing via return(<action>).

[CheaterCodes]

Since this keeps bothering me, I figured I'd take a look at this. It appears that this isn't too much work but there are a few design details.

I think going for #[return(clone)], #[return(ref)], #[return(deref)] makes sense, with ref being the default. However, it appears that returnisn't valid in this position; Rust requires an identifier as an attribute name (makes sense I guess).
So first question, what else? My first intuition would be to scope the attribute like #[salsa(ref)], though that would probably imply that other attributes would do the same, like #[salsa(no_eq, ref)] or #[salsa(return(ref), get(name))].

The other question is whether tracked functions should do the same? I'm guessing yes, is ref also the sensible default here? It feels a bit strange to rewrite the function signature like that, but I think ref is always the lesser foot-gun.

[davidbarsky]

If we can't use #[return] without resorting to raw syntax, I'd vote for #[ret] as an alternative.

I don't think it makes sense to apply the same logic to tracked functions: with structs, it's a more akin to field access, but as you've correctly identified with functions, a signature is a bit more sacrosanct.

[carljm]

Or what about returns(ref), returns(clone), returns(deref)?

[CheaterCodes]

Bot ret and returns are reasonable, though I'd prefer returns I think.

Regarding functions, it still feels bad that calling a

#[salsa::tracked]
fn compute_large_vec(db: &dyn Db) -> Vec<SomeStruct> { ... }

implicitly clones. If implicitly rewriting isn't an acceptable default option, maybe having a default of returns(copy) would be reasonable? That way it works fine for tracked structs by default, but doesn't do surprising clones (requires an explicit returns(clone)).

[MichaReiser]

It's not really clear to me what the difference of fields and queries is that requires to treat them differently.

I think we should have the same default everywhere. It's otherwise very unpredictable and the footgun is the same for fields as it is for queries.

[CheaterCodes]

The footgun is the same, so I think neither should have a clone default.
The difference to me is that fields are accessed through a getter, so it's not surprising if it turns into a reference.
However, tracked functions (look like they) are called verbatim, so it's a bit confusing if the type changes.

I think it's not a huge deal either way, as long as the default is either ref or copy, so something without much expected cost.

I guess having copy on both would be an option, though I have a lot of return_ref spammed in my tracked structs right now, would be nice to have that go away.

[CheaterCodes]

I guess I'll just try to implement the returns(clone) functionality now, deciding on the default at a later point should be no issue.

[CheaterCodes]

Since #772 now got merged, I think this can be revisited.

Currently, the default for tracked fields and functions is returns(clone). It is very easy to forget to explicitly specify the return mode, so we end up often cloning where we didn't mean to. For example, forgetting to specify a returns(deref) on the contents of a source file, could end up cloning the entire content for every call to the tracked function source().

The solution to this is to swap the default to something that is always cheap. We have basically 2 options:

• returns(copy) means that there is no surprising behavior, but we'd end up needing explicit annotations in most cases, since most types aren't Copy. Non-copy types will clearly show a compilation error, so it's impossible to miss problems related to this.
• returns(ref) is more ergonomic, since it works with all types. It may be slightly confusing that the return type of tracked functions is implicitly rewritten to return a reference, Copy types will often require an explicit de-reference, which indicates to the user that using returns(Copy) may be appropriate.

I'm ok with either one, without a strong preference.

Once a default is decided on, implementation should be very easy, thought rewriting all the tests may be a chore.

[MichaReiser]

I don't have a strong opinion. I'm fine with either copy or ref but I do have a weak preference towards ref because it's less annoying (and the compiler probably still does the right thing in most cases).

[Veykril]

The default should definitely work for all cases by default, so I'd prefer ref over clone over copy for defaults.

[regexident]

So first question, what else? My first intuition would be to scope the attribute like #[salsa(ref)], though that would probably imply that other attributes would do the same, like #[salsa(no_eq, ref)] or #[salsa(return(ref), get(name))].

From a user's perspective I would greatly appreciate all salsa helper-attributes being namespaced under salsa(…). 👍