-
Notifications
You must be signed in to change notification settings - Fork 34
Add $result for Return Refinements
#145
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
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @rajshivu,
This is not really what we were looking for. Your approach replaces the variables return and _ with $result, which is incorrect and causes the tests to fail.
To allow $result to represent return values of methods, you need to modify the grammar to allow identifiers to start with $, substitute the variable $result with _ (similarly to return), and finally add some tests to check if your implementation is correct.
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.
Also, if you have any suggestions other than $result, which is not very idiomatic for Java, feel free to let us know!
|
Hi @rcosta358 , Based on your feedback, I understand the correct direction is to:
I’ll update the implementation accordingly and push a revised PR. Thanks again for the guidance, this was very helpful. |
|
Fixed the handling of method return values in refinements: Added a regression test ResultRefinementRegression.java to verify $result works correctly in refinements. Hope This change resolves the issue where return was incorrectly treated as a variable and ensures $result can be safely used in refinement expressions. |
rcosta358
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.
Hey @rajshivu,
Almost there. Your implementation is not quite correct yet. Currently what you're doing is replacing all return and $result variables with _ in the CreateASTVisitor. That way, the following code passes, when it shouldn't:
void test() {
@Refinement("$result > 0") // $result is replaced with `_`, which refers to `x`, so it passes
int x = 1;
}Actually, $result or return should only be allowed on refinements that target methods, such as:
@Refinement("$result > 0")
int test() {
return 1;
}So, we don't actually want to modify the CreateASTVisitor. Instead, we want to modify the MethodsFunctionsChecker. More specifically, the handleFunctionRefinements, and add an additional case for $result, similarly to return:
private Predicate handleFunctionRefinements(RefinedFunction f, CtElement method, List<CtParameter<?>> params) throws LJError {
// ...
ret = ret.substituteVariable("return", Keys.WILDCARD);
ret = ret.substituteVariable("$result", Keys.WILDCARD); // ADD THIS
f.setRefReturn(ret);
// ...
}Finally, please add tests to liquidjava-example/src/main/java/testSuite, not liquidjava-example/src/main/java/testMultiple. For tests that should pass, they must include "Correct" in their file name, whereas for tests that should fail, they should include "Error", along with the expected error in the first line of the file as a comment. To run the tests locally, you can run mvn test.
liquidjava-verifier/src/main/java/liquidjava/rj_language/visitors/CreateASTVisitor.java
Show resolved
Hide resolved
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.
You should also not modify this, since you should include your tests in testSuite and not testMultiple.
$result for Return Refinements
|
Hi @rcosta358 , Thanks for the clarification! Got it — I’ll revert the changes in CreateASTVisitor and TestMultiple, and instead implement $result handling in MethodsFunctionsChecker.handleFunctionRefinements as suggested. I’ll also move the regression tests to testSuite with the proper naming convention. |
|
Hi @rajshivu, just out of curiosity, how would you pass a Turing test? |
|
Hello @alcides , you caught me, I used a bit of AI to help polish my explanation and easy for understand, but I promise there's a real person behind the keyboard. |
87ef6e6 to
9d7e2dc
Compare
Updated CreateASTVisitor to map refinement variables '_' and 'return' to '$result' to prevent conflicts.

Verified by manual test in ReturnRefinementTest.java.