rusty icon indicating copy to clipboard operation
rusty copied to clipboard

Add validation for comparison operators

Open mhasel opened this issue 2 years ago • 5 comments

Is your feature request related to a problem? Please describe. When trying to compile code with a comparison statement which tries to compare incompatible types, we currently do not validate against it and the compilation will fail during codegen.

FUNCTION main : DINT
        VAR
            x1 : ARRAY[0..2] OF TOD;
            x2 : STRING;
        END_VAR
            x1 > x2;  
            GT(x1, x2);
END_FUNCTION

Describe the solution you'd like We should validate if the two types in the binary expression can be compared to each other

mhasel avatar Nov 23 '23 10:11 mhasel

The standard seems to say that all elementary datatypes should be comparable to values of their own type and it seems to suggest that you cannot compare different data types without an explicit conversion (looks like section 2.5.1.4 in my copy). I tested out comparing different types and it seems like we allow a lot of cross-type comparison through integer promotion (zext, sext, uitofp, etc) and string comparison didn't seem to work for me (maybe that's defined in a standard library that I need to link?)

So sounds like the scope of this issue is to disallow ALL comparison between values of different data types? Does that sound correct?

nturley-copia avatar Jul 08 '24 14:07 nturley-copia

So the decision to still compare types even if the norm disallows it is that we don't want to be a strict standard compliant compiler, we saw that C for example would convert values during comparison. The plan was always to add validations to get closer to the norm if desired. In regards to strings and other comparisons, because such a comparison is not easy to build in the language directly we opted out to implement it in the standard library. The way this works is for any datatype being compared, if the type is not native we will look for <DATATYPE>_EQUAL and <DATATYPE>_GT and <DATATYPE>_LT functions. They can be defined anywhere, we just also provide them as part of the standard function because we assume the user will always link with it.

ghaith avatar Jul 09 '24 07:07 ghaith

The C rules about integer promotion are part of the C standard but as far as I know, Rust does not perform integer promotion because they have a strong bias towards safety and implicit casting can be a source of bugs.

If we aren't targeting the IEC standard then what dialect of structured text are we targeting? Are we just trying to reproduce the behavior of the proprietary PLC vendor compilers? Or are we making up our own dialect with features that we think are generally useful?

nturley-copia avatar Jul 09 '24 13:07 nturley-copia

We're targeting both the IEC standard and Codesys compatibility, but we would also like to have the flexibility in the compiler to diverge from that. For example we think implicit type conversion can be useful in some cases but at the same time we know that some applications rely on a strict conversion. Another example that comes to mind is that we support variables inside of bit access, the standard expects bit access to happen using numbers only. Codesys requires constants to be used as bit access accessors. So the goal is to have a compiler that feels good to use, while maintaining compatibility where needed with Codesys and the standard where you could enable such profiles using our error code features. We're just also adding some ergonomic features that we see as being useful.

ghaith avatar Jul 09 '24 14:07 ghaith

standard perspective: the standard (IEC 61131-3:2013) mentions in 7.3.3.2.1 that comparisons are allowed on same types and if one value is implicit convertable into another

in general as @ghaith mentioned, we are not as strict as the standard. we try to see what could make sense to allow more and give the user the decision if standard compliance is a goal for the use case. that means usually we try to make validations configurable to be able to set a standard compliant configuration, a plc vendor compliant configuration or a user configuration as needed. so i would say we implement a configurable dialect that could be adapted to the needs of a user

tisis2 avatar Jul 09 '24 14:07 tisis2