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

Handle async functions in traits #2786

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Conversation

braw-lee
Copy link
Contributor

@braw-lee braw-lee commented Jan 3, 2024

Fixes #2785
I opened this pull request because I have some doubts.
So I created a switch-case for async keyword when reading trait items. I generate an error (I know its not the right way) and return nullptr.
The returning nullptr generates additional errors that results in the test case failing.
The correct way to generate error would be to add check in ASTValidation::visit (AST::TraitFunctionDecl &decl) function but it won't work right now because we never made the AST node.
To solve this problem I think we need to implement parse_trait_async() and to implement this function we need to make an AST node AST::TraitItemAsync.
But this node will be pretty much useless as async isn't allowed inside traits so I don't know if this is the right way to approach this problem
@CohenArthur Is there any other way to solve this problem, I am sorry if I failed to reach an obvious solution, lemme know.

@P-E-P
Copy link
Member

P-E-P commented Jan 4, 2024

function but it won't work right now because we never made the AST node. To solve this problem I think we need to implement parse_trait_async() and to implement this function we need to make an AST node AST::TraitItemAsync.

We usually only create a node for a given syntax tree component, the async keyword gives us information but cannot have any "child", it contains the asyncness information. Function declarations store the asyncness in function qualifiers.

Even though async should probably be rejected in the ast validation stage, the compiler struggles at an earlier stage (during parsing) per your example. It expects some token and find another, it already covered all possible cases and expects a macro call (!).

This means we should modify the parsing stage to accept the async token, store the asyncness information in the function qualifiers and reject an async function declaration in traits at a later stage during ast validation probably around here like it has been done with const functions.

Hopefully this is clearer ?

@braw-lee
Copy link
Contributor Author

braw-lee commented Jan 6, 2024

yes, this is clearer. Thanks for help.

@braw-lee braw-lee changed the title Handle async function declarations in traits Handle async functions in traits Jan 6, 2024
@braw-lee braw-lee marked this pull request as ready for review January 6, 2024 02:29
Fixes Rust-GCC#2785

gcc/rust/ChangeLog:

	* checks/errors/rust-ast-validation.cc (ASTValidation::visit):
	Added check for `async` functions inside trait.
	* parse/rust-parse-impl.h (Parser::parse_trait_item):
	Added switch-case for ASYNC token.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-2785.rs: New test.

Signed-off-by: Kushal Pal <[email protected]>
@braw-lee
Copy link
Contributor Author

@P-E-P is this PR good to merge?

@P-E-P P-E-P requested review from P-E-P and CohenArthur January 16, 2024 09:57
@P-E-P P-E-P added this to the GCC 14.1 release milestone Jan 16, 2024
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

looks great to me! thanks!

gcc/rust/checks/errors/rust-ast-validation.cc Outdated Show resolved Hide resolved
gcc/rust/ChangeLog:

	* checks/errors/rust-ast-validation.cc (ASTValidation::visit):
	Enclose const in single quotes.

gcc/testsuite/ChangeLog:

	* rust/compile/const_trait_fn.rs:
	Enclose const in single quotes.

Signed-off-by: Kushal Pal <[email protected]>
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

thanks a lot!

@CohenArthur CohenArthur added this pull request to the merge queue Jan 16, 2024
Merged via the queue into Rust-GCC:master with commit 6d85a80 Jan 16, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Unhandled async for trait items
3 participants