Reject duplicate match_kind declarations (#5656)

Signed-off-by: aeron-gh <agab0323@gmail.com>

Author: aeron-gh

frontends/p4/toP4/toP4.cpp

@@ -152,8 +152,9 @@ bool ToP4::preorder(const IR::P4Program *program) {
     for (auto a : program->objects) {
         // Check where this declaration originates
         auto sourceFileOpt = ifSystemFile(a);
-        // Errors can come from multiple files
-        if (!a->is<IR::Type_Error>() && sourceFileOpt.has_value()) {
+        // Errors and match_kinds can come from multiple files
+        if (!a->is<IR::Type_Error>() && !a->is<IR::Declaration_MatchKind>() &&
+            sourceFileOpt.has_value()) {
             /* FIXME -- when including a user header file (sourceFile !=
              * mainFile), do we want to emit an #include of it or not?  Probably
              * not when translating from P4-14, as that would create a P4-16
@@ -692,10 +693,19 @@ bool ToP4::preorder(const IR::Type_Error *d) {
 
 bool ToP4::preorder(const IR::Declaration_MatchKind *d) {
     dump(1);
+
+    const auto userMatchKinds = d->getDeclarations()
+                                    ->where([this](const IR::IDeclaration *e) {
+                                        // only print if not from a system file
+                                        return !ifSystemFile(e->getNode()).has_value();
+                                    })
+                                    ->toVector();
+    if (userMatchKinds.empty()) return false;
+
     builder.append("match_kind ");
     builder.blockStart();
     bool first = true;
-    for (auto a : *d->getDeclarations()) {
+    for (auto a : userMatchKinds) {
         if (!first) builder.append(",\n");
         dump(1, a->getNode(), 1);
         first = false;

frontends/parsers/p4/p4parser.ypp

@@ -315,8 +315,9 @@ using namespace P4;
                               parserDeclaration controlDeclaration enumDeclaration
                               typedefDeclaration packageTypeDeclaration typeDeclaration
 %type<IR::Type_Error*>      errorDeclaration
+%type<IR::Declaration_MatchKind*>  matchKindDeclaration
 %type<IR::Node*>            fragment
-%type<IR::Node*>            declaration externDeclaration matchKindDeclaration
+%type<IR::Node*>            declaration externDeclaration
 %type<IR::Type_Parser*>     parserTypeDeclaration
 %type<IR::Type_Control*>    controlTypeDeclaration
 %type<IR::IndexedVector<IR::Declaration>>  parserLocalElements controlLocalDeclarations
@@ -493,7 +494,7 @@ declaration
     | controlDeclaration      { $$ = $1; }
     | instantiation           { $$ = $1; }
     | errorDeclaration        { $$ = driver.onReadErrorDeclaration($1) ? $1 : nullptr; }
-    | matchKindDeclaration    { $$ = $1; }
+    | matchKindDeclaration    { $$ = driver.onReadMatchKindDeclaration($1) ? $1 : nullptr; }
     | functionDeclaration     { $$ = $1; }
     ;
 

frontends/parsers/parserDriver.cpp

@@ -306,6 +306,15 @@ bool P4ParserDriver::onReadErrorDeclaration(IR::Type_Error *error) {
     return false;
 }
 
+bool P4ParserDriver::onReadMatchKindDeclaration(IR::Declaration_MatchKind *matchKind) {
+    if (allMatchKinds == nullptr) {
+        allMatchKinds = matchKind;
+        return true;
+    }
+    allMatchKinds->members.append(matchKind->members);
+    return false;
+}
+
 }  // namespace P4
 
 namespace P4::V1 {

frontends/parsers/parserDriver.h

@@ -183,6 +183,11 @@ class P4ParserDriver final : public AbstractParserDriver {
     //          been combined into a previous one (and should be elided)
     bool onReadErrorDeclaration(IR::Type_Error *error);
 
+    /// Notify that the parser parsed a P4 `match_kind` declaration.
+    //  @return true if this is the first match_kind declaration, false if it has
+    //          been combined into a previous one (and should be elided)
+    bool onReadMatchKindDeclaration(IR::Declaration_MatchKind *matchKind);
+
     ////////////////////////////////////////////////////////////////////////////
     // Shared state manipulated directly by the lexer and parser.
     ////////////////////////////////////////////////////////////////////////////
@@ -217,6 +222,11 @@ class P4ParserDriver final : public AbstractParserDriver {
     /// lazily created the first time we see an `error` declaration. (This node
     /// is present in @declarations as well.)
     IR::Type_Error *allErrors = nullptr;
+
+    /// All P4 `match_kind` declarations are merged together in the node, which
+    /// is lazily created the first time we see a `match_kind` declaration. (This
+    /// node is present in @declarations as well.)
+    IR::Declaration_MatchKind *allMatchKinds = nullptr;
 };
 
 }  // namespace P4

testdata/p4_16_errors/issue5085.p4

@@ -0,0 +1,12 @@
+// It is an error to declare the same match_kind identifier multiple times,
+// even when the duplicates appear in separate match_kind declarations.
+// See https://github.com/p4lang/p4c/issues/5085
+
+match_kind {
+  foo,
+  bar
+}
+
+match_kind {
+  foo
+}

testdata/p4_16_errors_outputs/issue5085.p4-stderr

@@ -0,0 +1,7 @@
+issue5085.p4(11): [--Werror=duplicate] error: foo: Duplicates declaration foo
+  foo
+  ^^^
+issue5085.p4(6)
+  foo,
+  ^^^
+[--Werror=overlimit] error: 1 errors encountered, aborting compilation

testdata/p4_16_samples/issue5085.p4

@@ -0,0 +1,15 @@
+// User-defined match_kind identifiers spread across several declarations are
+// merged into a single match_kind node. ToP4 prints them together in one block
+// after the core.p4 include, dropping the match_kinds already defined there.
+// See https://github.com/p4lang/p4c/issues/5085
+
+#include <core.p4>
+
+match_kind {
+    first_kind,
+    second_kind
+}
+
+match_kind {
+    third_kind
+}

testdata/p4_16_samples/pipe.p4

@@ -7,11 +7,6 @@
 
 #include <core.p4>
 
-match_kind {
-    ternary,
-    exact
-}
-
 typedef bit<9> BParamType;
 struct TArg1 {
     bit<9> field1;

testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-first.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #include <pna.p4>
 
 typedef bit<48> EthernetAddress;
@@ -483,11 +488,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) {
     hdr.u0_udp.setInvalid();
     meta.tunnel_pointer = 16w0;
 }
-match_kind {
-    list,
-    range_list
-}
-
 control acl(inout headers_t hdr, inout metadata_t meta) {
     action permit() {
     }

testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-frontend.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #include <pna.p4>
 
 typedef bit<48> EthernetAddress;
@@ -313,11 +318,6 @@ control dash_deparser(packet_out packet, in headers_t hdr, in metadata_t meta, i
     }
 }
 
-match_kind {
-    list,
-    range_list
-}
-
 control dash_ingress(inout headers_t hdr, inout metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) {
     @name("dash_ingress.meta") metadata_t meta_0;
     @name("dash_ingress.meta") metadata_t meta_5;