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

Emit a reasonable error if someone uses type in place where expression is exprected #4411

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

vlstill
Copy link
Contributor

@vlstill vlstill commented Feb 12, 2024

Found by fuzzy testing. Fuzzer created an annotation that contained something like val=4<H>2 where H is a type variable. Interestingly, I was not able to reproduce this with p4test, I am getting parsing errors there (both when trying to use type name in annotation and in expression in a control). It seems like the only place where this can trigger is in an annotation on the thing that is parametrized by the type variable. The annotation is seemingly (and for parser) before the variable declaration, but findDeclaration finds it, probably since the declaration is semantically inside the object that introduces the variable. This is a very niche case, but still it should not hit a BUG as it did before (type-in-expr.p4(4): Could not find type of <Type_Var>(201) H/7 H/7).

@vlstill vlstill self-assigned this Feb 12, 2024
@vlstill vlstill marked this pull request as ready for review February 12, 2024 09:34
@fruffy
Copy link
Collaborator

fruffy commented Feb 12, 2024

What is the error you get when you try this within in the parser? Or as default argument for a parameter.

@vlstill
Copy link
Contributor Author

vlstill commented Feb 12, 2024

@fruffy

What is the error you get when you try this within in the parser? Or as default argument for a parameter.

In parser state, for a parser parameterized by H:

1709: Error compiling
1709: type-in-expr.p4(13):syntax error, unexpected ;, expecting (
1709:         var = buf > H;
1709:                      ^

In a default argument in a package:

1709: type-in-expr.p4(13):syntax error, unexpected >, expecting (
1709:     ctrl<H> val = ctrl(H >
1709:                          ^

Frankly, I'm surprised how much does the P4 parser take the context into account (i.e. that H is a type variable in these locations)!

P4 code:

parser pars<H>(in bit<16> buf, out H hdrs) {
    bool var;
    state start {
        var = buf > H; 
    }
}

control ctrl<H>(bool val);

package pkg<H>(
    ctrl<H> val = ctrl(H > 3)
);

I could commit those as tests too if you want (they would be separate files thought as the parsing errors cause the compiler to stop right there).

@fruffy
Copy link
Collaborator

fruffy commented Feb 12, 2024

Okay, was just wondering if this error is only reproducible in actions. It can't hurt to add these tests for future front ends.

@@ -3052,6 +3052,9 @@ const IR::Node *TypeInference::postorder(IR::PathExpression *expression) {
if (type != nullptr)
// may be nullptr because typechecking may have failed
type = cloneWithFreshTypeVariables(type->to<IR::Type_MethodBase>());
} else if (decl->is<IR::Type_Declaration>()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering whether this might not be too restrictive. On the other hand a PathExpression reference to a Type_Declaration should probably parsed as Type_Name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly my thought. And a PathExpression is an expression, so it can be used to build more complex expressions. Having a type there directly does not make much sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any name that is in-scope as a type will be lexed as a TYPE_IDENTIFIER and recognized as a typeName in the parser, so should create a Type_Name rather than a Path. The exception is when the type is used before its declaration, which (I think) can only happen with type parameters.

@@ -3052,6 +3052,9 @@ const IR::Node *TypeInference::postorder(IR::PathExpression *expression) {
if (type != nullptr)
// may be nullptr because typechecking may have failed
type = cloneWithFreshTypeVariables(type->to<IR::Type_MethodBase>());
} else if (decl->is<IR::Type_Declaration>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any name that is in-scope as a type will be lexed as a TYPE_IDENTIFIER and recognized as a typeName in the parser, so should create a Type_Name rather than a Path. The exception is when the type is used before its declaration, which (I think) can only happen with type parameters.

@vlstill vlstill enabled auto-merge (squash) February 13, 2024 07:10
@vlstill vlstill merged commit fce82c0 into p4lang:main Feb 13, 2024
16 checks passed
@vlstill vlstill deleted the type-checking-type-expr branch February 13, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants