Shadowing a control parameter within an apply block is disallowed

[jaehyun1ee]

issue2544_shadowing1.p4 defines a local variable h, of the same name with the control parameter h, but this is disallowed by the frontend.

control ingress(inout Headers h) {
    apply {
        Headers h = h;
    }
}

// issue2544_shadowing1.p4(25): [--Werror=shadow] error: declaration of 'h' shadows a parameter 'h'
//         Headers h = h;
//         ^^^^^^^^^^^^^
// issue2544_shadowing1.p4(23)
// control ingress(inout Headers h) {
//                              ^

inout Headers h and Headers h = h reside in different blocks, so there should be no problem in disambiguating the right-hand side of the assignment h (coming from the control parameter) from the local h.

I suppose #2589 was a fix for this issue, but is there a reason for the p4c frontend to reject this program?

[ChrisDodd]

By normal (C/C++) scoping rules, one would expect h in the initializer to refer to the local, not the param, so this is initializing the local with itself (an uninitialized value), which should be either an error or garbage. The P4 spec is not very clear, but implies that the local h is in scope for the entire apply block; its just an error to use it before it is declared, which is maybe what this error is?

[jaehyun1ee]

Hmm, to clarify if I understood correctly, does it mean that

control ingress(inout Headers h /* A */) {
    apply {
        Headers h /* B */ = h /* C */;
    }
}

C is referring to B, not A? I thought that C refers to A.

[ChrisDodd]

C is referring to B, not A? I thought that C refers to A.

Yes, I think C unambiguously refers to B here, not A.

More problematic is something like:

control ingress(inout Headers h /* A */) {
    apply {
        Headers x = h /* C */;
        Headers h; /* B */
    }
}

here, does C refer to A or B? Yes, it is clearly before B is defined, but the spec doesn't say that B is ignored for name lookup, just that referring to it in this way is an error.

Note the spec's statement " all uses of a symbol must follow the symbol’s declaration. (This is a departure from P4_14, which allows declarations in any order. This requirement significantly simplifies the implementation of compilers for P4_16)" which is patently false due to this sort of problem -- it adds a whole bunch of implementation difficulty to name resolution lookup and doesn't really help parsing.

[vlstill]

Assignments like Headers h = h; can be quite confusing, so it think it is better if they are just plain forbidden. We might want to add that to the spec.

Note that in C++ (at least from my testing) shadowing a function argument is disallowed, but only when the shadowing happens in the top-level block -- if it happens in a sub-block, it is allowed, but the RHS already refers to the new name. In the case of use-before-define, C++ does not take the "to be declared" variable into account, which is consistent with the wannabe-single-pass parsing relict from C. C++ does have a case where name resolution works as you suggested though -- in the class constructor's initialization section, Foo(int x /* A */) : x/* B */(x /* C */) {} B would refer to the class data member, and C would refer to A. I can't remember any other place where in an imperative language assignment would work like that.

[jaehyun1ee]

control ingress(inout Headers h /* A */) {
    /* control-local layer */
    apply {
        Headers h /* B */ = h /* C */;
    }
}

It does make sense to reject this program if A and B live in the same scoping level. But I thought A and B live in different levels because they are separated by a block (curly braces { and }), leaving a control-local layer in between.

One piece of evidence is that, below program is accepted by the p4c frontend, yet with shadow warnings.

control ingress(inout Headers h) {
    Headers h = h;
    apply {
        Headers h = h;
    }
}

scoping.p4(24): [--Wwarn=shadow] warning: 'h' shadows 'h'
    Headers h = h;
    ^^^^^^^^^^^^^
scoping.p4(23)
control ingress(inout Headers h) {
                              ^
scoping.p4(26): [--Wwarn=shadow] warning: 'h' shadows 'h'
        Headers h = h;
        ^^^^^^^^^^^^^
scoping.p4(24)
    Headers h = h;
    ^^^^^^^^^^^^^

So, it seems like control parameters and local variables in an apply block are not in the same scoping level, because there is a control-local layer in between.

If we were to write the program as below (with abuse of P4 syntax), then A and B indeed lies in the same scope. However, I believe that the presence of a control-local layer differentiates the two.

control ingress {
    apply(inout Headers h /* A */) {
        Headers h /* B */ = h /* C */;
    }
}

[fruffy]

This warrants a P4-language spec group discussion.

[jonathan-dilorenzo]

Discussed in P4 LDWG. This is quite thorny and involves many separate issues:

1. I forked off https://github.com/p4lang/p4-spec/issues/1372 to discuss the issue of using application parameters in control block initialization.
2. @ChrisDodd and @ajwalton, would you like to fork off an issue about the meaning of statements like X a = a?
3. @KunJeong and/or @jaehyun1ee, could you fork off an issue about:

control ingress(inout Headers h) {
    Headers h = [some valid value other than h];
    ...
}

which I believe we think should probably rejected on the basis of being in the same name space?

4. With that, we come to the potential shadowing issue mentioned at the start of the issue:

control ingress(inout Headers h) {
    apply {
        Headers h = [some valid value];
    }
}

which seems like it should be accepted as shadowing something with a broader scope (the whole control block) in a smaller scope (the apply block), possibly with a warning?

@rcgoodfellow, @fruffy, and @jafingerhut for visibility too.