Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify instantiation of objects with abstract methods #1355

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jaehyun1ee
Copy link
Contributor

Following #1346, this PR adds more rules to the semantics of abstract method initialization.

First, it restricts that the abstract method being implemented, must have the same parameter names as declared in the object declaration.

This is to (i) disambiguate what abstract method is being implemented, and (ii) provide a clear interface for users.

For example, one may define an extern object with overloaded abstract methods, as follows.

extern Virtual {
    Virtual();
    abstract bit<32> g(inout bit<32> x); // (1)
    abstract bit<32> g(inout bit<32> y); // (2)
}

And in the instantiation site,

Virtual() virt = {
    bit<32> g(inout bit<32> z) { return x - 1; } // (a)
    bit<32> g(inout bit<32> w) { return x + 1; } // (b)
};

Without a restriction on matching parameter names, it is unclear whether (a) implements (1) or (2), and (b) implements (1) or (2).

This can further cause confusion when invoking the abstract methods. P4 defines overload resolution via parameter names. If we were to allow such a program, then a user would have to invoke virt.g(z = 32w42); and vir.g(w = 32w42);, while the extern object declaration suggests that it should be callable by virt.g(x = 32w42); or virt.g(y = 32w42); instead. i.e., there is a gap between the interface defined in the extern object declaration and what is actually implemented.

The consequences of adding this restriction to the spec would be: virtual.p4, issue2175-1.p4, issue2175-3.p4, and issue2175-4.p4 in the p4c test suite would have to be rejected by the frontend.

Second, it adds the initializer block to the scope of abstract method.

When implementing an abstract method, the scope is limited to the supplied arguments or values that are in the top-level scope.

But this overlooks the case where an instantiation declaration is used inside an object initializer block. Instantiation declaration may be used inside an object initializer block, which is used by program virtual2.p4.

objDeclaration
    : functionDeclaration
    | instantiation
    ;
extern Virtual {
    Virtual();
    void run(in bit<16> ix);
    @synchronous(run) abstract bit<16> f(in bit<16> ix);
}
extern State {
    State(int<16> size);
    bit<16> get(in bit<16> index);
}
...
Virtual() cntr = {
    State(16s1024) state;
    bit<16> f(in bit<16> ix) {
        return state.get(ix);
    }
};

Notice that f uses state in its implementation.

Strictly applying what is written in the spec, the program should not be accepted, since f refers to state that is neither in the top-level scope or the parameter list. i.e., the current spec syntactically allows instantiation inside object initializer but semantically disallows any way to refer to the instantiated instance.

So this PR adds the object initializer block to the scope as well.

@jaehyun1ee jaehyun1ee added the ldwg-discussion Plan to discuss at next LDWG label Dec 4, 2024
@ChrisDodd
Copy link
Contributor

ChrisDodd commented Dec 4, 2024

I think it should follow roughly the same rules as overload resolution -- so if the functionDeclaration unambiguously matches one in the extern (ignoring argument names), that should be fine. If the argument names match exactly to resolve any ambiguity that should be fine too, but if there are multiple abstract functions with the same function name that differ only in argument names, and the names don't match, that should be an error.

@jaehyun1ee
Copy link
Contributor Author

I think it is a good idea. Just to clarify, below is my mental model of how it should work.

Overload resolution in P4 is based on argument arity and, if arguments are named, then we use the names to disambiguate. So the strategy would be:

(i) First lookup an abstract method with method name and arity.
(ii) If (i) yields a single method, then matching is done.
(iii) If (i) yields multiple methods, lookup again with method name, parameter names, and arity.
(iv) If (iii) yields a single method, then matching is done.
(v) If (iii) still yields multiple methods, we emit an error due to ambiguity.

extern Virtual() {
   Virtual();
   abstract bit<32> f(bit<32> a, bit<32> b); // (1)
   abstract bit<32> f(bit<32> c, bit<32> d); // (2)
   abstract bit<32> f(bit<32> a, bit<32> b, bit<32> c); // (3)
}
...
Virtual() cntr = {
   bit<32> f(bit<32> c, bit<32> d) { // (a)
        return 32w42;
    }
    bit<32> f(bit<32> d, bit<32> e, bit<32> f) { // (b)
        return 32w42;
    }
   bit<32> f(bit<32> a, bit<32> b) { // (c)
        return 32w42;
    }
};

This would match (1) to (c), (2) to (a), and (3) to (b).

But at the same time, I also think that restricting the abstract method implementations to use the same parameter names as they were declared, helps maintain a consistent call interface.

Referring to the example above, after instantiating the object, we would have to call cntr.f(d = 32w1, e = 32w2, f = 32w3); (if using named arguments), which is different from what was proposed in the object declaration, cntr.f(a = 32w1, b = 32w2, c = 32w3);.

@ChrisDodd
Copy link
Contributor

Hmm. I think you're probably right that having different argument names should at least be a warning, as it makes calls with named arguments confusing as, depending on context, you might need to use either set of names -- calling cntr directly would use the names as you've done, while passing cntr as a directionless argument to a control with a Virtual v argument would require a call like v.f(a = 1, b = 2, c = 3); in that control.

@rcgoodfellow
Copy link
Collaborator

LDWG discussion:

For accessing abstract methods, the changes proposed to codify what is already in the P4C compiler and should go in.

For the other bit, abstract methods should not be overloaded in a way where the only distinguishing factor is parameter names. Programmers should be free to choose their own names for parameters in implementations of overloaded methods. This is at odds with parameter names being the only distinguishing feature between abstract overloads.

Copy link
Collaborator

@jafingerhut jafingerhut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, although I have two small suggestions.

@@ -2765,7 +2765,7 @@ either by the number of arguments or by the names of the arguments,
when calls are specifying argument names. Argument type information
is not used in disambiguating calls.

Notice that overloading of parsers, controls, or packages is not allowed:
Notice that overloading of abstract methods, parsers, controls, or packages is not allowed:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just point out this forbids overloading of abstract methods completely, i.e. also by the number of arguments. I am OK with that, but I wanted to highlight that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was intended because allowing overload via only the arity, and only for abstract methods, would be too complex. But in some future, if someone finds such a feature useful, we might consider lift this restriction then.

p4-16/spec/P4-16-spec.adoc Outdated Show resolved Hide resolved
p4-16/spec/P4-16-spec.adoc Outdated Show resolved Hide resolved
Signed-off-by: Jaehyun Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ldwg-discussion Plan to discuss at next LDWG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants