fix: moved shl and shr library functions to builtin #1562
Conversation
…patible with integers
| #[derive(Default, Debug)] | ||
| #[repr(C)] | ||
| struct MainTypePrg3569 { | ||
| a: u32, | ||
| } | ||
|
|
||
| #[test] | ||
| fn bug_prg_3569_shl_must_be_usable_with_int() { | ||
| let src = " | ||
| PROGRAM main | ||
| VAR | ||
| a : INT; | ||
| END_VAR | ||
| a := SHL(21,2); | ||
| END_PROGRAM | ||
| "; | ||
| let sources = SourceCode::new(src, "main.st"); | ||
| let mut maintype = MainTypePrg3569::default(); | ||
| let _res: u32 = compile_and_run(sources, &mut maintype); | ||
| let shift_left = 21 << 2; | ||
| assert_eq!(maintype.a, shift_left); | ||
| } | ||
|
|
||
| #[test] | ||
| fn bug_prg_3569_shr_must_be_usable_with_int() { | ||
| let src = " | ||
| PROGRAM main | ||
| VAR | ||
| a : INT; | ||
| END_VAR | ||
| a := SHR(21,2); | ||
| END_PROGRAM | ||
| "; | ||
| let sources = SourceCode::new(src, "main.st"); | ||
| let mut maintype = MainTypePrg3569::default(); | ||
| let _res: u32 = compile_and_run(sources, &mut maintype); | ||
| let shift_right = 21 >> 2; | ||
| assert_eq!(maintype.a, shift_right); | ||
| } |
There was a problem hiding this comment.
Can you use lit for these integration tests? For context, we have decided to use lit for all of our integration tests moving forward. At some point we would also like to migrate the existing ones but that hasn't happened yet.
For reference https://github.com/PLC-lang/rusty/tree/master/tests/lit
| "SHL", | ||
| BuiltIn { | ||
| decl: " | ||
| FUNCTION SHL<T: ANY> : T |
There was a problem hiding this comment.
By declaring ANY (instead of ANY_BIT as was declared previously), SHL('foo', 2) now becomes valid which will result in a panic
thread '<unnamed>' panicked at src/builtins.rs:624:74:
Found ArrayValue(ArrayValue { name: "", address: 0x7c7718007c00, is_const: false, is_const_array: false, is_const_data_array: false, is_null: false, llvm_value: " %0 = load [4 x i8], [4 x i8]* @utf08_literal_0, align 1", llvm_type: ArrayType { array_type: Type { address: 0x7c771801f650, llvm_type: "[4 x i8]" } } }) but expected the IntValue variant
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrack
In general I'm not sure if ANY this is the correct solution here. I believe the underlying issue is a resolver "bug". I took a quick look and it seems as if the derive_generic_types call is the culprit here (the call-flow is annotate_call_statement -> update_generic_call_statement -> derive_generic_types). The function is also the reason why we get a BOOL annotation on the generic type T, at least that's what I get when debugging
#[test]
fn temp() {
let id_provider = IdProvider::default();
let (unit, mut index) = index_with_ids(
r#"
FUNCTION SHLL<T: ANY_BIT>: T
VAR_INPUT
IN : T;
n : UDINT;
END_VAR
END_FUNCTION
FUNCTION main
SHLL(5, 2);
END_FUNCTION
"#,
id_provider.clone(),
);
let annotations = annotate_with_ids(&unit, &mut index, id_provider);
let stmt = &unit.implementations.iter().find(|imp| imp.name == "main").unwrap().statements[0];
let AstStatement::CallStatement(CallStatement { operator, parameters: Some(parameters), .. }) =
stmt.get_stmt()
else {
unreachable!();
};
// SHLL(5, 2);
// ^^^^^^^^^^
insta::assert_debug_snapshot!(annotations.get(stmt), @r#"
Some(
Value {
resulting_type: "BOOL",
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type(stmt, &index), @r#"
Some(
DataType {
name: "BOOL",
initial_value: None,
information: Integer {
name: "BOOL",
signed: false,
size: 8,
semantic_size: Some(
1,
),
},
nature: Bit,
location: SourceLocation {
span: None,
},
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type_hint(stmt, &index), @"None");
let AstStatement::ExpressionList(arguments) = ¶meters.stmt else {
unreachable!();
};
// SHLL(5, 2);
// ^
insta::assert_debug_snapshot!(annotations.get(&arguments[0]), @r#"
Some(
Value {
resulting_type: "DINT",
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type(&arguments[0], &index), @r#"
Some(
DataType {
name: "DINT",
initial_value: None,
information: Integer {
name: "DINT",
signed: true,
size: 32,
semantic_size: None,
},
nature: Signed,
location: SourceLocation {
span: None,
},
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type_hint(&arguments[0], &index), @r#"
Some(
DataType {
name: "BOOL",
initial_value: None,
information: Integer {
name: "BOOL",
signed: false,
size: 8,
semantic_size: Some(
1,
),
},
nature: Bit,
location: SourceLocation {
span: None,
},
},
)
"#);
// SHLL(5, 2);
// ^
insta::assert_debug_snapshot!(annotations.get(&arguments[1]), @r#"
Some(
Value {
resulting_type: "DINT",
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type(&arguments[1], &index), @r#"
Some(
DataType {
name: "DINT",
initial_value: None,
information: Integer {
name: "DINT",
signed: true,
size: 32,
semantic_size: None,
},
nature: Signed,
location: SourceLocation {
span: None,
},
},
)
"#);
insta::assert_debug_snapshot!(annotations.get_type_hint(&arguments[1], &index), @r#"
Some(
DataType {
name: "UDINT",
initial_value: None,
information: Integer {
name: "UDINT",
signed: false,
size: 32,
semantic_size: None,
},
nature: Unsigned,
location: SourceLocation {
span: None,
},
},
)
"#);| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift left operation on bytes | ||
| pub fn SHL__BYTE(input: u8, n: u32) -> u8 { | ||
| input << n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift left operation on word | ||
| pub fn SHL__WORD(input: u16, n: u32) -> u16 { | ||
| input << n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift left operation on dword | ||
| pub fn SHL__DWORD(input: u32, n: u32) -> u32 { | ||
| input << n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift left operation on lword | ||
| pub fn SHL__LWORD(input: u64, n: u32) -> u64 { | ||
| input << n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift right operation on bytes | ||
| pub fn SHR__BYTE(input: u8, n: u32) -> u8 { | ||
| input >> n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift right operation on word | ||
| pub fn SHR__WORD(input: u16, n: u32) -> u16 { | ||
| input >> n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift right operation on dword | ||
| pub fn SHR__DWORD(input: u32, n: u32) -> u32 { | ||
| input >> n | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| #[no_mangle] | ||
| /// Shift right operation on lword | ||
| pub fn SHR__LWORD(input: u64, n: u32) -> u64 { | ||
| input >> n | ||
| } | ||
|
|
There was a problem hiding this comment.
@ghaith why the migration from SH{L,R} as a standard library function to a built-in one? Shouldn't we try to keep the built-ins as small as possible?
|
I could be wrong, but I thought the main issue was that any_bit does not include ints in our typesystem, so i proposed move it to any and validate at compile time. Moving to builtins would be to allow such a validation at compile time without panics, we just have to make sure the type are numerical or int. But again, did not debug much i just proposed the move based on that i saw on a quick look. Might make sense if you guys pair on this if you have a different proposal. Builtin shr makes sense because they are intrinsic. I would eventually like us to be able to handle intrisnsics differently but that was already in place |
I believe that's the issue, Edit: Now that I think about it, SH{L,R} might make sense as a built-in given other languages also do it. Maybe the (easiest) solution is |
Changed:
Testing: