Encompass child node's location in parent.

[evantypanski]

Closes #1893

When a child is added with addChild, the parent's location should (generally) span over that child as well. This primarily helps for cases where a node doesn't have much of a location until it gets children added - like AttributeSet. The locations for those should encompass all of the attributes within the set. That logic applies for any node with a child: if it's the child, then its location should reflect that.

There is an edge case that needed special handling: later in node::detail::deepcopy, including the child's location there caused issues with resolved names. A field used in an attribute, for example, would get added as a child to that attribute. Then the location would weirdly go up to encompass the field, then whatever is in the middle, then the attribute. There may be other cases that a child is added that is generated or something, but I didn't catch any and it doesn't seem super pressing if that's the case.

[rsmmr]

Haven't looked very closely but quick comment: That location merging is pretty complex, and I'd be reluctant to run that on every addChild(). Is the issue we are addressing here primarily relevant to just AttributeSet, or do we see it more generally? If the former, I'm wondering if could fix it more narrowly too. For example, AttributeSet::add() could update locations itself; or we could have the resolver look at AttributeSets and recompute their locations if necessary.

One other idea: we could merge locations only when we actually retrieve them. Either the validator does it just before printing an error; or built into location() to always do it on access.

But overall I think the node-specific solution (e.g., do it in AttributeSet::add() would be my preference if we can identify the (small number of) cases where it matters. Could still have a combineLocations() in Node, but would call it only on demand from child classes.

[evantypanski]

Is the issue we are addressing here primarily relevant to just AttributeSet, or do we see it more generally?

So the two cases in the debug logs in the PR are related to attributes, namely:

1. AttributeSet
2. Attribute like &cxxname

no others seemed to pop up, hard to tell if the locs would be bogus but we never use them in diagnostics or something else.

It seems to me like this is generally applicable, but adding this logic on every added child may be a bit much, I agree. That was the intention of this check, but then you still go through the trouble of accessing the cache every time a child is added that's just unneeded work, too.

I did initially put this in Location but got tripped up with the Meta caching stuff. I understand it better now though, so it can probably go back there :) I'll work on that.

[bbannier]

Thanks, this looks good to me though I wonder how much more expensive aggressively merging locations on addChild would have been. @rsmmr, are we good to go here?

[rsmmr]

Yeah, I like this, just a couple further tiny suggestions.