Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encompass child node's location in parent. #1899

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

evantypanski
Copy link
Contributor

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.

@evantypanski evantypanski force-pushed the topic/etyp/encompass-children-locs branch from b418983 to 14845ec Compare October 17, 2024 20:00
Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hilti/toolchain/src/ast/node.cc Outdated Show resolved Hide resolved
hilti/toolchain/src/ast/meta.cc Outdated Show resolved Hide resolved
@evantypanski
Copy link
Contributor Author

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.

@evantypanski evantypanski force-pushed the topic/etyp/encompass-children-locs branch 2 times, most recently from 5831197 to 8c7ba5a Compare October 18, 2024 14:00
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.
@evantypanski evantypanski force-pushed the topic/etyp/encompass-children-locs branch from 42882f4 to adc5e8d Compare October 18, 2024 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttributeSet report invalid locations
3 participants