Skip to content

Commit

Permalink
Encompass child node's location in parent.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
evantypanski committed Oct 17, 2024
1 parent 0e3bc48 commit 2da497e
Show file tree
Hide file tree
Showing 12 changed files with 353 additions and 299 deletions.
2 changes: 2 additions & 0 deletions hilti/toolchain/include/ast/location.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class Location {
auto file() const { return _file.generic_string(); }
auto from() const { return _from_line; }
auto to() const { return _to_line; }
auto fromChar() const { return _from_character; }
auto toChar() const { return _to_character; }

/**
* Returns a string representation of the location.
Expand Down
5 changes: 5 additions & 0 deletions hilti/toolchain/include/ast/meta.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ class Meta {
void setLocation(Location l) { _location = std::move(l); }
void setComments(Comments c) { _comments = std::move(c); }

/**
* Creates a new Meta instance whose location encompasses both instances.
*/
Meta combineLocs(const Meta* other) const;

/**
* Returns true if the location does not equal a default constructed
* instance.
Expand Down
8 changes: 6 additions & 2 deletions hilti/toolchain/include/ast/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <memory>
#include <optional>
#include <ostream>
#include <set>
#include <string>
#include <unordered_set>
#include <utility>
Expand Down Expand Up @@ -486,16 +485,21 @@ class Node {
* @param ctx current context in use
* @param n child node to add; it's ok for this to be null to leave a child slot unset
*/
void addChild(ASTContext* ctx, Node* n) {
void addChild(ASTContext* ctx, Node* n, bool update_meta = true) {
if ( ! n ) {
_children.emplace_back(nullptr);
return;
}

n = _newChild(ctx, n);

// The child should always inherit the parent's location if it doesn't have
// one. Otherwise, try to get a more accurate location for nodes with
// children. The location should encompass all children, if possible.
if ( ! n->location() && _meta->location() )
n->_meta = _meta;
else if ( update_meta && _meta->location() && n->location() )
setMeta(_meta->combineLocs(n->_meta));

_children.emplace_back(n);
n->_parent = this;
Expand Down
43 changes: 43 additions & 0 deletions hilti/toolchain/src/ast/meta.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,46 @@
using namespace hilti;

std::unordered_set<Meta> Meta::_cache;

Meta Meta::combineLocs(const Meta* other) const {
if ( location().file() != other->location().file() )
return *this;

// We have to be careful about column numbers when the line changes
int from_line = -1;
int to_line = -1;
int from_character = -1;
int to_character = -1;

// These calculations are weird. The logic:
//
// 1) If this starts on a line before other, then choose this for the first point
// 2) If this and other are same line, choose this if it is a lower column
// 3) Otherwise, choose other
//
// Then the same logic applies to 'to' as well
if ( location().from() < other->location().from() ||
(location().from() == other->location().from() && location().fromChar() < other->location().fromChar()) ) {
from_line = location().from();
from_character = location().fromChar();
}
else {
from_line = other->location().from();
from_character = other->location().fromChar();
}

if ( location().to() > other->location().to() ||
(location().to() == other->location().to() && location().toChar() > other->location().toChar()) ) {
to_line = location().to();
to_character = location().toChar();
}
else {
to_line = other->location().to();
to_character = other->location().toChar();
}

Location loc(location().file(), from_line, to_line, from_character, to_character);

// Children can keep their own, separate comments. Just use this one's comments.
return Meta(loc, _comments);
}
3 changes: 2 additions & 1 deletion hilti/toolchain/src/ast/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ Node* node::detail::deepcopy(ASTContext* ctx, Node* n, bool force) {
auto clone = n->_clone(ctx);

for ( const auto& c : n->children() )
clone->addChild(ctx, c); // this will copy the children recursively (because they have a parent already)
clone->addChild(ctx, c, /*update_meta=*/false); // this will copy the children recursively
// (because they have a parent already)

return clone;
}
Expand Down
3 changes: 1 addition & 2 deletions spicy/toolchain/src/compiler/validator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -363,8 +363,7 @@ struct VisitorPost : visitor::PreOrder, hilti::validator::VisitorMixIn {
const bool isAlias = n->type()->print() != n->typeID().str();
if ( isAlias ) {
if ( ! n->attributes()->attributes().empty() )
// TODO(#1893): This should diagnose on the attribute set
error("attributes are not allow on type aliases", n);
error("attributes are not allow on type aliases", n->attributes());
}

if ( n->linkage() == hilti::declaration::Linkage::Public && n->type()->alias() ) {
Expand Down
Loading

0 comments on commit 2da497e

Please sign in to comment.