-
Notifications
You must be signed in to change notification settings - Fork 33
Fix Alias & Ghost Argument Mismatch #127
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
Conversation
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge previous PR and rebase pls. These are too many files to review 😅
|
Sure thing! Let me just finish up #126. |
- Add ArgumentMismatchError - Check argument count and types of aliases - Add not found error for aliases as well - Add locations to LJErrors after catching them in the places they don't include positions
- Validate ghost invocations instead of relying on Z3 output - Throw LJErrors in AST expressions
bc9c458 to
ea9d845
Compare
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments
liquidjava-verifier/src/main/java/liquidjava/rj_language/ast/Expression.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // no overload matched, use the first candidate to throw the error | ||
| GhostFunction g = candidates.get(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we provide the hints in here instead of creating an error with the first candidate? Like "gh(int) not found. Maybe you meant gh(double), gh(int, double)"
Similar to what you did on the warning for external method not found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! I'll do that in another PR with more cases.
| * | ||
| * @throws LJError | ||
| */ | ||
| public void validateGhostInvocations(Context ctx, Factory f) throws LJError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method is too long. check the suggestions i left, it might improve. If not, we might need to split this into smaller bits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This PR fixes the problem mentioned in #126, where we were getting a not found error instead of an argument mismatch error on wrong alias invocations, because we were not validating them beforehand. Also, we were previously relying on the Z3 output to get ghost invocation errors. Instead, we now also validate ghost calls before sending them to the SMT solver. For both ghosts and aliases, in either wrong argument count or types, the
ArgumentMismatchErrorwas introduced.