Skip to content

Conversation

@chachacollins
Copy link

The topleveldef parser rule was missing error handling for most parsing paths, so users would get unhelpful error messages when they had syntax errors in imports, extern etc. I have added error handling for all the paths so that problem should be fixed by this PR.

@mell-o-tron
Copy link
Collaborator

Great! Thank you for this. I tried compiling it, and there are a few warning regarding productions leading to error that are never produced:

File "lib/parser.mly", line 273, characters 4-9:
Warning: production expr_list -> error is never reduced.
File "<standard.mly>", line 143, characters 19-19:
Warning: production loption(separated_nonempty_list(Semicolon,topleveldef)) -> is never reduced.
File "lib/parser.mly", line 302, characters 4-9:
Warning: production perkdeclorfun_list -> error is never reduced.
File "lib/parser.mly", line 290, characters 4-9:
Warning: production perkdeforfun_list -> error is never reduced.
File "lib/parser.mly", line 284, characters 4-9:
Warning: production perktype_list -> error is never reduced.
File "lib/parser.mly", line 296, characters 4-9:
Warning: production perkvardesc_list -> error is never reduced.
Warning: in total, 6 productions are never reduced.

If you could address this and maybe add a few tests for these error scenarios it would be much appreciated! If you want to do this, the non-ok tests should go in test/errorexec/ (or failed exec idk, if you find a better name take the liberty of choosing it).

@chachacollins
Copy link
Author

Didn't catch that mb. It must have gotten lost in the wall of warnings. I assume you intentionally ignore the precedence warnings or should I fix those too

@mell-o-tron
Copy link
Collaborator

Great, thanks! If you want you can also work on the precedence warnings in a separate PR, if that's your jam. We're certainly thinking of dealing with them eventually, but we fear it might be a bit of a monumental effort...

For this PR, if you can just add a few failing tests so that I can verify your changes work as intended, I'll merge it :)

@Alex23087 Alex23087 force-pushed the main branch 11 times, most recently from c97cb73 to 6657c90 Compare July 16, 2025 23:25
@Alex23087
Copy link
Owner

Most importantly, for the precedence warnings, the exact syntax of some things is still not entirely crystallised, so we are deferring dealing with that to some later time where it won't be changing as frequently as it is.

By the way, there is now a folder and a make target for tests that are expected to fail. Unfortunately, due to how we are dealing with them right now, we are not checking that the test fails in the expected way, i.e., we only check that the program fails, but not what is the error message that the compiler spits out. This is subject to changes in the future, if we can figure out a nice way to do it that won't break tests just because we changed the error message. Some error identifier could be a good solution.

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