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

gccrs: fix a compiler crash when path expression contains nothing #3286

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liushuyu
Copy link
Contributor

@liushuyu liushuyu commented Dec 3, 2024

gcc/rust/ChangeLog:

* ast/rust-path.h: allows error nodes to specify the source
location, so that it could be used in diagnostics later.
* parse/rust-parse-impl.h: creates the PathInExpression error node
with source location and prints proper diagnostics if the path
is empty.

gcc/testsuite/ChangeLog:

* rust/compile/paamayim-nekudotayim.rs: add a test for testing
proper error recovery logic when there is no path names.

@powerboat9
Copy link
Contributor

This should probably be handled by the parser, rather than the type checker, but good catch

@liushuyu
Copy link
Contributor Author

liushuyu commented Dec 3, 2024

This should probably be handled by the parser, rather than the type checker, but good catch

I want to handle this in the parser, but there seems to be a curious comment in the parser code (https://github.com/Rust-GCC/gccrs/pull/3286/files#diff-0ec0ef9112d5a4a50636e9fde592f348f936f51b4139796976c7ad1608f935f8R6716-R6717) that states:

      // skip after somewhere?
      // don't necessarily throw error but yeah

@powerboat9
Copy link
Contributor

Yeah, that's a bit odd. It looks like that comment was introduced by a2ea9f7e7027a9945744f5f7c126a0618a30c334 (at that point in file gcc/rust/test3/parse/rust-parse.cc) and it looks like according to https://doc.rust-lang.org/reference/paths.html#paths-in-expressions paths must have at least one segment. I'd think the comment could be removed -- @philberty @CohenArthur?

@CohenArthur
Copy link
Member

I agree with @powerboat9. I think this case should be handled in the parser, this is how rustc seems to do it at least. so I'm happy with removing the comment and handling it in the parser @liushuyu. Once this is done I think this PR is ready to go 👍

@liushuyu liushuyu force-pushed the fix-empty-path-expr branch 2 times, most recently from 8b039e3 to 658ca0f Compare December 4, 2024 18:19
gcc/rust/ChangeLog:

	* ast/rust-path.h: allows error nodes to specify the source
	location, so that it could be used in diagnostics later.
	* parse/rust-parse-impl.h: creates the PathInExpression error node
	with source location and prints proper diagnostics if the path
	is empty.

gcc/testsuite/ChangeLog:

	* rust/compile/paamayim-nekudotayim.rs: add a test for testing
	proper error recovery logic when there is no path names.
@liushuyu liushuyu force-pushed the fix-empty-path-expr branch from 658ca0f to b963d59 Compare December 4, 2024 21:56
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.

I think we need for #3295 to go through or this PR will also fail if it gets rebased. but I'm happy to merge it

Comment on lines +11641 to +11643
{
rust_error_at (path.get_locus (), "expected identifier");
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
rust_error_at (path.get_locus (), "expected identifier");
}
rust_error_at (path.get_locus (), "expected identifier");

small formatting nit :)

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