Revert "Implemented QuoteString which mimics QuoteSourceLocation but uses string with offset as source (#73)" (#74)

This reverts commit d5ab5e6.

Author: smolkaj

p4_constraints/BUILD.bazel

@@ -21,12 +21,6 @@ cc_library(
     ],
 )
 
-cc_library(
-    name = "constraint_source",
-    hdrs = ["constraint_source.h"],
-    deps = [":ast_cc_proto"],
-)
-
 cc_test(
     name = "ast_test",
     size = "small",
@@ -58,11 +52,7 @@ cc_library(
     hdrs = ["quote.h"],
     deps = [
         ":ast_cc_proto",
-        ":constraint_source",
-        "//gutils:status",
         "@com_github_google_glog//:glog",
-        "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
-        "@com_google_absl//absl/strings:str_format",
     ],
 )

p4_constraints/backend/BUILD.bazel

@@ -21,7 +21,6 @@ cc_library(
         "//gutils:status",
         "//p4_constraints:ast",
         "//p4_constraints:ast_cc_proto",
-        "//p4_constraints:constraint_source",
         "//p4_constraints:quote",
         "@com_github_google_glog//:glog",
         "@com_github_p4lang_p4runtime//:p4runtime_cc_proto",
@@ -74,7 +73,6 @@ cc_library(
         "//gutils:status",
         "//p4_constraints:ast",
         "//p4_constraints:ast_cc_proto",
-        "//p4_constraints:constraint_source",
         "//p4_constraints:quote",
         "//p4_constraints/frontend:lexer",
         "//p4_constraints/frontend:parser",
@@ -101,6 +99,7 @@ cc_test(
         "//gutils:ordered_map",
         "//gutils:parse_text_proto",
         "//gutils:status",
+        "//net/proto2/io/public:io",
         "//p4_constraints:ast_cc_proto",
         "//p4_constraints/frontend:lexer",
         "//p4_constraints/frontend:parser",

p4_constraints/backend/constraint_info.cc

@@ -30,7 +30,6 @@
 #include "p4/config/v1/p4info.pb.h"
 #include "p4_constraints/ast.pb.h"
 #include "p4_constraints/backend/type_checker.h"
-#include "p4_constraints/constraint_source.h"
 #include "p4_constraints/frontend/lexer.h"
 #include "p4_constraints/frontend/parser.h"
 #include "re2/re2.h"
@@ -42,7 +41,8 @@ namespace {
 using p4::config::v1::MatchField;
 using p4::config::v1::Table;
 
-absl::StatusOr<ConstraintSource> ExtractConstraint(const Table& table) {
+absl::StatusOr<absl::optional<ast::Expression>> ParseTableConstraint(
+    const Table& table) {
   // We expect .p4 files to have the following format:
   // ```p4
   //   @file(__FILE__)              // optional
@@ -58,38 +58,38 @@ absl::StatusOr<ConstraintSource> ExtractConstraint(const Table& table) {
   const RE2 line_annotation = {R"RE(@line[(](\d+)[)])RE"};
   const RE2 constraint_annotation = {R"RE(@entry_restriction)RE"};
 
-  absl::string_view constraint_string = "";
-  ast::SourceLocation constraint_location;
+  ast::SourceLocation location;
   int line = 0;
+  absl::string_view constraint = "";
   for (absl::string_view annotation : table.preamble().annotations()) {
     if (RE2::Consume(&annotation, file_annotation,
-                     constraint_location.mutable_file_path()))
+                     location.mutable_file_path()))
       continue;
     if (RE2::Consume(&annotation, line_annotation, &line)) continue;
     if (RE2::Consume(&annotation, constraint_annotation)) {
-      constraint_string = annotation.data();
+      constraint = annotation.data();
       break;
     }
   }
-
-  if (constraint_string.empty()) return ConstraintSource();
-
-  constraint_location.set_line(line);
-  if (constraint_location.file_path().empty()) {
-    constraint_location.set_table_name(table.preamble().name());
+  location.set_line(line);
+  if (location.file_path().empty()) {
+    location.set_table_name(table.preamble().name());
   }
 
-  if (!absl::ConsumePrefix(&constraint_string, "(\"") ||
-      !absl::ConsumeSuffix(&constraint_string, "\")")) {
+  if (constraint.empty()) {
+    return absl::optional<ast::Expression>();
+  }
+  if (!absl::ConsumePrefix(&constraint, "(\"") ||
+      !absl::ConsumeSuffix(&constraint, "\")")) {
     return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
            << "In table " << table.preamble().name() << ":\n"
            << "Syntax error: @entry_restriction must be enclosed in "
               "'(\"' and '\")'";
   }
-  return ConstraintSource{
-      .constraint_string = std::string(constraint_string),
-      .constraint_location = constraint_location,
-  };
+  // TODO(smolkaj): With C++17, we can use std::string_view throughout and
+  // won't need to make an extra copy here.
+  const auto constraint_str = std::string(constraint);
+  return ParseConstraint(Tokenize(constraint_str, location));
 }
 
 absl::StatusOr<ast::Type> ParseKeyType(const MatchField& key) {
@@ -145,26 +145,14 @@ absl::StatusOr<TableInfo> ParseTableInfo(const Table& table) {
     }
   }
 
-  ASSIGN_OR_RETURN(ConstraintSource constraint_source,
-                   ExtractConstraint(table));
-
-  absl::optional<ast::Expression> constraint = absl::nullopt;
-  if (!constraint_source.constraint_string.empty()) {
-    ASSIGN_OR_RETURN(constraint, ParseConstraint(Tokenize(
-                                     constraint_source.constraint_string,
-                                     constraint_source.constraint_location)));
-  }
-
-  TableInfo table_info{
-      .id = table.preamble().id(),
-      .name = table.preamble().name(),
-      .constraint = constraint,
-      .constraint_source = constraint_source,
-      .keys_by_id = keys_by_id,
-      .keys_by_name = keys_by_name,
-  };
+  TableInfo table_info{.id = table.preamble().id(),
+                       .name = table.preamble().name(),
+                       .constraint = {},  // Inserted in a second.
+                       .keys_by_id = keys_by_id,
+                       .keys_by_name = keys_by_name};
 
-  // Type check constraint.
+  // Parse and type check constraint.
+  ASSIGN_OR_RETURN(table_info.constraint, ParseTableConstraint(table));
   if (table_info.constraint.has_value()) {
     RETURN_IF_ERROR(
         InferAndCheckTypes(&table_info.constraint.value(), table_info));

p4_constraints/backend/constraint_info.h

@@ -32,7 +32,6 @@
 #include "absl/types/variant.h"
 #include "p4/config/v1/p4info.pb.h"
 #include "p4_constraints/ast.pb.h"
-#include "p4_constraints/constraint_source.h"
 
 namespace p4_constraints {
 
@@ -51,9 +50,6 @@ struct TableInfo {
 
   // An optional constraint (aka entry_restriction) on table entries.
   absl::optional<ast::Expression> constraint;
-  // If member `constraint` is present, this captures its source. Arbitrary
-  // otherwise.
-  ConstraintSource constraint_source;
 
   // Maps from key IDs/names to KeyInfo.
   // Derives from Table.match_fields in p4info.proto.

p4_constraints/backend/interpreter.cc

@@ -41,7 +41,6 @@
 #include "p4_constraints/ast.h"
 #include "p4_constraints/ast.pb.h"
 #include "p4_constraints/backend/constraint_info.h"
-#include "p4_constraints/constraint_source.h"
 #include "p4_constraints/quote.h"
 
 namespace p4_constraints {
@@ -523,8 +522,7 @@ absl::StatusOr<const Expression*> MinimalSubexpressionLeadingToEvalResult(
           }
 
           if (candidate_subexpressions.empty()) return &expression;
-          // Returns `MinimalSubexpressionLeadingToEvalResult` of sole
-          // candidate.
+          // Returns `MinimalSubexpressionLeadingToEvalResult` of sole candidate
           if (candidate_subexpressions.size() == 1) {
             return MinimalSubexpressionLeadingToEvalResult(
                 *candidate_subexpressions[0], entry, eval_cache, size_cache);
@@ -563,34 +561,32 @@ absl::StatusOr<const Expression*> MinimalSubexpressionLeadingToEvalResult(
 
 // Returns human readable explanation of constraint violation.
 absl::StatusOr<std::string> ExplainConstraintViolation(
-    const Expression& expr, const ConstraintSource& source,
-    const TableEntry& entry, EvaluationCache& eval_cache,
-    ast::SizeCache& size_cache) {
+    const Expression& expr, const TableEntry& entry,
+    EvaluationCache& eval_cache, ast::SizeCache& size_cache) {
   ASSIGN_OR_RETURN(const ast::Expression* explanation,
                    MinimalSubexpressionLeadingToEvalResult(
                        expr, entry, eval_cache, size_cache));
   ASSIGN_OR_RETURN(bool truth_value,
                    EvalToBool(*explanation, entry, &eval_cache));
-  ASSIGN_OR_RETURN(std::string reason,
-                   QuoteSubConstraint(source, explanation->start_location(),
-                                      explanation->end_location()));
   // Ordered for determinism when golden testing.
   std::string key_info = absl::StrJoin(
       gutils::Ordered(entry.keys), "\n", [](std::string* out, auto pair) {
         absl::StrAppend(out, "Key:\"", pair.first,
                         "\" -> Value: ", EvalResultToString(pair.second));
       });
-
+  // TODO(b/241957581): Find better representation for explanation. Consider
+  // modifying quote function to use string in constraint info
   return absl::StrFormat(
       "All entries must %ssatisfy:"
       "\n\n%s\n"
-      "But your entry does%s.\n"
+      "But your entry did%s\n"
       ">>>Entry Info<<<\n"
-      "Table Name: \"%s\"\n"
+      "Table Name:\"%s\"\n"
       "Priority:%d\n"
+      "Key Info\n"
       "%s\n",
-      (truth_value ? "not " : ""), reason, (truth_value ? "" : " not"),
-      entry.table_name, entry.priority, key_info);
+      (truth_value ? "not " : ""), explanation->DebugString(),
+      (truth_value ? "" : " not"), entry.table_name, entry.priority, key_info);
 }
 // -- Main evaluator -----------------------------------------------------------
 
@@ -804,8 +800,8 @@ absl::StatusOr<std::string> ReasonEntryViolatesConstraint(
     return "";
   }
   SizeCache size_cache;
-  return ExplainConstraintViolation(constraint, table_info.constraint_source,
-                                    parsed_entry, eval_cache, size_cache);
+  return ExplainConstraintViolation(constraint, parsed_entry, eval_cache,
+                                    size_cache);
 }
 
 }  // namespace p4_constraints