iets3.opensource icon indicating copy to clipboard operation
iets3.opensource copied to clipboard

Update interpreter semantics of && and || logic operators to be consistent with other logic ops

Open HeikoBecker opened this issue 3 years ago • 4 comments

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 avatar Feb 22 '23 09:02 HeikoBecker

@HeikoBecker the target branch maintenance/mps20211 is no longer under maintenance. Do you mind updating it to maintenance/mps20213?

arimer avatar Mar 07 '24 09:03 arimer

@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.

HeikoBecker avatar Mar 11 '24 09:03 HeikoBecker

The PR now targets the more recent mps 2022.2 maintenance branch.

HeikoBecker avatar Apr 25 '24 06:04 HeikoBecker

@HeikoBecker I've updated the build script and ran some panding migrations. Hope this is OK for you.

arimer avatar May 13 '24 14:05 arimer

Just some minor findings we should clarify/address before merging:

  1. 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.

  1. 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

  1. 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.

  1. 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.

  1. also the execution of NixHandlerCanShortCircuit is using some deprecated API

Fixed in latest commit.

HeikoBecker avatar Jun 05 '24 12:06 HeikoBecker

I have also rebased and the PR should be ready now.

HeikoBecker avatar Jun 05 '24 12:06 HeikoBecker