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

Add proper error message #3149

Merged
merged 1 commit into from
Feb 9, 2023
Merged

Add proper error message #3149

merged 1 commit into from
Feb 9, 2023

Conversation

radumereuta
Copy link
Contributor

AddSortInjections would fail with a stack trace if it failed to find a label in the main module.

The message now reads:

[Error] Compiler: Could not find productions for KApply with label _=__TEST-SYNTAX_Stuff_LValue_RValue in module TEST
	Source(/home/radu/work/test/./2.test)
	Location(1,1,1,6)
	1 |	a = b
	  .	^~~~~

The change is pretty simple. I don't think it warrants a test. But if you think it's worth it, I can also add a test.

AddSortInjections  would fail with a stack trace if it failed to find a label in the main module.
@radumereuta radumereuta requested a review from Baltoli February 9, 2023 17:56
@radumereuta radumereuta self-assigned this Feb 9, 2023
@radumereuta
Copy link
Contributor Author

This was found by @virgil-serbanuta.
He forgot to import the syntax module into the main module.
Alternatively, we may want to allow for that. I'm not sure how deep the changes to the AddSortInjections step would be.

@dwightguth
Copy link
Collaborator

I mean, if the production isn't in the main module, then it's not part of the definition that the compiler sees at runtime, so of course the compiler won't be able to compute sort injections for it. The only way you might be able to do so would be if you added the sorts and subsorts from the syntax module into the sort graph used by the algorithm, but that wouldn't be sound.

Adding a friendly error message is definitely the correct solution here.

@radumereuta
Copy link
Contributor Author

Is the error message good enough, then?
Can you approve?

@rv-jenkins rv-jenkins merged commit 06ec44c into develop Feb 9, 2023
@rv-jenkins rv-jenkins deleted the fixErrMsg branch February 9, 2023 22:02
@Baltoli Baltoli mentioned this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants