Let's discuss LinkContext

[Stebalien]

1. The internal context can be null (by design). This will cause crashes.
2. It contains dynamically scoped information about the link that may affect resolution (can it?). This will make it very difficult to reason about correctness/behavior.
3. It's big (11 words, 88 bytes) and passed by value. That's a lot of copying.

As far as I can tell, we only use LinkNode. And we only use that to set the LinkTragetNodePrototypeChooser which could probably be explicitly set.

So, I'd very, VERY much like to get rid of this or at the very least pair it down and document it extensively.

[warpfork]

On 1: the documentation states if context isn't present, functions that consume it should treat this as context.Background. If we have a function that's failing to do that (and thus crashing), it's a bug.

On 2: yes, that's accurate. On the other hand: hasn't been a problem yet. And having context.Context at all introduces that problem, since technically anything can be attached to it. (We should probably actually expect this at some point. Say someone wants to be doing loading of data that requires authorization of some kind -- I'd probably suggest they pass around the auth material as a context attachment.)

On 3/performance: potentially, but I kind of want to see a benchmark showing us this is a problem. My current bet is that even as a fairly large pass-by-value, it's not going to end up in the top of the list of first optimization targets. (I'm more betting we're going to see Path concatenations during traversal operations are terrible alloc monsters that dwarf this, for instance.)

On the other hand for performance: if we do turn out to want any of these values it's passing down, having this struct in here means we have a place do it while avoiding the need to do a construction of a new context.Context to attach values to -- and that means this pass-by-value trades away not one but several heap allocs. (3 or 4, at least, I think by the time the map clone is done? Haven't tested.) This is my primary reason for thinking we might want some struct here, whatever its exact contents may be.

ParentNode could probably go. There's a comment about that already. I'd be easily convinced; there just hasn't been enough of an impetus to make me actively pursue throwing it out yet.

I'm sort of surprised LinkPath doesn't get much use. I figured it would be useful for debug printfs, if nothing else.

All in all, I do agree this could do with a review pass. If we do change anything I'd probably line that up to be a "v0.11" thing (i.e. slightly down the road, chronologically; if we do any breaking API changes here, I'd like it to be after a cooldown period since we just did the LinkSystem ones).

[Stebalien]

On 1: the documentation states if context isn't present, functions that consume it should treat this as context.Background. If we have a function that's failing to do that (and thus crashing), it's a bug.

That doesn't really change anything. Any API that requires a "if nil, make it not nil" dance is bad.

One simple solution is to make the context private, add a WithContext function, and add Context accessor function. But I'd rather just keep the context separate.

On 2: yes, that's accurate. On the other hand: hasn't been a problem yet. And having context.Context at all introduces that problem, since technically anything can be attached to it. (We should probably actually expect this at some point. Say someone wants to be doing loading of data that requires authorization of some kind -- I'd probably suggest they pass around the auth material as a context attachment.)

Sure, but can we just get rid of it? Do we really need it?

I'm sort of surprised LinkPath doesn't get much use. I figured it would be useful for debug printfs, if nothing else.

This kind of stuff should just go into a normal context as it's optional and we don't really need it. There's no reason for a custom context.

---

So, my question is why? Do we really need this? If not, please drop it. If so, please explain why. Having a grab-bag "because maybe we'll need it" argument is really nasty.