Duplicate annotations are not diagnosed

[asl]

testdata/p4_16_samples/action_profile_max_group_size_annotation.p4 contains the following code:

...
    table indirect {
        key = { }
        actions = { drop; NoAction; }
        const default_action = NoAction();
        // For an action profile without an action selector, the
        // `max_group_size`, `selector_size_semantics`, and `max_member_weight`
        // annotations are meaningless and should result in a warning.
        @name("ap") @max_group_size(200)
        @selector_size_semantics("sum_of_weights") @max_member_weight(4000)
        implementation = action_profile(32w128);
    }

    table indirect_ws {
        key = { meta.hash1 : selector; }
        actions = { drop; NoAction; }
        const default_action = NoAction();
        @name("ap_ws") @max_group_size(200)
        // For an action profile with an action selector using the `sum_of_weights`
        // size semantics, the `max_member_weight` annotation is meaningless and
        // should result in a warning.
        @name("ap") @max_group_size(200)
        @selector_size_semantics("sum_of_weights") @max_member_weight(4000)
        implementation = action_selector(HashAlgorithm.identity, 32w1024, 32w10);
    }

Note that indirect_ws has two @name and two @max_group_size annotations specified. Likely just a copy-paste from table indirect. Still, the duplicate annotations are not diagnosed, however, they are looked in p4c by name, so arbitrary results might be returned depending on the lookup order. While this certainly benign for max_member_weight, the @name clashes could lead to unexpected side effects (here fortunately, first the correct name was defined and lookup normally returns first annotations having the given name).

[jafingerhut]

Some of the discussion on this older p4-spec issue might be relevant, but I have not re-digested it all to come up with a summary yet: p4lang/p4-spec#715

[jafingerhut]

I agree that we should consider it a p4c bug if the behavior of having multiple annotations with the same annotation name on the same P4 object is unpredictable.

While one alternative is to make this situation a compile-time error, it does seem worth a discussion in the P4 LDWG and P4 API work group meetings whether there is a good use case for such annotations, and if so, why.

Pinging several folks for their thoughts: @smolkaj @chrispsommers @jonathan-dilorenzo

[jafingerhut]

Here is a section of the P4Runtime specification that shows multiple @pkginfo and multiple @platform_property annotations on the same P4 object: https://p4.org/p4-spec/docs/p4runtime-spec-working-draft-html-version.html#sec-annotating-p4-code-with-pkginfo

If there are people who currently actively write and use such P4 programs, it seems less like a good idea to make it a compile-time error for there ever to be two annotations with the same annotation name on the same P4 object.

These possibilities come to mind, but I do not know which statements made below are true:

(a) The example in the P4Runtime API spec is obsolete, and no one wants to do this kind of thing. All stakeholders agree that making this situation a compile-time error is acceptable.
(b) The example in the spec is in current active use by one or more stakeholders. A warning might be acceptable, or perhaps a warning except for a limited list of annotations like @pkginfo and @platform_property where no one wants a warning for multiple annotations with that annotation name on the same P4 object.

[ChrisDodd]

It should probably be up to the target/arch as to whether duplicate annotations are allowed or should be a warning or should be an error. We could perhaps adds something to ParseAnnotations to allow/force backends to specify what to do for each specific annotation.

[asl]

(a) The example in the P4Runtime API spec is obsolete, and no one wants to do this kind of thing. All stakeholders agree that making this situation a compile-time error is acceptable. (b) The example in the spec is in current active use by one or more stakeholders. A warning might be acceptable, or perhaps a warning except for a limited list of annotations like @pkginfo and @platform_property where no one wants a warning for multiple annotations with that annotation name on the same P4 object.

I guess this effectively boils down to the question if two annotations of the same name could be merged together in a well-defined way. My view would be that this is only possible for dictionary-like annotations that take pairs of key / values. In such case We'd just combine two dictionaries together. Still, the question is whether duplicate key would override the old value or cause some further merging.

While we can say that this is for target to define, I'd probably respond, that not always. Annotations are used by frontend / midend, so at least some annotations that are defined in the language spec should have some well-defined behavior. Is there any sane target that would allow multiple @name annotations?