fix: 'else if' is now lowered into an else > if > else#1620
fix: 'else if' is now lowered into an else > if > else#1620Angus-Bethke-Bachmann merged 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Nice work!
Can you have a look at the complex_call_statement_in_elif_condition test in rusty/src/lowering/calls.rs - we have an example there with a FIXME comment about side-effects for calls in elsif blocks - they were always being evaluated.
Your PR should solve this, but I'd like to see it proven in a test aswell. E.g.:
IF TRUE THEN
ELSIF printHello()
END_IF
The test should fail if printHello actually prints anything.
Then please also remove the now outdated FIXME comment.
Side note: I feel like this change should have maybe affected some IR snapshots aswell, but that doesn't seem to be the case. Maybe something to look into. Maybe it's a lack of coverage, but I would have expected the generated IR to change if these side effects are now no longer reproducible.
| @@ -0,0 +1,255 @@ | |||
| use plc_ast::{ | |||
There was a problem hiding this comment.
Could you maybe add a top-level comment explaining the motivation/reason to do this? if someone is unaware of the aggregate-type lowering regression/behaviour for if/elsif/else blocks, this might look very strange and like an absolutely mental thing to do 😄
There was a problem hiding this comment.
I have added a top-level comment detailing the purpose of this lowerer.
PS: Replying to your top comment here as well 😄
I have also removed the FIXME and adjusted that test to include the additional lowerer. Snapshot changes look to be as expected.
With regards to the IR changes, it looks like the codegen test utility is not using the pipeline participants, so I have manually added the lowerer to the test utility... but I did leave a comment stating that it is probably a good idea (at some point in the future) for the codegen test utility to use all of the participants (this would give us a more accurate reflection of the code that would be generated by the compiler). Bonus points if we use the same mechanism to fetch the participants as that would mean that if we add a new participant it is automatically consumed by the codegen test utility and all codegen test snapshots would update to reflect that.
There was a problem hiding this comment.
Ah, of course - the test utils. I've stumbled over this a few times myself. I think one of the reasons we kept them is #1407. Another one is the fact that the compiler isn't yet fully split into separate crates, creating circular dependencies when trying to import certain runners into the rusty main crate.
Definitely something to look at in future.
| @@ -0,0 +1,44 @@ | |||
| // RUN: (%COMPILE %s && %RUN) | %CHECK %s | |||
| FUNCTION foo : ARRAY[0..1] OF STRING[10] | |||
There was a problem hiding this comment.
I'd like to see some codegen (IR-snapshot) tests for this aswell.
There was a problem hiding this comment.
Done 😄
You will find them under src/codegen/tests/control_statement_tests.rs
| assert_eq!(implementation.name, "mainProg"); | ||
|
|
||
| let control_statement = &implementation.statements[2]; | ||
| assert_snapshot!(format!("{:#?}", control_statement), @r#" |
There was a problem hiding this comment.
Let's make use of the AstSerializer here, reads nicer imo.
There was a problem hiding this comment.
Done 😄
| use plc_source::SourceCode; | ||
|
|
||
| #[test] | ||
| fn elseif_is_lowered_to_else_with_nested_if() { |
There was a problem hiding this comment.
nit: Can you think of some further tests and edge-cases to test here? Maybe brainstorm a bit with a clanker, usually works pretty good (opus 4.6 or gpt 5.X models).
There was a problem hiding this comment.
For example
PROGRAM mainProg
VAR
i : INT;
val : INT;
cVar : CHAR;
END_VAR
FOR i := 0 TO 10 DO
IF val = 3 THEN
cVar := 'f';
ELSIF val = 5 THEN
cVar := 'b';
ELSE
cVar := 'x';
END_IF
END_FOR
END_PROGRAM
There was a problem hiding this comment.
I have added some additional test cases that I could think of. Is that sufficient?
3 participants
Changed
Testing