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

Wrong downcast warning #828

Closed
volsa opened this issue Mar 27, 2023 · 3 comments · Fixed by #1114
Closed

Wrong downcast warning #828

volsa opened this issue Mar 27, 2023 · 3 comments · Fixed by #1114

Comments

@volsa
Copy link
Member

volsa commented Mar 27, 2023

The following code

FUNCTION main : INT
    VAR
        a, b, c : INT;
    END_VAR

    a := b + c;
    a := a * b + c;
END_FUNCTION

will report

warning: Potential loss of information due to assigning 'DINT' to variable of type 'INT'.
  ┌─ demo.st:6:5
  │
6 │     a := b + c;
  │     ^^^^^^^^^^ Potential loss of information due to assigning 'DINT' to variable of type 'INT'.

warning: Potential loss of information due to assigning 'DINT' to variable of type 'INT'.
  ┌─ demo.st:7:5
  │
7 │     a := a * b + c;
  │     ^^^^^^^^^^^^^^ Potential loss of information due to assigning 'DINT' to variable of type 'INT'.

which is wrong because a, b and c are of type INT

@volsa volsa added refactor internal change, cleanup, code-style-improvement and removed refactor internal change, cleanup, code-style-improvement labels Mar 27, 2023
@99NIMI
Copy link
Member

99NIMI commented Mar 29, 2023

rusty/src/resolver.rs

Lines 897 to 911 in 47bb28f

if l_intrinsic_type.is_numerical() && r_intrinsic_type.is_numerical() {
let bigger_type = if l_intrinsic_type.is_bool() && r_intrinsic_type.is_bool() {
left_type
} else {
let ty = if left_type.is_bit() && right_type.is_bit() {
right_type
} else {
self.index.get_type_or_panic(DINT_TYPE)
};
get_bigger_type(
get_bigger_type(left_type, right_type, self.index),
ty,
self.index,
)
};

i think we can replace everything in the first else with
get_bigger_type(left_type, right_type, self.index)
it should do the job for bit types and i'm not sure we why we called get_bigger_type(..) a second time with DINT

@ghaith
Copy link
Collaborator

ghaith commented Mar 31, 2023

Default behavior on all arithmetic operations is to do them with DINT (same as C does with the int type)
All literals are also DINT based, so a := a + 100 will also have the same issue

The warning you get is technically correct, the addition and multiplication will always default to DINT.

Not sure yet how we solve this, @99NIMI's idea is certainly a possibility if there is no reason for the conversion. Another is we need to mark this type of conversions (where all types are the same) as legal..
@riederm ideas? TBH I can't recall the reason we followed the C convention here.
I know we wanted to have the same datatype everywhere in the operation, but not why we forced DINT (initially we forced INT actually) Was it performance? (i32 operations are easier)

@riederm
Copy link
Collaborator

riederm commented Mar 31, 2023

... i'm not sure we why we called get_bigger_type(..) a second time with DINT

We don't really need to do this, but we followed C's integral promotion strategy. The idea is basically, that anyway every arithmetic operation will be performed on at least 32bit numbers on the CPU level, so we need to cast them anyway. Btw. Java does exactly the same! Furthermore it is incredibly hard to guess the correct type-size for a literal.

Since the LLVM IR offers arithmetic operations for all sizes, we would just need to ensure that both operands have the same type. This means that annotating literal integers are a bit harder :-/. If we keep them at DINT we will have the same problem.

mySINT := 7; 
         ^^^ at the moment: DINT

We already do some type-updates of "right hand side" expressions but it gets hard if you have complex stuff like:

mySINT := (myByte * 99) - 200;   // here all iterals should be of type byte?
mySINT := (myByte * 99) - 300;   // now 300 no longer fits into a byte?

myDINT := 
       myOtherDINT +
             ( myINT + 
                 ( mySINT + 100));

For the last example you start annotating this tree from the bottom up:

  • annotate 200 with Byte?
  • mySINT + 100 results in SINT? (probably already overflow)
  • (mySINT + 100) annotated as INT, because it is added to an INT ( 1 cast here)
  • myInt + (...) will result in an INT which will be casted to DINT (1 cast here)

this example gives you probably the wrong result, without any validation problems (you didnt want the mySINT + 100 to overflow when you ulitmately assign it to a DINT. Is it possible to backpropagate the DINT down the tree once you realized the end-result? - is it easy? idk

This whole feature reminds me more of rust's checked_add, checked_sub, etc. functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants