Strict keys

[SteveSandersonMS]

Implements #11908

As suggested by @Andrzej-W, this changes the default behavior for @key so that during diffing, if we encounter any duplicate values then we throw and fail. This makes it more likely for developers to discover any bugs they might have with unexpected duplicates, and avoids the risk that element/component preservation might behave differently than the developer expects in those cases.

This PR also adds the option to have any given key treated as "loose", meaning that it's allowed to be null or a duplicate of another key value. Both of these scenarios are handled by simply ignoring that key, including any other instances where it is duplicated. That's more or less the same as the previous behavior, except that previously we didn't allow null at all.

Later, I expect the syntax for loose mode to be @key:loose (or @key:loose="true"), but since that's not yet implemented in the compiler, loose mode currently can only be used from manual rendertreebuilder logic, hence implementing part of the E2E test that way.

Update: as per discussion below, I've removed the loose mode, because (1) it's hard to say what the circumstances would be where you would actually want to use it, and (2) if you really do, then it can be achieved in user code instead. For example, if you have a list where some of them items are already saved to the DB and hence have a PK value, whereas others are still being created and hence don't have a PK value, then you probably still do want predictable preservation behavior for the uncommitted items, so you would achieve that by giving them a temporary GUID or something until they are committed. In case someone doesn't want to supply any key at all for a certain item, it's now allowed just to pass null, which treats that item as unkeyed.

[SteveSandersonMS]

Actually, having done this, I’m now reconsidering the idea of there being a loose mode option. It may be better just to only have the strict mode.

The reason is that you could make something functionally equivalent to loose mode in user code anyway (e.g. a method that returns the key if it’s unique within a set, or a guid/null if it’s not). It’s an edge case and probably doesn’t warrant being a built-in flag.

[javiercn]

The reason is that you could make something functionally equivalent to loose mode in user code anyway (e.g. a method that returns the key if it’s unique within a set, or a guid/null if it’s not). It’s an edge case and probably doesn’t warrant being a built-in flag.

I think I agree with your reasoning. If the key is null though, wouldn't that throw?

[SteveSandersonMS]

If the key is null though, wouldn't that throw?

Yes, I didn't mention, but part of the intention for the "no loose mode" plan was to tolerate nulls, and treat them as meaning "don't have a key on this one".

[SteveSandersonMS]

Strictly speaking this change isn't required in this PR, but I'd like to include it because it's a good thing and means we have less to reason about in the future if we want to make use of the memory space this frees up.

[SteveSandersonMS]

OK, I've updated this PR now so that the only mode is strict mode.

[javiercn]

Do we still avoid throwing on null/default keys?

[javiercn]

Do we still avoid throwing on null/default keys?

Couldn't this hide implementation errors?

[javiercn]

Changes look good. I'm wondering if we should check for duplicate nulls/defaults as that might indicate a developer error is lurking below. If you have a key, you should have a good function for it that produces unique values.

After all, you are likely not using the key with object that have a value identity but with objects that have some sense of reference identity.

Do we use GetHashCode() by default, should we be concerned about collisions there?
Should this just be guidance to use key when you have a field that has some sort of referential identity?

[SteveSandersonMS]

If you have a key, you should have a good function for it that produces unique values.

You usually would do, but there are cases where you don't. For example you could have a collection that contains some items that are already saved to the DB and hence have IDs, but other items that are still being created and hence don't have IDs.

In cases like this developers may choose to assign temporary IDs to the unsaved items, or they may not. If they don't, it's pretty awkward if we force them to write if/else blocks just to vary whether they are specifying a @key directive attribute. It's much friendlier just to allow null.

Couldn't this hide implementation errors?

Maybe, but it's a tradeoff.

Do we use GetHashCode() by default, should we be concerned about collisions there?

No, we don't use GetHashCode. We're just using Dictionary<T,V>'s key-equality comparer.

Should this just be guidance to use key when you have a field that has some sort of referential identity?

If by "referential" you mean object reference identity, then I don't think so - it would be legit to use it with successive DB queries that return different actual object instances but retain ID values.

The guidance is to use it when you have something that's meaningful to use as a key, which could be object identity, or it could be something like a PK value from a DB query result.