uast: properly load node arrays with registered types

[dennwc]

Some UAST schema types may contain fields with arrays of nodes. This change fixes the problem with loading such structs from generic UAST nodes to concrete Go types (currently it just fails).

Signed-off-by: Denys Smirnov denys@sourced.tech

[juanjux]

Must be the post vacational syndrome but it's not clear to me what this does from the description. Could you provide a small pseudo-AST example?

[dennwc]

I believe it has no use right now, it's a backport from #337.

But, if we will have any UAST schema similar to this:

type MyNode struct{
  List []Any
}

the SDK will fail to convert generic UAST nodes like

nodes.Object{
  "@type": "uast:MyNode",
  "List": Arr{...}
}

to a Go struct of type MyNode.

This is a bug in the current reflection code in SDK.

[juanjux]

LGTM, thanks for the explanation!

[juanjux]

Nice to have: some test to check we don't have regressions (and shut up codecov).

[creachadair]

Since you know the exact size anyway, and there's no early exit, you might as well just allocate and assign rather than appending: make([]Any, sz) … arr[i] = v

[dennwc]

I usually prefer not to. Because an exact size is only an optimization, and shouldn't affect the code below.

[creachadair]

Generally if you don't care about optimality, I recommend the simpler code: var arr []Any. The append builtin reallocates intelligently to avoid quadratic moves. If the allocation really matters, it's better not to append at all, and assign directly. The only time this pattern really makes sense is if you're copying out of a map, where there is no index without a separate explicit induction variable.

[dennwc]

It's more like a middle ground. When I don't care about optimality, I use var arr []Any, but when I do care, I simply change that single line to arr := make([]Any, 0, sz) and don't touch the code below (it's valid for both cases).

You may argue that it's might be less optimal than a direct assignment, since append will increment the counter, but it should be negligible comparing to any allocation and the code is a bit more flexible in case some decide to add additional items. I think it's a good balance, but maybe I should use the optimal one, as you suggest.

[creachadair]

I guess my point is that there's no real need to add an explicit allocation at all, if the cost is negligible (which, in this case, it likely is). The explicit allocation is a red flag to the reader that something interesting is happening—when in fact it's not really. :)

[dennwc]

Stolen an original test case from Ambiguity PR (this change is a backport from it). Should be good to go.