PUBLIC: [p4-constraints] Refactor GetMatchFields to GetFields an AddM…

…atchFields to AddFields and extend AddFields to handle action parameters. (#122) I refactored GetMatchFields and AddMatchFields to GetFields and AddFields in ast.cc, ast.h, and ast_test.cc to better reflect how those functions are being used for ExplainReasonViolatesConstraint for both action and table constraints. I also extended AddFields to handle action parameters and updated its corresponding test. PiperOrigin-RevId: 585747337 Co-authored-by: PINS Team <[email protected]>
p4lang · Nov 28, 2023 · 7d07835 · 7d07835
1 parent 071abdc
commit 7d07835
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 17 deletions.
diff --git a/p4_constraints/ast.cc b/p4_constraints/ast.cc
@@ -205,30 +205,31 @@ bool HaveSameSource(const SourceLocation& source_location_1,
   return gutils::ProtoEqual(source_location_1, source_location_2, differ);
 }
 
-// Populates `field_set` with the fields used in `expr`.
-void AddMatchFields(const ast::Expression& expr,
-                    absl::flat_hash_set<std::string>& field_set) {
+// Populates `variable_set` with the variables used in `expr`.
+void AddVariables(const ast::Expression& expr,
+                  absl::flat_hash_set<std::string>& variable_set) {
   switch (expr.expression_case()) {
     case ast::Expression::kKey:
-      field_set.insert(expr.key());
+      variable_set.insert(expr.key());
       return;
     case ast::Expression::kActionParameter:
+      variable_set.insert(expr.action_parameter());
       return;
     case ast::Expression::kBooleanNegation:
-      AddMatchFields(expr.boolean_negation(), field_set);
+      AddVariables(expr.boolean_negation(), variable_set);
       return;
     case ast::Expression::kArithmeticNegation:
-      AddMatchFields(expr.arithmetic_negation(), field_set);
+      AddVariables(expr.arithmetic_negation(), variable_set);
       return;
     case ast::Expression::kTypeCast:
-      AddMatchFields(expr.type_cast(), field_set);
+      AddVariables(expr.type_cast(), variable_set);
       return;
     case ast::Expression::kBinaryExpression:
-      AddMatchFields(expr.binary_expression().left(), field_set);
-      AddMatchFields(expr.binary_expression().right(), field_set);
+      AddVariables(expr.binary_expression().left(), variable_set);
+      AddVariables(expr.binary_expression().right(), variable_set);
       return;
     case ast::Expression::kFieldAccess:
-      AddMatchFields(expr.field_access().expr(), field_set);
+      AddVariables(expr.field_access().expr(), variable_set);
       return;
     // Currently priority is the only metadata and that is not a key.
     case ast::Expression::kAttributeAccess:
@@ -242,9 +243,9 @@ void AddMatchFields(const ast::Expression& expr,
   }
 }
 
-absl::flat_hash_set<std::string> GetMatchFields(const ast::Expression& expr) {
+absl::flat_hash_set<std::string> GetVariables(const ast::Expression& expr) {
   absl::flat_hash_set<std::string> field_set;
-  AddMatchFields(expr, field_set);
+  AddVariables(expr, field_set);
   return field_set;
 }
 

diff --git a/p4_constraints/ast.h b/p4_constraints/ast.h
@@ -73,8 +73,10 @@ Type TypeCaseToType(Type::TypeCase type_case);
 bool HaveSameSource(const SourceLocation& source_location_1,
                     const SourceLocation& source_location_2);
 
-// Returns a set containing the `fields` present in `expr`.
-absl::flat_hash_set<std::string> GetMatchFields(const ast::Expression& expr);
+// Returns a set containing all keys and action_parameters that appear in
+// `expr`. Example use case is to extract variables from a constraint to print
+// the reason why an entry violates a constraint.
+absl::flat_hash_set<std::string> GetVariables(const ast::Expression& expr);
 
 // Cache for results of `Size`.
 using SizeCache = absl::flat_hash_map<const Expression*, int>;

diff --git a/p4_constraints/ast_test.cc b/p4_constraints/ast_test.cc
@@ -294,15 +294,17 @@ TEST(SizeTest, NoCacheOkay) {
   EXPECT_THAT(Size(expr, nullptr), IsOkAndHolds(Eq(7)));
 }
 
-TEST(AddMatchFields, ReturnsEmptyForActionParameter) {
+TEST(AddVariables, ReturnsEmptyForActionParameter) {
   Expression expr = ParseRawAst(R"pb(
     binary_expression {
       binop: GE
       left { action_parameter: "1" }
       right { action_parameter: "2" }
     }
   )pb");
-  EXPECT_THAT(GetMatchFields(expr), testing::IsEmpty());
+
+  absl::flat_hash_set<std::string> expected = {"1", "2"};
+  EXPECT_EQ(GetVariables(expr), expected);
 }
 
 }  // namespace ast

diff --git a/p4_constraints/backend/interpreter.cc b/p4_constraints/backend/interpreter.cc
@@ -604,7 +604,7 @@ absl::StatusOr<std::string> ExplainConstraintViolation(
                                       explanation->end_location()));
 
   const absl::flat_hash_set<std::string> relevant_fields =
-      ast::GetMatchFields(*explanation);
+      ast::GetVariables(*explanation);
   return std::visit(
       gutils::Overload{
           [&](const TableEntry& table_entry) -> std::string {