GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add `#result` for Return Refinements by rajshivu · Pull Request #145 · liquid-java/liquidjava Conversation
rcosta358
reviewed
|
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
$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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hi @rajshivu, just out of curiosity, how would you pass a Turing test? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
87ef6e6 to
9d7e2dc
Compare
|
Hi @rcosta358 , I’ve pushed a fix for this issue in last commits, |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Changing all IDs to allow for the $ will make it harder to pinpoint errors in other variables. Since the only thing we want is to allow $result as a possible var in the refinements we can add it to the place i pointed in the comments. Try the change and test it to see if it works
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| OBJECT_TYPE: | ||
| (([a-zA-Z][a-zA-Z0-9]+) ('.' [a-zA-Z][a-zA-Z0-9]*)+); | ||
| ID : '#'*[a-zA-Z_][a-zA-Z0-9_#]*; | ||
| ID : '#'*('$')? [a-zA-Z_] [a-zA-Z0-9_#]* ; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is not a great idea to change all the IDs to allow the $. We can improve it by only allowing $result and only in the places it can appear (not when a ghost or state is created for example)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hi @CatarinaGamboa , thanks for the clarification |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hi @rcosta358 @CatarinaGamboa , I’ve pushed an updated fix. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks better! Merge main into this branch so there are no conflicts and then we fix the nitpick below. Then request a new review and maybe we'll merge it
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return null; | ||
| } else { | ||
| } | ||
| // else { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
dont leave commented code
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
The branch is already up to date with main and commented code has been removed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
There's a minor conflict in CreateASTVisitor that does not allow the tests to run.
Also added some small requests.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (fi == null) | ||
| return;// null check refinement | ||
|
|
||
| if (fi.getRefReturn() == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I also faced a NullPointerException because of fi being null, which I fixed in #150 and added the same check there. However, fi.getRefReturn() never returns null. With that being said, you can remove both of these checks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Optional<Predicate> oret = rtc.getRefinementFromAnnotation(method); | ||
| Predicate ret = oret.orElse(new Predicate()); | ||
| ret = ret.substituteVariable("return", Keys.WILDCARD); | ||
| ret = ret.substituteVariable("$result", Keys.WILDCARD);// added for refinement |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary comment.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I added the null check after hitting a NullPointerException with fi (fixed in #150), and mirrored the same guard here for safety. Since fi.getRefReturn() is guaranteed to be not-null, I agree the checks are unnecessary. I removed both null checks and comment. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
LGTM. However, do you mind changing it from |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sure, no problem. I’ll update it to #result |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
$result for Return Refinements#result for Return Refinements
Requested changes must be addressed to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Changereturn to $result for method return values in refinements
There was an error while loading. Please reload this page.