Skip to content
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

generic validation false positive #681

Closed
mhasel opened this issue Dec 5, 2022 · 3 comments · Fixed by #683
Closed

generic validation false positive #681

mhasel opened this issue Dec 5, 2022 · 3 comments · Fixed by #683
Assignees
Labels
bug Something isn't working validation candidate for syntactic or semantic validation

Comments

@mhasel
Copy link
Member

mhasel commented Dec 5, 2022

Describe the bug
When calling the builtin EXPT function with int types, without explicitly annotating the exponent, a validation error is reported.
This looks like a false-positive.

To Reproduce
Steps to reproduce the behavior:

    FUNCTION main : DINT
    VAR
        i : DINT;
        i2: DINT;
        r : REAL;
    END_VAR
        i := 2**DINT#3; // this works
        i2 := 2**3; // this line reports an error
        r := 3.0**2; // also does not seem to happen with REAL types
    END_FUNCTION

Tests
Failing test can be found in linked dev branch.

@mhasel mhasel added the bug Something isn't working label Dec 5, 2022
@mhasel mhasel linked a pull request Dec 5, 2022 that will close this issue
@mhasel
Copy link
Member Author

mhasel commented Dec 5, 2022

This error is caused due to behaviour of the builtin EXPT function. Here, we differentiate between signed and unsigned integer types and, if smaller than 32 bit, annotate their types as DINT or UDINT respectively.
This UDINT annotation causes stmt_validator::validate_type_nature to return a diagnostics error for EXPT calls without explicit exponent annotation, e.g. 2**3 due to a mismatch of DINT (statement) and UDINT (exponent literal) types.

I have discussed this with @99NIMI and we tried to always use DINT, regardless of sign.
While we looked over the newly generated snapshots we noticed that previously, the base of 2**3 was annotated as REAL, which also doesn't seem right.
The change we made to EXPT fixed the diagnostics issue and the generated IR looks good, there is an edge case however:

For an exponentiation K**N where i32::MAX < N <= u32::MAX, N will overflow.
e.g.:

        PROGRAM prg
        VAR
            x : UDINT;
        END_VAR
          x := 1**UDINT#3_000_000_000;
        END_PROGRAM

Granted, the result of such a large exponent will overflow for almost any K larger than 1.. Here is a Rust Playground example of 1.001**(i32::MAX + 1).

A different approach would be to once again emulate C and have the result of EXPT always be LREAL and typecast the result
if needed or to change the validator to accept different types between statement and literal, if they derive from the same type.

Please discuss.

@mhasel mhasel self-assigned this Dec 5, 2022
@mhasel
Copy link
Member Author

mhasel commented Dec 6, 2022

@ghaith pointed out that we can't simply drop the sign check because negative exponents result in floating point results, which, in hindsight, is obvious.

@mhasel mhasel mentioned this issue Dec 15, 2022
@mhasel
Copy link
Member Author

mhasel commented Dec 20, 2022

linked: StandardFunctions issue

@riederm riederm added the validation candidate for syntactic or semantic validation label Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working validation candidate for syntactic or semantic validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants