I'm running into an awkward problem because Val does not impl PartialEq.
#Val PartialEq
tired trout
Funny enough if I simply wrap the Val in a Vec, I can assert on its equality. Because while Val's can't be compared, Vecs of any type can be compared even if their contained types cannot.
I remember this was originally why we added EnvVal a year or more ago.
But we removed it after we had started to use the type in ways that was not helpful.
graceful saddle
hmm, I guess we could just create an SDK wrapper for Val, similar to what we do for a lot of other types
it could also benefit from other useful operations, like from/into operations to convert from/to other SDK types
tired trout
I think that's doable. The wrapper could contain a MaybeEnv. Actually, couldn't the env Val contain a MaybeEnv? 🤔
Just like how Symbol contains a MaybeEnv.
graceful saddle
yes, that's what I mean by 'SDK wrapper'
tired trout
Oh I see.
I thought it was the Env Symbol that contained the MaybeEnv, but it is the SDKs.
This could be a lot of work... but also really benefit the API.
graceful saddle
I'm not 100% sure if Val would benefit from optional env though; for symbol we can omit Env when we're creating short symbols. I guess for Val we can avoid it when converting 'simple' types to the respective Val
yeah, I would imagine Val is not used too frequently, but providing proper SDK support would still be nice
tired trout
After a quick look, I think the maintenance burden is going to be pretty high to implement this in the SDK.
If we implement it in the SDK, we have to reimplement the TryFromVal, etc traits for the wrapper Val type for all other types. A blanket impl is the lowest maintenance burden way to do that, but we can't impl a blanket impl for that in the SDK because of generic type rules / constraints in Rust.
I think we could probably do it in the env-common crate without that constraint though. To avoid the constraint we need to implement it in the crate where the trait is defined.
Would it be reasonable to add something like MaybeEnv to the env Val 🤔. I guess there's a perf hit of all the additional reference count tracking.
graceful saddle
I'm not sure it's possible/feasible, given that we pass Val through ABI (like all the object types)
ionic obsidian
yeah that's not really feasible. what's the need for PartialEq?
fwiw we could define PartialEq correctly for Val -- I think a partial equivalence is definable in absence of an env -- but not Eq, and .. I'm pretty sure every meaningful context actually wants Eq
graceful saddle
it honestly would just make general sense to have Val as SDK type with all the typical features that SDK types have. I even think that MaybeEnv optimization is not necessary.
ionic obsidian
like a partial equivalence can observe u32(5) is equal to u32(5) and even that obj#7 is equal to obj#7 it just doesn't know when it's equal to obj#9 which it might or might not be structurally equal to
tired trout
yeah that's not really feasible. what's the need for PartialEq?
The need is so that we can compare Vals. Context is:
I noticed we had some testutils using soroban_sdk::Vec<(..., Val)>, and it would be better if they were std::vec::Vec<(..., Val)>.
When I tried to change it, I discovered there were places assert_eq! the sdk Vec returned, and that works. But when I changed it to std::vec::Vec it failed, because it needs the Val to be comparable.
Obviously I can just not change the Vec to a std Vec and we can forget about this.
But feels like something that could be inconvenient again in the future.
ionic obsidian
shrug I don't have a strong opinion on whether you add a wrapper type at the SDK level. at the env-interface level we don't even have the time to investigate the possibility of making such a change, and I've fairly high convidence it wouldn't work anyways, or would be extremely disruptive (it'd be like reviving EnvVal and re-plumbing it everywhere, and Val is currently a Copy type..)
tired trout
I'm very skeptical there isn't some trait constraints that'll block us making that change.
I think at some point we discussed interning, but I'm guesing we decided against it (for good reasons).
ionic obsidian
yeah I don't remember the exact reasoning around interning but I recall something going fairly wrong when we explored it.
and PartialEq itself .. idk, I mean, I don't want to be The Guy Who Always Says No I just think it's likely hazardous to expose because people will think it means something it doesn't
we do expose .shallow_eq and even .get_payload() and stuff, it's just a minor speed bump so people stop and think
graceful saddle
well, I think in the context mentioned (testing) we need proper equality