From 896254eea52cffef340c630d7c640ec3563a5ce8 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 24 Jan 2017 12:48:47 -0800 Subject: [PATCH] Fix error in changed ranges calculation There was an error in the way that we calculate the reference scope sequences that are used as the basis for assertions about changed ranges in randomized tests. The error caused some characters' scopes to not be checked. This corrects the reference implementation and fixes a previously uncaught bug in the implementation of `tree_path_get_changed_ranges`. Previously, when iterating over the old and new trees, we would only perform comparisons of visible nodes. This resulted in a failure to do any comparison for portions of the text in which there were trailing invisible child nodes (e.g. trailing `_line_break` nodes inside `statement` nodes in the JavaScript grammar). Now, we additionally perform comparisons at invisible leaf nodes, based on their lowest visible ancestor. --- spec/helpers/point_helpers.cc | 4 +- spec/helpers/scope_sequence.cc | 25 +++---- src/runtime/length.h | 9 +-- src/runtime/tree_path.h | 122 +++++++++++++++++++-------------- 4 files changed, 86 insertions(+), 74 deletions(-) diff --git a/spec/helpers/point_helpers.cc b/spec/helpers/point_helpers.cc index e9c99259..60f4f9a7 100644 --- a/spec/helpers/point_helpers.cc +++ b/spec/helpers/point_helpers.cc @@ -15,7 +15,9 @@ bool operator==(const TSRange &left, const TSRange &right) { } bool operator==(const Length &left, const Length &right) { - return length_eq(left, right); + return left.bytes == right.bytes && + left.chars == right.chars && + left.extent == right.extent; } bool operator<(const TSPoint &left, const TSPoint &right) { diff --git a/spec/helpers/scope_sequence.cc b/spec/helpers/scope_sequence.cc index 87e059dc..d6e2e3b1 100644 --- a/spec/helpers/scope_sequence.cc +++ b/spec/helpers/scope_sequence.cc @@ -23,20 +23,21 @@ static void append_to_scope_sequence(ScopeSequence *sequence, ScopeStack *current_scopes, TSNode node, TSDocument *document, const std::string &text) { - append_text_to_scope_sequence(sequence, current_scopes, text, ts_node_start_byte(node) - sequence->size()); + append_text_to_scope_sequence( + sequence, current_scopes, text, ts_node_start_byte(node) - sequence->size() + ); - string scope = ts_node_type(node, document); - current_scopes->push_back(scope); - size_t child_count = ts_node_child_count(node); - if (child_count > 0) { - for (size_t i = 0; i < child_count; i++) { - TSNode child = ts_node_child(node, i); - append_to_scope_sequence(sequence, current_scopes, child, document, text); - } - } else { - size_t length = ts_node_end_byte(node) - ts_node_start_byte(node); - append_text_to_scope_sequence(sequence, current_scopes, text, length); + current_scopes->push_back(ts_node_type(node, document)); + + for (size_t i = 0, n = ts_node_child_count(node); i < n; i++) { + TSNode child = ts_node_child(node, i); + append_to_scope_sequence(sequence, current_scopes, child, document, text); } + + append_text_to_scope_sequence( + sequence, current_scopes, text, ts_node_end_byte(node) - sequence->size() + ); + current_scopes->pop_back(); } diff --git a/src/runtime/length.h b/src/runtime/length.h index 2477bbe1..352215d2 100644 --- a/src/runtime/length.h +++ b/src/runtime/length.h @@ -21,12 +21,11 @@ static inline void length_set_unknown_chars(Length *self) { } static inline Length length_min(Length len1, Length len2) { - return (len1.chars < len2.chars) ? len1 : len2; + return (len1.bytes < len2.bytes) ? len1 : len2; } static inline Length length_add(Length len1, Length len2) { Length result; - result.chars = len1.chars + len2.chars; result.bytes = len1.bytes + len2.bytes; result.extent = point_add(len1.extent, len2.extent); @@ -57,10 +56,4 @@ static inline Length length_zero() { return (Length){ 0, 0, {0, 0} }; } -static inline bool length_eq(Length self, Length other) { - return self.bytes == other.bytes && self.chars == other.chars && - self.extent.row == other.extent.row && - self.extent.column == other.extent.column; -} - #endif diff --git a/src/runtime/tree_path.h b/src/runtime/tree_path.h index 6fd4ef97..f64dd02f 100644 --- a/src/runtime/tree_path.h +++ b/src/runtime/tree_path.h @@ -21,61 +21,66 @@ static void range_array_add(RangeArray *results, TSPoint start, TSPoint end) { } } -static bool tree_path_descend(TreePath *path, TSPoint position) { +static bool tree_path_descend(TreePath *path, Length position) { uint32_t original_size = path->size; + bool did_descend; do { did_descend = false; TreePathEntry entry = *array_back(path); - Length child_position = entry.position; + Length child_left = entry.position; for (uint32_t i = 0; i < entry.tree->child_count; i++) { Tree *child = entry.tree->children[i]; - Length child_right_position = - length_add(child_position, ts_tree_total_size(child)); - if (point_lt(position, child_right_position.extent)) { - TreePathEntry child_entry = { child, child_position, i }; - if (child->visible) { + Length child_right = length_add(child_left, ts_tree_total_size(child)); + if (position.bytes < child_right.bytes) { + TreePathEntry child_entry = { child, child_left, i }; + if (child->visible || child->child_count == 0) { array_push(path, child_entry); return true; - } else if (child->child_count > 0 && child->visible_child_count > 0) { + } else if (child->visible_child_count > 0) { array_push(path, child_entry); did_descend = true; break; } } - child_position = child_right_position; + child_left = child_right; } } while (did_descend); + path->size = original_size; return false; } static uint32_t tree_path_advance(TreePath *path) { uint32_t ascend_count = 0; + while (path->size > 0) { TreePathEntry entry = array_pop(path); - if (path->size == 0) - break; + if (path->size == 0) break; TreePathEntry parent_entry = *array_back(path); if (parent_entry.tree->visible) ascend_count++; - Length position = - length_add(entry.position, ts_tree_total_size(entry.tree)); + + Length position = length_add(entry.position, ts_tree_total_size(entry.tree)); for (uint32_t i = entry.child_index + 1; i < parent_entry.tree->child_count; i++) { Tree *next_child = parent_entry.tree->children[i]; - if (next_child->visible || next_child->visible_child_count > 0) { + if (next_child->visible || + next_child->child_count == 0 || + next_child->visible_child_count > 0) { if (parent_entry.tree->visible) ascend_count--; array_push(path, ((TreePathEntry){ .tree = next_child, .child_index = i, .position = position, })); - if (!next_child->visible) - tree_path_descend(path, (TSPoint){ 0, 0 }); + if (!next_child->visible) { + tree_path_descend(path, length_zero()); + } return ascend_count; } position = length_add(position, ts_tree_total_size(next_child)); } } + return ascend_count; } @@ -94,8 +99,27 @@ static void tree_path_init(TreePath *path, Tree *tree) { .position = { 0, 0, { 0, 0 } }, .child_index = 0, })); - if (!tree->visible) - tree_path_descend(path, (TSPoint){ 0, 0 }); + if (!tree->visible) { + tree_path_descend(path, length_zero()); + } +} + +Tree *tree_path_visible_tree(TreePath *self) { + for (uint32_t i = self->size - 1; i + 1 > 0; i--) { + Tree *tree = self->contents[i].tree; + if (tree->visible) return tree; + } + return NULL; +} + +Length tree_path_start_position(TreePath *self) { + TreePathEntry entry = *array_back(self); + return length_add(entry.position, entry.tree->padding); +} + +Length tree_path_end_position(TreePath *self) { + TreePathEntry entry = *array_back(self); + return length_add(length_add(entry.position, entry.tree->padding), entry.tree->size); } static bool tree_must_eq(Tree *old_tree, Tree *new_tree) { @@ -112,67 +136,59 @@ static bool tree_must_eq(Tree *old_tree, Tree *new_tree) { static void tree_path_get_changes(TreePath *old_path, TreePath *new_path, TSRange **ranges, uint32_t *range_count) { - TSPoint position = { 0, 0 }; + Length position = length_zero(); RangeArray results = array_new(); while (old_path->size && new_path->size) { bool is_changed = false; - TSPoint next_position = position; + Length next_position = position; - TreePathEntry old_entry = *array_back(old_path); - TreePathEntry new_entry = *array_back(new_path); - Tree *old_tree = old_entry.tree; - Tree *new_tree = new_entry.tree; - uint32_t old_start_byte = old_entry.position.bytes + old_tree->padding.bytes; - uint32_t new_start_byte = new_entry.position.bytes + new_tree->padding.bytes; - TSPoint old_start_point = - point_add(old_entry.position.extent, old_tree->padding.extent); - TSPoint new_start_point = - point_add(new_entry.position.extent, new_tree->padding.extent); - TSPoint old_end_point = point_add(old_start_point, old_tree->size.extent); - TSPoint new_end_point = point_add(new_start_point, new_tree->size.extent); + Tree *old_tree = tree_path_visible_tree(old_path); + Tree *new_tree = tree_path_visible_tree(new_path); + Length old_start = tree_path_start_position(old_path); + Length new_start = tree_path_start_position(new_path); + Length old_end = tree_path_end_position(old_path); + Length new_end = tree_path_end_position(new_path); // #define NAME(t) (ts_language_symbol_name(language, ((Tree *)(t))->symbol)) - // printf("At [%-2lu, %-2lu] Compare (%-20s\t [%-2lu, %-2lu] - [%lu, %lu])\tvs\t(%-20s\t [%lu, %lu] - [%lu, %lu])\n", - // position.row, position.column, NAME(old_tree), old_start_point.row, - // old_start_point.column, old_end_point.row, old_end_point.column, - // NAME(new_tree), new_start_point.row, new_start_point.column, - // new_end_point.row, new_end_point.column); + // printf("At [%-2u, %-2u] Compare (%-20s\t [%-2u, %-2u] - [%u, %u])\tvs\t(%-20s\t [%u, %u] - [%u, %u])\n", + // position.extent.row, position.extent.column, + // NAME(old_tree), old_start.extent.row, old_start.extent.column, old_end.extent.row, old_end.extent.column, + // NAME(new_tree), new_start.extent.row, new_start.extent.column, new_end.extent.row, new_end.extent.column); - if (point_lt(position, old_start_point)) { - if (point_lt(position, new_start_point)) { - next_position = point_min(old_start_point, new_start_point); + if (position.bytes < old_start.bytes) { + if (position.bytes < new_start.bytes) { + next_position = length_min(old_start, new_start); } else { is_changed = true; - next_position = old_start_point; + next_position = old_start; } - } else if (point_lt(position, new_start_point)) { + } else if (position.bytes < new_start.bytes) { is_changed = true; - next_position = new_start_point; - } else if (old_start_byte == new_start_byte && - tree_must_eq(old_tree, new_tree)) { - next_position = old_end_point; + next_position = new_start; + } else if (old_start.bytes == new_start.bytes && tree_must_eq(old_tree, new_tree)) { + next_position = old_end; } else if (old_tree->symbol == new_tree->symbol) { if (tree_path_descend(old_path, position)) { if (!tree_path_descend(new_path, position)) { tree_path_ascend(old_path, 1); is_changed = true; - next_position = new_end_point; + next_position = new_end; } } else if (tree_path_descend(new_path, position)) { tree_path_ascend(new_path, 1); is_changed = true; - next_position = old_end_point; + next_position = old_end; } else { - next_position = point_min(old_end_point, new_end_point); + next_position = length_min(old_end, new_end); } } else { is_changed = true; - next_position = point_min(old_end_point, new_end_point); + next_position = length_min(old_end, new_end); } - bool at_old_end = point_lte(old_end_point, next_position); - bool at_new_end = point_lte(new_end_point, next_position); + bool at_old_end = old_end.bytes <= next_position.bytes; + bool at_new_end = new_end.bytes <= next_position.bytes; if (at_new_end && at_old_end) { uint32_t old_ascend_count = tree_path_advance(old_path); @@ -190,7 +206,7 @@ static void tree_path_get_changes(TreePath *old_path, TreePath *new_path, tree_path_ascend(new_path, ascend_count); } - if (is_changed) range_array_add(&results, position, next_position); + if (is_changed) range_array_add(&results, position.extent, next_position.extent); position = next_position; }