Fix precedence calculations when building parse table

* Recurse into choice rules
* Compute reduction precedence differently than shift precedence

include/tree_sitter/compiler.h

@@ -15,7 +15,8 @@ class Rule;
 typedef std::shared_ptr<Rule> rule_ptr;
 
 enum Associativity {
-  AssociativityLeft = 1,
+  AssociativityNone = 0,
+  AssociativityLeft,
   AssociativityRight,
 };
 

project.gyp

@@ -14,6 +14,7 @@
         'src/compiler/build_tables/build_parse_table.cc',
         'src/compiler/build_tables/build_tables.cc',
         'src/compiler/build_tables/first_symbols.cc',
+        'src/compiler/build_tables/get_completion_status.cc',
         'src/compiler/build_tables/get_metadata.cc',
         'src/compiler/build_tables/item.cc',
         'src/compiler/build_tables/item_set_closure.cc',

spec/compiler/build_tables/get_metadata_spec.cc

@@ -7,11 +7,24 @@ using namespace build_tables;
 
 START_TEST
 
-describe("get_metadata", []() {
+describe("get_metadata(rule, key)", []() {
   MetadataKey key1 = MetadataKey(100);
   MetadataKey key2 = MetadataKey(101);
   rule_ptr rule;
 
+  describe("with a rule without the metadata key", [&]() {
+    it("returns the zero range", [&]() {
+      rule = sym("x");
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(0, 0)));
+
+      rule = seq({ sym("x"), sym("y") });
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(0, 0)));
+
+      rule = metadata(seq({ sym("x"), sym("y") }), {{key2, 5}});
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(0, 0)));
+    });
+  });
+
   describe("when given a metadata rule", [&]() {
     before_each([&]() {
       rule = make_shared<Metadata>(sym("x"), map<MetadataKey, int>({
@@ -21,12 +34,12 @@ describe("get_metadata", []() {
     });
 
     it("returns the value for the given key", [&]() {
-      AssertThat(get_metadata(rule, key1), Equals(1));
-      AssertThat(get_metadata(rule, key2), Equals(2));
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(1, 1)));
+      AssertThat(get_metadata(rule, key2), Equals(MetadataRange(2, 2)));
     });
 
     it("returns 0 if the rule does not have the key", [&]() {
-      AssertThat(get_metadata(rule, MetadataKey(0)), Equals(0));
+      AssertThat(get_metadata(rule, MetadataKey(0)), Equals(MetadataRange(0, 0)));
     });
 
     describe("when the rule contains another metadata rule", [&]() {
@@ -35,15 +48,58 @@ describe("get_metadata", []() {
           { key1, 1 }
         })), map<MetadataKey, int>());
 
-        AssertThat(get_metadata(rule, key1), Equals(1));
+        AssertThat(get_metadata(rule, key1), Equals(MetadataRange(1, 1)));
       });
     });
   });
 
-  describe("when given a non-metadata rule", [&]() {
-    it("returns 0", [&]() {
-      auto rule = sym("x");
-      AssertThat(get_metadata(rule, key1), Equals(0));
+  describe("with a sequence starting with a metadata rule", [&]() {
+    it("returns the metadata rule's value for the key", [&]() {
+      rule = seq({
+        metadata(sym("x"), {{key1, 5}}),
+        sym("y")
+      });
+
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(5, 5)));
+    });
+  });
+
+  describe("with a sequence whose starting value can be blank", [&]() {
+    it("includes later elements of the sequence in the returned range", [&]() {
+      rule = seq({
+        repeat(metadata(sym("x"), {{key1, 3}})),
+        choice({ metadata(sym("x"), {{key1, 5}}), blank() }),
+        metadata(sym("x"), {{key1, 7}}),
+        metadata(sym("x"), {{key1, 9}}),
+      });
+
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(3, 7)));
+    });
+  });
+
+  describe("with a sequence whose starting value can be blank", [&]() {
+    it("includes later elements of the sequence in the returned range", [&]() {
+      rule = seq({
+        repeat(metadata(sym("x"), {{key1, 3}})),
+        choice({ metadata(sym("x"), {{key1, 5}}), blank() }),
+        metadata(sym("x"), {{key1, 7}}),
+        metadata(sym("x"), {{key1, 9}}),
+      });
+
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(3, 7)));
+    });
+  });
+
+  describe("with a choice rule", [&]() {
+    it("merges the ranges for the choices elements", [&]() {
+      rule = choice({
+        metadata(sym("a"), {{key1, 5}}),
+        metadata(sym("b"), {{key1, 3}}),
+        sym("c"),
+        metadata(sym("d"), {{key1, 1}}),
+      });
+
+      AssertThat(get_metadata(rule, key1), Equals(MetadataRange(1, 5)));
     });
   });
 });

src/compiler/build_tables/build_lex_table.cc

@@ -6,6 +6,8 @@
 #include <vector>
 #include "compiler/build_tables/lex_conflict_manager.h"
 #include "compiler/build_tables/item_set_transitions.h"
+#include "compiler/build_tables/get_completion_status.h"
+#include "compiler/build_tables/get_metadata.h"
 #include "compiler/build_tables/lex_item.h"
 #include "compiler/parse_table.h"
 #include "compiler/lexical_grammar.h"
@@ -106,13 +108,16 @@ class LexTableBuilder {
   }
 
   void add_accept_token_actions(const LexItemSet &item_set, LexStateId state_id) {
-    for (const LexItem &item : item_set)
-      if (item.is_done()) {
+    for (const LexItem &item : item_set) {
+      CompletionStatus completion_status = get_completion_status(item.rule);
+      if (completion_status.is_done) {
         auto current_action = lex_table.state(state_id).default_action;
-        auto new_action = LexAction::Accept(item.lhs, item.precedence());
+        auto new_action =
+          LexAction::Accept(item.lhs, completion_status.precedence);
         if (conflict_manager.resolve(new_action, current_action))
           lex_table.state(state_id).default_action = new_action;
       }
+    }
   }
 
   void add_token_start(const LexItemSet &item_set, LexStateId state_id) {
@@ -140,8 +145,11 @@ class LexTableBuilder {
 
   set<int> precedence_values_for_item_set(const LexItemSet &item_set) const {
     set<int> result;
-    for (const auto &item : item_set)
-      result.insert(item.precedence());
+    for (const auto &item : item_set) {
+      auto precedence_range = get_metadata(item.rule, rules::PRECEDENCE);
+      result.insert(precedence_range.min);
+      result.insert(precedence_range.max);
+    }
     return result;
   }
 };

src/compiler/build_tables/build_parse_table.cc

@@ -9,6 +9,8 @@
 #include "compiler/build_tables/item_set_transitions.h"
 #include "compiler/build_tables/parse_conflict_manager.h"
 #include "compiler/build_tables/parse_item.h"
+#include "compiler/build_tables/get_completion_status.h"
+#include "compiler/build_tables/get_metadata.h"
 #include "compiler/lexical_grammar.h"
 #include "compiler/syntax_grammar.h"
 #include "compiler/rules/symbol.h"
@@ -109,12 +111,14 @@ class ParseTableBuilder {
       const ParseItem &item = pair.first;
       const set<Symbol> &lookahead_symbols = pair.second;
 
-      if (item.is_done()) {
+      CompletionStatus completion_status = get_completion_status(item.rule);
+      if (completion_status.is_done) {
         ParseAction action =
           (item.lhs == rules::START())
             ? ParseAction::Accept()
             : ParseAction::Reduce(item.lhs, item.consumed_symbols.size(),
-                                  item.precedence(), item.associativity(),
+                                  completion_status.precedence,
+                                  completion_status.associativity,
                                   get_production_id(item.consumed_symbols));
 
         for (const auto &lookahead_sym : lookahead_symbols)
@@ -206,8 +210,11 @@ class ParseTableBuilder {
     set<int> result;
     for (const auto &pair : item_set) {
       const ParseItem &item = pair.first;
-      if (!item.consumed_symbols.empty())
-        result.insert(item.precedence());
+      if (!item.consumed_symbols.empty()) {
+        auto precedence_range = get_metadata(item.rule, rules::PRECEDENCE);
+        result.insert(precedence_range.min);
+        result.insert(precedence_range.max);
+      }
     }
     return result;
   }

src/compiler/build_tables/get_completion_status.cc

@@ -0,0 +1,53 @@
+#include "compiler/build_tables/get_completion_status.h"
+#include "compiler/rules/visitor.h"
+#include "compiler/rules/choice.h"
+#include "compiler/rules/seq.h"
+#include "compiler/rules/metadata.h"
+
+namespace tree_sitter {
+namespace build_tables {
+
+class GetCompletionStatus : public rules::RuleFn<CompletionStatus> {
+ protected:
+  CompletionStatus apply_to(const rules::Choice *rule) {
+    for (const auto &element : rule->elements) {
+      CompletionStatus status = apply(element);
+      if (status.is_done)
+        return status;
+    }
+    return { false, 0, rules::AssociativityNone };
+  }
+
+  CompletionStatus apply_to(const rules::Metadata *rule) {
+    CompletionStatus result = apply(rule->rule);
+    if (result.is_done && !result.associativity) {
+      result.precedence = rule->value_for(rules::PRECEDENCE);
+      result.associativity =
+        (rules::Associativity)(rule->value_for(rules::ASSOCIATIVITY));
+    }
+    return result;
+  }
+
+  CompletionStatus apply_to(const rules::Repeat *rule) {
+    return { true, 0, rules::AssociativityNone };
+  }
+
+  CompletionStatus apply_to(const rules::Blank *rule) {
+    return { true, 0, rules::AssociativityNone };
+  }
+
+  CompletionStatus apply_to(const rules::Seq *rule) {
+    CompletionStatus left_status = apply(rule->left);
+    if (left_status.is_done)
+      return apply(rule->right);
+    else
+      return { false, 0, rules::AssociativityNone };
+  }
+};
+
+CompletionStatus get_completion_status(const rules::rule_ptr &rule) {
+  return GetCompletionStatus().apply(rule);
+}
+
+}  // namespace build_tables
+}  // namespace tree_sitter

src/compiler/build_tables/get_completion_status.h

@@ -0,0 +1,20 @@
+#ifndef COMPILER_BUILD_TABLES_GET_COMPLETION_STATUS_H_
+#define COMPILER_BUILD_TABLES_GET_COMPLETION_STATUS_H_
+
+#include "tree_sitter/compiler.h"
+
+namespace tree_sitter {
+namespace build_tables {
+
+struct CompletionStatus {
+  bool is_done;
+  int precedence;
+  rules::Associativity associativity;
+};
+
+CompletionStatus get_completion_status(const rules::rule_ptr &);
+
+}  // namespace build_tables
+}  // namespace tree_sitter
+
+#endif  // COMPILER_BUILD_TABLES_GET_COMPLETION_STATUS_H_

src/compiler/build_tables/get_metadata.cc

@@ -1,32 +1,74 @@
 #include "compiler/build_tables/get_metadata.h"
+#include <utility>
+#include <string>
 #include "compiler/rules/visitor.h"
 #include "compiler/rules/seq.h"
+#include "compiler/rules/repeat.h"
+#include "compiler/rules/choice.h"
+#include "compiler/build_tables/rule_can_be_blank.h"
 
 namespace tree_sitter {
 namespace build_tables {
 
-int get_metadata(const rules::rule_ptr &rule, rules::MetadataKey key) {
-  class GetMetadata : public rules::RuleFn<int> {
+using std::pair;
+using std::string;
+using std::to_string;
+
+std::ostream &operator<<(std::ostream &stream, const MetadataRange &range) {
+  return stream << string("{") << to_string(range.min) << string(", ")
+                << to_string(range.max) << string("}");
+}
+
+MetadataRange get_metadata(const rules::rule_ptr &rule, rules::MetadataKey key) {
+  class GetMetadata : public rules::RuleFn<pair<MetadataRange, bool>> {
     rules::MetadataKey metadata_key;
 
    public:
     explicit GetMetadata(rules::MetadataKey key) : metadata_key(key) {}
 
    protected:
-    int apply_to(const rules::Metadata *rule) {
-      int result = rule->value_for(metadata_key);
-      return (result != 0) ? result : apply(rule->rule);
+    pair<MetadataRange, bool> apply_to(const rules::Metadata *rule) {
+      pair<MetadataRange, bool> result = apply(rule->rule);
+      if (result.second) {
+        return result;
+      } else {
+        int value = rule->value_for(metadata_key);
+        return { { value, value }, value != 0 };
+      }
     }
 
-    // TODO -
-    // Remove this. It is currently needed to make the rule generated
-    // by `LexTableBuilder::after_separators` have the right precedence.
-    int apply_to(const rules::Seq *rule) {
-      return apply(rule->left);
+    pair<MetadataRange, bool> apply_to(const rules::Choice *rule) {
+      pair<MetadataRange, bool> result(MetadataRange(0, 0), false);
+      for (const auto &element : rule->elements)
+        merge_result(&result, apply(element));
+      return result;
+    }
+
+    pair<MetadataRange, bool> apply_to(const rules::Seq *rule) {
+      pair<MetadataRange, bool> result = apply(rule->left);
+      if (rule_can_be_blank(rule->left))
+        merge_result(&result, apply(rule->right));
+      return result;
+    }
+
+    pair<MetadataRange, bool> apply_to(const rules::Repeat *rule) {
+      return apply(rule->content);
+    }
+
+   private:
+    void merge_result(pair<MetadataRange, bool> *left,
+                      const pair<MetadataRange, bool> &right) {
+      if (right.second) {
+        if (!left->second || right.first.min < left->first.min)
+          left->first.min = right.first.min;
+        if (!left->second || right.first.max > left->first.max)
+          left->first.max = right.first.max;
+        left->second = true;
+      }
     }
   };
 
-  return GetMetadata(key).apply(rule);
+  return GetMetadata(key).apply(rule).first;
 }
 
 }  // namespace build_tables

src/compiler/build_tables/get_metadata.h

@@ -7,7 +7,21 @@
 namespace tree_sitter {
 namespace build_tables {
 
-int get_metadata(const rules::rule_ptr &, rules::MetadataKey);
+struct MetadataRange {
+  MetadataRange() : MetadataRange(0, 0) {}
+  MetadataRange(int min, int max) : min(min), max(max) {}
+
+  bool operator==(const MetadataRange &other) const {
+    return min == other.min && max == other.max;
+  }
+
+  int min;
+  int max;
+};
+
+std::ostream &operator<<(std::ostream &stream, const MetadataRange &range);
+
+MetadataRange get_metadata(const rules::rule_ptr &, rules::MetadataKey);
 
 }  // namespace build_tables
 }  // namespace tree_sitter

src/compiler/build_tables/item.cc

@@ -1,7 +1,4 @@
-#include "compiler/build_tables/get_metadata.h"
 #include "compiler/build_tables/item.h"
-#include "compiler/build_tables/rule_can_be_blank.h"
-#include "compiler/rules/metadata.h"
 #include "tree_sitter/compiler.h"
 
 namespace tree_sitter {
@@ -10,13 +7,5 @@ namespace build_tables {
 Item::Item(const rules::Symbol &lhs, const rules::rule_ptr rule)
     : lhs(lhs), rule(rule) {}
 
-bool Item::is_done() const {
-  return rule_can_be_blank(rule);
-}
-
-int Item::precedence() const {
-  return get_metadata(rule, rules::PRECEDENCE);
-}
-
 }  // namespace build_tables
 }  // namespace tree_sitter

src/compiler/build_tables/item.h

@@ -10,8 +10,6 @@ namespace build_tables {
 class Item {
  public:
   Item(const rules::Symbol &lhs, rules::rule_ptr rule);
-  bool is_done() const;
-  int precedence() const;
 
   rules::Symbol lhs;
   rules::rule_ptr rule;

src/compiler/build_tables/lex_item.cc

@@ -1,5 +1,5 @@
 #include "compiler/build_tables/lex_item.h"
-#include "compiler/build_tables/rule_can_be_blank.h"
+#include "compiler/build_tables/get_metadata.h"
 #include "compiler/rules/symbol.h"
 #include "compiler/rules/metadata.h"
 #include "compiler/rules/seq.h"
@@ -19,22 +19,7 @@ bool LexItem::operator==(const LexItem &other) const {
 }
 
 bool LexItem::is_token_start() const {
-  class IsTokenStart : public rules::RuleFn<bool> {
-    bool apply_to(const rules::Seq *rule) {
-      if (apply(rule->left))
-        return true;
-      else if (rule_can_be_blank(rule->left))
-        return apply(rule->right);
-      else
-        return false;
-    }
-
-    bool apply_to(const rules::Metadata *rule) {
-      return rule->value_for(rules::START_TOKEN);
-    }
-  };
-
-  return IsTokenStart().apply(rule);
+  return get_metadata(rule, rules::START_TOKEN).max > 0;
 }
 
 ostream &operator<<(ostream &stream, const LexItem &item) {

src/compiler/build_tables/parse_conflict_manager.cc

@@ -31,9 +31,12 @@ pair<bool, ConflictType> ParseConflictManager::resolve(
         int min_precedence = *old_action.precedence_values.begin();
         int max_precedence = *old_action.precedence_values.rbegin();
         int new_precedence = *new_action.precedence_values.rbegin();
-        if (new_precedence < min_precedence)
+        if (new_precedence < min_precedence ||
+            (new_precedence == min_precedence && min_precedence < max_precedence))
           return { false, ConflictTypeResolved };
-        else if (new_precedence > max_precedence)
+        else if (new_precedence > max_precedence ||
+                 (new_precedence == max_precedence &&
+                  min_precedence < max_precedence))
           return { true, ConflictTypeResolved };
         else if (min_precedence == max_precedence) {
           switch (new_action.associativity) {

src/compiler/build_tables/parse_item.cc

@@ -1,6 +1,4 @@
 #include "compiler/build_tables/parse_item.h"
-#include "compiler/build_tables/get_metadata.h"
-#include "compiler/rules/metadata.h"
 #include "tree_sitter/compiler.h"
 
 namespace tree_sitter {
@@ -32,10 +30,6 @@ bool ParseItem::operator<(const ParseItem &other) const {
   return rule < other.rule;
 }
 
-rules::Associativity ParseItem::associativity() const {
-  return rules::Associativity(get_metadata(rule, rules::ASSOCIATIVITY));
-}
-
 ostream &operator<<(ostream &stream, const ParseItem &item) {
   return stream << string("(item ") << item.lhs << string(" ") << *item.rule
                 << string(")");

src/compiler/build_tables/parse_item.h

@@ -6,7 +6,6 @@
 #include <vector>
 #include "compiler/build_tables/item.h"
 #include "compiler/rules/symbol.h"
-#include "compiler/rules/metadata.h"
 
 namespace tree_sitter {
 namespace build_tables {
@@ -17,7 +16,6 @@ class ParseItem : public Item {
             const std::vector<rules::Symbol> &consumed_symbols);
   bool operator==(const ParseItem &other) const;
   bool operator<(const ParseItem &other) const;
-  rules::Associativity associativity() const;
   std::vector<rules::Symbol> consumed_symbols;
 };