Update interpreter semantics of && and || logic operators to be consistent with other logic ops
As described in #651 ,the Nix semantics of && and || are inconsistent with those of the other logic operators == and !=. This PR tries to address this issue by changing their semantics.
The major changes are in ExprSimpleTypesInterpreter (node url).
The PR also adds tests for the NixSupport of these operators to the nix testcase (node url).
These tests mirror the tests described in the logicaloperators testcase.
Before the PR, the code for && and || did not consistently return empty , whereas both == and != always evaluate to empty if either argument is a NixValue. This PR makes the behavior consistent.
The PR further adds a testsolution test.nixHandler.expr.os which ensures that it is also possible to implement short-circuit evaluation with a NixSupport implementation.
Slack discussion for reference: https://itemis.slack.com/archives/CCHR0PVHD/p1676362663689259
@HeikoBecker the target branch maintenance/mps20211 is no longer under maintenance. Do you mind updating it to maintenance/mps20213?
@HeikoBecker the target branch maintenance/mps20211 is no longer under maintenance. Do you mind updating it to
maintenance/mps20213?
Thanks for noticing. I have rebased the branch correctly and updated the target branch.
We can properly review this now.
The PR now targets the more recent mps 2022.2 maintenance branch.
@HeikoBecker I've updated the build script and ran some panding migrations. Hope this is OK for you.
Just some minor findings we should clarify/address before merging:
- TestNixHandlerExtension and TestNixHandler are shown in the diff, although the model seem not to be present. Could you crosscheck if it was removed properly?
I could not find any references locally. They should be gone now.
- org.iets3.core.expr.shortcircuit is an empty langauge containing only an extension to the extension point nixHandler. Do we really need a new lanague for it or can we place the code from the plugin model somewhere else?
Plugin was moved to a model alongside the tests
- NixSupport.get(index) and NixSupport.getReplaced(index) are both public and one is calling the other. Do we really need both methods being public?
I cannot say this for sure since both methods were not introduced by me. I suggest we keep this to a separate PR.
- I don't think we need this test here nixHandler_logandbecause both cases are already covered here logicalgroups do we have any difference in the nix declaration?
Agreed. I removed the test.
- also the execution of NixHandlerCanShortCircuit is using some deprecated API
Fixed in latest commit.
I have also rebased and the PR should be ready now.