Skip to content

Commit d5ab5e6

Browse files
verios-googlePINS Team
andauthored
Implemented QuoteString which mimics QuoteSourceLocation but uses string with offset as source (#73)
-TableInfo struct now has members that hold table constraint string and its source location (for offset) -ExtractConstraint replaces ParseTableConstraint. -Small change in test for compatibility with new struct members PiperOrigin-RevId: 465638588 Co-authored-by: PINS Team <copybara-servicebot@google.com>
1 parent c821488 commit d5ab5e6

12 files changed

+540
-241
lines changed

p4_constraints/BUILD.bazel

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ cc_library(
2121
],
2222
)
2323

24+
cc_library(
25+
name = "constraint_source",
26+
hdrs = ["constraint_source.h"],
27+
deps = [":ast_cc_proto"],
28+
)
29+
2430
cc_test(
2531
name = "ast_test",
2632
size = "small",
@@ -52,7 +58,11 @@ cc_library(
5258
hdrs = ["quote.h"],
5359
deps = [
5460
":ast_cc_proto",
61+
":constraint_source",
62+
"//gutils:status",
5563
"@com_github_google_glog//:glog",
64+
"@com_google_absl//absl/status:statusor",
5665
"@com_google_absl//absl/strings",
66+
"@com_google_absl//absl/strings:str_format",
5767
],
5868
)

p4_constraints/backend/BUILD.bazel

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ cc_library(
2121
"//gutils:status",
2222
"//p4_constraints:ast",
2323
"//p4_constraints:ast_cc_proto",
24+
"//p4_constraints:constraint_source",
2425
"//p4_constraints:quote",
2526
"@com_github_google_glog//:glog",
2627
"@com_github_p4lang_p4runtime//:p4runtime_cc_proto",
@@ -73,6 +74,7 @@ cc_library(
7374
"//gutils:status",
7475
"//p4_constraints:ast",
7576
"//p4_constraints:ast_cc_proto",
77+
"//p4_constraints:constraint_source",
7678
"//p4_constraints:quote",
7779
"//p4_constraints/frontend:lexer",
7880
"//p4_constraints/frontend:parser",
@@ -99,7 +101,6 @@ cc_test(
99101
"//gutils:ordered_map",
100102
"//gutils:parse_text_proto",
101103
"//gutils:status",
102-
"//net/proto2/io/public:io",
103104
"//p4_constraints:ast_cc_proto",
104105
"//p4_constraints/frontend:lexer",
105106
"//p4_constraints/frontend:parser",

p4_constraints/backend/constraint_info.cc

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "p4/config/v1/p4info.pb.h"
3131
#include "p4_constraints/ast.pb.h"
3232
#include "p4_constraints/backend/type_checker.h"
33+
#include "p4_constraints/constraint_source.h"
3334
#include "p4_constraints/frontend/lexer.h"
3435
#include "p4_constraints/frontend/parser.h"
3536
#include "re2/re2.h"
@@ -41,8 +42,7 @@ namespace {
4142
using p4::config::v1::MatchField;
4243
using p4::config::v1::Table;
4344

44-
absl::StatusOr<absl::optional<ast::Expression>> ParseTableConstraint(
45-
const Table& table) {
45+
absl::StatusOr<ConstraintSource> ExtractConstraint(const Table& table) {
4646
// We expect .p4 files to have the following format:
4747
// ```p4
4848
// @file(__FILE__) // optional
@@ -58,38 +58,38 @@ absl::StatusOr<absl::optional<ast::Expression>> ParseTableConstraint(
5858
const RE2 line_annotation = {R"RE(@line[(](\d+)[)])RE"};
5959
const RE2 constraint_annotation = {R"RE(@entry_restriction)RE"};
6060

61-
ast::SourceLocation location;
61+
absl::string_view constraint_string = "";
62+
ast::SourceLocation constraint_location;
6263
int line = 0;
63-
absl::string_view constraint = "";
6464
for (absl::string_view annotation : table.preamble().annotations()) {
6565
if (RE2::Consume(&annotation, file_annotation,
66-
location.mutable_file_path()))
66+
constraint_location.mutable_file_path()))
6767
continue;
6868
if (RE2::Consume(&annotation, line_annotation, &line)) continue;
6969
if (RE2::Consume(&annotation, constraint_annotation)) {
70-
constraint = annotation.data();
70+
constraint_string = annotation.data();
7171
break;
7272
}
7373
}
74-
location.set_line(line);
75-
if (location.file_path().empty()) {
76-
location.set_table_name(table.preamble().name());
77-
}
7874

79-
if (constraint.empty()) {
80-
return absl::optional<ast::Expression>();
75+
if (constraint_string.empty()) return ConstraintSource();
76+
77+
constraint_location.set_line(line);
78+
if (constraint_location.file_path().empty()) {
79+
constraint_location.set_table_name(table.preamble().name());
8180
}
82-
if (!absl::ConsumePrefix(&constraint, "(\"") ||
83-
!absl::ConsumeSuffix(&constraint, "\")")) {
81+
82+
if (!absl::ConsumePrefix(&constraint_string, "(\"") ||
83+
!absl::ConsumeSuffix(&constraint_string, "\")")) {
8484
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
8585
<< "In table " << table.preamble().name() << ":\n"
8686
<< "Syntax error: @entry_restriction must be enclosed in "
8787
"'(\"' and '\")'";
8888
}
89-
// TODO(smolkaj): With C++17, we can use std::string_view throughout and
90-
// won't need to make an extra copy here.
91-
const auto constraint_str = std::string(constraint);
92-
return ParseConstraint(Tokenize(constraint_str, location));
89+
return ConstraintSource{
90+
.constraint_string = std::string(constraint_string),
91+
.constraint_location = constraint_location,
92+
};
9393
}
9494

9595
absl::StatusOr<ast::Type> ParseKeyType(const MatchField& key) {
@@ -145,14 +145,26 @@ absl::StatusOr<TableInfo> ParseTableInfo(const Table& table) {
145145
}
146146
}
147147

148-
TableInfo table_info{.id = table.preamble().id(),
149-
.name = table.preamble().name(),
150-
.constraint = {}, // Inserted in a second.
151-
.keys_by_id = keys_by_id,
152-
.keys_by_name = keys_by_name};
148+
ASSIGN_OR_RETURN(ConstraintSource constraint_source,
149+
ExtractConstraint(table));
150+
151+
absl::optional<ast::Expression> constraint = absl::nullopt;
152+
if (!constraint_source.constraint_string.empty()) {
153+
ASSIGN_OR_RETURN(constraint, ParseConstraint(Tokenize(
154+
constraint_source.constraint_string,
155+
constraint_source.constraint_location)));
156+
}
157+
158+
TableInfo table_info{
159+
.id = table.preamble().id(),
160+
.name = table.preamble().name(),
161+
.constraint = constraint,
162+
.constraint_source = constraint_source,
163+
.keys_by_id = keys_by_id,
164+
.keys_by_name = keys_by_name,
165+
};
153166

154-
// Parse and type check constraint.
155-
ASSIGN_OR_RETURN(table_info.constraint, ParseTableConstraint(table));
167+
// Type check constraint.
156168
if (table_info.constraint.has_value()) {
157169
RETURN_IF_ERROR(
158170
InferAndCheckTypes(&table_info.constraint.value(), table_info));

p4_constraints/backend/constraint_info.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "absl/types/variant.h"
3333
#include "p4/config/v1/p4info.pb.h"
3434
#include "p4_constraints/ast.pb.h"
35+
#include "p4_constraints/constraint_source.h"
3536

3637
namespace p4_constraints {
3738

@@ -50,6 +51,9 @@ struct TableInfo {
5051

5152
// An optional constraint (aka entry_restriction) on table entries.
5253
absl::optional<ast::Expression> constraint;
54+
// If member `constraint` is present, this captures its source. Arbitrary
55+
// otherwise.
56+
ConstraintSource constraint_source;
5357

5458
// Maps from key IDs/names to KeyInfo.
5559
// Derives from Table.match_fields in p4info.proto.

p4_constraints/backend/interpreter.cc

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
#include "p4_constraints/ast.h"
4242
#include "p4_constraints/ast.pb.h"
4343
#include "p4_constraints/backend/constraint_info.h"
44+
#include "p4_constraints/constraint_source.h"
4445
#include "p4_constraints/quote.h"
4546

4647
namespace p4_constraints {
@@ -522,7 +523,8 @@ absl::StatusOr<const Expression*> MinimalSubexpressionLeadingToEvalResult(
522523
}
523524

524525
if (candidate_subexpressions.empty()) return &expression;
525-
// Returns `MinimalSubexpressionLeadingToEvalResult` of sole candidate
526+
// Returns `MinimalSubexpressionLeadingToEvalResult` of sole
527+
// candidate.
526528
if (candidate_subexpressions.size() == 1) {
527529
return MinimalSubexpressionLeadingToEvalResult(
528530
*candidate_subexpressions[0], entry, eval_cache, size_cache);
@@ -561,32 +563,34 @@ absl::StatusOr<const Expression*> MinimalSubexpressionLeadingToEvalResult(
561563

562564
// Returns human readable explanation of constraint violation.
563565
absl::StatusOr<std::string> ExplainConstraintViolation(
564-
const Expression& expr, const TableEntry& entry,
565-
EvaluationCache& eval_cache, ast::SizeCache& size_cache) {
566+
const Expression& expr, const ConstraintSource& source,
567+
const TableEntry& entry, EvaluationCache& eval_cache,
568+
ast::SizeCache& size_cache) {
566569
ASSIGN_OR_RETURN(const ast::Expression* explanation,
567570
MinimalSubexpressionLeadingToEvalResult(
568571
expr, entry, eval_cache, size_cache));
569572
ASSIGN_OR_RETURN(bool truth_value,
570573
EvalToBool(*explanation, entry, &eval_cache));
574+
ASSIGN_OR_RETURN(std::string reason,
575+
QuoteSubConstraint(source, explanation->start_location(),
576+
explanation->end_location()));
571577
// Ordered for determinism when golden testing.
572578
std::string key_info = absl::StrJoin(
573579
gutils::Ordered(entry.keys), "\n", [](std::string* out, auto pair) {
574580
absl::StrAppend(out, "Key:\"", pair.first,
575581
"\" -> Value: ", EvalResultToString(pair.second));
576582
});
577-
// TODO(b/241957581): Find better representation for explanation. Consider
578-
// modifying quote function to use string in constraint info
583+
579584
return absl::StrFormat(
580585
"All entries must %ssatisfy:"
581586
"\n\n%s\n"
582-
"But your entry did%s\n"
587+
"But your entry does%s.\n"
583588
">>>Entry Info<<<\n"
584-
"Table Name:\"%s\"\n"
589+
"Table Name: \"%s\"\n"
585590
"Priority:%d\n"
586-
"Key Info\n"
587591
"%s\n",
588-
(truth_value ? "not " : ""), explanation->DebugString(),
589-
(truth_value ? "" : " not"), entry.table_name, entry.priority, key_info);
592+
(truth_value ? "not " : ""), reason, (truth_value ? "" : " not"),
593+
entry.table_name, entry.priority, key_info);
590594
}
591595
// -- Main evaluator -----------------------------------------------------------
592596

@@ -800,8 +804,8 @@ absl::StatusOr<std::string> ReasonEntryViolatesConstraint(
800804
return "";
801805
}
802806
SizeCache size_cache;
803-
return ExplainConstraintViolation(constraint, parsed_entry, eval_cache,
804-
size_cache);
807+
return ExplainConstraintViolation(constraint, table_info.constraint_source,
808+
parsed_entry, eval_cache, size_cache);
805809
}
806810

807811
} // namespace p4_constraints

0 commit comments

Comments
 (0)