Bindnode fixes continued #233
Conversation
…ld representation level view of subsequent keys and values. Also: add a defaulting pass to struct type construction, which makes the reprStrategy function become rather more correct (if in a round-about way).
…callbacks! Amusingly, you only notice the difference when there's a union (and either a kinded or a stringjoin one at that (so far anyway)) that's a value in a map. This is because in all other cases, memory sideeffects are sufficient. But when you're working on a map value -- the finisher callbacks are actually important, and will cause copying action. Which means continued writing into the memory you were using up until that point becomes unimpactful. (Took me about 2 hours to find these. Computers.)
Relatively nice-to-have vs some of the other things needing fixing right now, but don't want to forget about it either, especially because it's one of those syntactically nonobvious potential panic zones.
…eld representation level view of subsequent values. This is comparable to the fix a couple commits above, for maps. Also a todo comment regarding struct tuple representation. I'm pretty sure there's some missing branches there (but not 100%, and I'm not currently recursing into verifying that).
|
I've got one more fairly nasty discovery: our approach to unions is problematic again: because it is now required to use pointer fields, if one wants to use a union as a map key, one is going to have a bad day. (As long as you hold on to literally the the values originally used as keys, you can use them to look things up again; but you'll have a hard time producing any other equivalent value, and that's probably going to result in massive frustration and nasty bugs, etc.) We'll have to reintroduce the ability to use an integer field as the in-memory discriminator, and allow non-pointer fields for the members. I think we can make this optional and just engage it if we find such a field. (And then still probably document some warnings about this, because it'll be in the court of the person authoring the go types to not put themselves in a bad situation.) I'll probably look into doing this tomorrow. |
…d certainly not return its typekind's behavior; that's always map. This was also causing all sorts of further nonsense, like encoding attempting to always recurse into the map member (assuming one was present; otherwise I suppose it probably would've panicked somehow, although I didn't check this). Oh, also, we were missing a key case from a switch in the schema package. *That's* embarassing. My kingdom, yet again, for sum types in golang. They would have, with 100% confidence, have prevented that latter bug from occuring at compile time, done so ages ago, and probably reduced the amount of pain this was to wade through by orders of magnitude.
…back up to type level. With this fix, together with the fixes in the prior commit, encoding a stringprefix union now works correctly. (And, I guess, a stringprefix union that's directly inside a kinded union, and that contains a struct with stringjoin representation, all works. Which, though it sounds like a mouthful, is in fact what I'm doing in the downstream application that's causing me to fix all this stuff.)
|
Why is allowing non-pointer fields for map keys a blocker? can we not implement an |
| @@ -53,8 +53,8 @@ func (w *_nodeRepr) Kind() datamodel.Kind { | |||
| return datamodel.Kind_Map | |||
| case schema.UnionRepresentation_Kinded: | |||
| haveIdx, _ := unionMember(w.val) | |||
| mtyp := w.schemaType.(*schema.TypeUnion).Members()[haveIdx] | |||
| return mtyp.TypeKind().ActsLike() | |||
| mtyp := w.schemaType.(*schema.TypeUnion).Members()[haveIdx] // TODO this is fragile: panics with index-out-of-range if the user has nil'd all fields. | |||
There was a problem hiding this comment.
put a , ok and an error for out-of-range rather than panic?
There was a problem hiding this comment.
Nontrivial. This method can't return errors. It's a core interface method, and a break-the-universe-badly one if we changed its return type.
| default: | ||
| panic(fmt.Sprintf("TODO: %T", stg)) | ||
| } | ||
| } | ||
|
|
||
| type _mapIteratorRepr _mapIterator |
There was a problem hiding this comment.
comment to explain why this wrapping and not just calling w's MapIterator is needed would be helpful
|
I was too optimistic in saying I'd work on the union thing immediately. I'm going to extract that to an issue instead. Now in: #234 It does turn out to not be a huge blocker for me today. If you use the keys given to you, carefully, everything works. It's just a very sizable footgun, because you can't "recreate" a key value that's "equal" in the way you'd expect.
No. Map keys. It's a native thing. |
|
More tests for this would be desirable, but I'm going to hope we can backfill those in the future. (And also, it should eventually be easier to do that if we get schema-schema wired up to be data-driven; then we can make purely data-driven language-agnostic tests, and gosh that would be nice.) Meanwhile, as a stopgap, I promise I've at least tested all these myself in downstreams. I'm also somewhat attached to these commits already, for the same reason (I've started depending on them by commit hash in a downstream already), so I'm going to boldly merge them, and we can continue improving in future PRs. |
|
All of these fixes make sense to me. I think we should start thinking about more table-driven tests before we continue doing more fixes, otherwise we're just building on quicksand. |
Like it says on the tin.
Details in commit messages.