Reject duplicate match_kind declarations

frontends/p4/toP4/toP4.cpp

@@ -152,8 +152,12 @@ 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 be declared across multiple files, so the
+        // merged node carries the source location of the first declaration (often
+        // a system file). Always visit them so any user-defined members are
+        // emitted instead of being hidden behind the #include.
+        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 +696,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/decl-matchkind-with-duplicate-fields-in-separate-declarations.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/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4-stderr

@@ -0,0 +1,7 @@
+decl-matchkind-with-duplicate-fields-in-separate-declarations.p4(11): [--Werror=duplicate] error: foo: Duplicates declaration foo
+  foo
+  ^^^
+decl-matchkind-with-duplicate-fields-in-separate-declarations.p4(6)
+  foo,
+  ^^^
+[--Werror=overlimit] error: 1 errors encountered, aborting compilation

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;

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

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #include <pna.p4>
 
 header ethernet_t {
@@ -312,11 +317,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.tmp_5") bit<32> tmp;
     @name("dash_ingress.tmp_6") bit<32> tmp_0;

testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk.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 = 0;
 }
-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-v1model-bmv2-first.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #define V1MODEL_VERSION 20180101
 #include <v1model.p4>
 
@@ -456,11 +461,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-v1model-bmv2-frontend.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #define V1MODEL_VERSION 20180101
 #include <v1model.p4>
 
@@ -314,11 +319,6 @@ control dash_deparser(packet_out packet, in headers_t hdr) {
     }
 }
 
-match_kind {
-    list,
-    range_list
-}
-
 control dash_ingress(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t standard_metadata) {
     @name("dash_ingress.meta") metadata_t meta_0;
     @name("dash_ingress.meta") metadata_t meta_5;

testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-midend.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #define V1MODEL_VERSION 20180101
 #include <v1model.p4>
 
@@ -304,11 +309,6 @@ control dash_deparser(packet_out packet, in headers_t hdr) {
     }
 }
 
-match_kind {
-    list,
-    range_list
-}
-
 control dash_ingress(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t standard_metadata) {
     @name("dash_ingress.tmp_7") bit<32> tmp;
     @name("dash_ingress.tmp_8") bit<32> tmp_0;

testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2.p4

@@ -4,6 +4,11 @@ error {
     InvalidIPv4Header
 }
 #include <core.p4>
+
+match_kind {
+    list,
+    range_list
+}
 #define V1MODEL_VERSION 20180101
 #include <v1model.p4>
 
@@ -456,11 +461,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) {
     hdr.u0_udp.setInvalid();
     meta.tunnel_pointer = 0;
 }
-match_kind {
-    list,
-    range_list
-}
-
 control acl(inout headers_t hdr, inout metadata_t meta) {
     action permit() {
     }

testdata/p4_16_samples_outputs/issue982-first.p4

@@ -1,5 +1,10 @@
 #include <core.p4>
 
+match_kind {
+    range,
+    selector
+}
+
 header clone_0_t {
     bit<16> data;
 }
@@ -95,11 +100,6 @@ struct psa_egress_output_metadata_t {
     PacketLength_t   truncate_payload_bytes;
 }
 
-match_kind {
-    range,
-    selector
-}
-
 action send_to_port(inout psa_ingress_output_metadata_t meta, in PortId_t egress_port) {
     meta.drop = false;
     meta.multicast_group = 10w0;

testdata/p4_16_samples_outputs/issue982-frontend.p4

@@ -1,5 +1,10 @@
 #include <core.p4>
 
+match_kind {
+    range,
+    selector
+}
+
 header clone_0_t {
     bit<16> data;
 }
@@ -94,11 +99,6 @@ struct psa_egress_output_metadata_t {
     PacketLength_t   truncate_payload_bytes;
 }
 
-match_kind {
-    range,
-    selector
-}
-
 extern PacketReplicationEngine {
 }
 

testdata/p4_16_samples_outputs/issue982-midend.p4

@@ -1,5 +1,10 @@
 #include <core.p4>
 
+match_kind {
+    range,
+    selector
+}
+
 header clone_0_t {
     bit<16> data;
 }
@@ -82,11 +87,6 @@ struct psa_egress_output_metadata_t {
     bit<14> truncate_payload_bytes;
 }
 
-match_kind {
-    range,
-    selector
-}
-
 extern PacketReplicationEngine {
 }