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

Do not allow arguments with type table, control, etc #3901

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

mihaibudiu
Copy link
Contributor

Fixes #3808

Signed-off-by: Mihai Budiu <[email protected]>
@@ -3590,6 +3596,17 @@ const IR::Node *TypeInference::postorder(IR::MethodCallExpression *expression) {
param = *paramIt;
}

if (param->type->is<IR::Type_Dontcare>())
typeError("%1%: Could not infer a type for parameter %2%", arg, param);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
typeError("%1%: Could not infer a type for parameter %2%", arg, param);
typeError("%1%: Could not infer a type for parameter %2% because the parameter's type is \"Type_Dontcare\"", arg, param);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type_Dontcare is really an internal class name of the compiler which does not mean much to the user.
But we can say "_".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe instead of Type_Dontcare calling it a don't-care type can work? _ also looks confusing.

Signed-off-by: Mihai Budiu <[email protected]>
if (param->type->is<IR::Type_Dontcare>())
typeError(
"%1%: Could not infer a type for parameter %2% "
"(inferred type is don't care '_')",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this test here be for arg->type rather than param->type? arg->type is the type from the argument expression (what we're inferring) while param->type will be the declared type of the parameter. As is, this would seem to flag any time we instantiate something a with a don't care and then actually infer something for it, which is very much the opposite of what the message is saying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really about the instantiation of a generic extern, where the generic types are used to specify parameters to some of the methods. You are allowed to use _ (don't care) for a type argument when you instantiate an extern, but then you are not supposed to call any of the methods that depend on that type. Here we detect the case when after type unification we still don't know what the parameter type is (it only unifies with _).

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be ok to use _ as a type argument if type inferencing can infer what the type should be based on the use. As it is, it will ALWAYS flag an error using _ as a type argument.

if (param->type->is<IR::Type_Table>() || param->type->is<IR::Type_Action>() ||
param->type->is<IR::Type_Control>() || param->type->is<IR::Type_Package>() ||
param->type->is<IR::Type_Parser>())
typeError("%1%: argument cannot have type %2%", arg, param->type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly here, should test arg->type instead of (as well as?) param->type

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.

Compiler bug with don'tcare type argument and table
4 participants