Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion src/validation/statement.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{collections::HashSet, mem::discriminant};

use plc_ast::control_statements::ForLoopStatement;
use plc_ast::control_statements::{ForLoopStatement, IfStatement};
use plc_ast::{
ast::{
flatten_expression_list, AstNode, AstStatement, DirectAccess, DirectAccessType, JumpStatement,
Expand Down Expand Up @@ -287,13 +287,34 @@ fn validate_for_loop<T: AnnotationMap>(
// by a VAR_INPUT {ref} function call.
}

fn validate_if_statement<T: AnnotationMap>(
validator: &mut Validator,
context: &ValidationContext<T>,
statement: &IfStatement,
) {
for block in &statement.blocks {
let kind = context.annotations.get_type_or_void(&block.condition, context.index);

if !kind.get_type_information().is_bool() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that this is wrong, but i'm interested to see how often people also use integers here the C way..
I would just accept this for now and and we discuss later if it might need to become a warning. In which case it will need its own error code btw.
Also note to self, when merging the diagnostics make sure this becomes E094

let slice = get_datatype_name_or_slice(validator.context, kind);
let message = format!("Expected a boolean, got `{slice}`");
validator.push_diagnostic(
Diagnostic::error(message)
.with_location(block.condition.get_location())
.with_error_code("E093"),
)
}
}
}

fn validate_control_statement<T: AnnotationMap>(
validator: &mut Validator,
control_statement: &AstControlStatement,
context: &ValidationContext<T>,
) {
match control_statement {
AstControlStatement::If(stmt) => {
validate_if_statement(validator, context, stmt);
stmt.blocks.iter().for_each(|b| {
visit_statement(validator, b.condition.as_ref(), context);
b.body.iter().for_each(|s| visit_statement(validator, s, context));
Expand Down
58 changes: 56 additions & 2 deletions src/validation/tests/statement_validation_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1402,7 +1402,7 @@ fn for_loop_conditions_are_numerical() {

FOR i := 100000 TO x BY y DO
END_FOR

END_PROGRAM
",
);
Expand Down Expand Up @@ -1436,7 +1436,7 @@ fn for_loop_conditions_are_real_and_trigger_error() {

FOR i := 10.0 TO x BY y DO
END_FOR

END_PROGRAM
",
);
Expand Down Expand Up @@ -1468,3 +1468,57 @@ fn for_loop_conditions_are_real_and_trigger_error() {

"###);
}

#[test]
fn if_statement_triggers_error_if_condition_is_not_boolean() {
let diagnostic = parse_and_validate_buffered(
"
FUNCTION main
VAR
x : BOOL;
y : DINT;
z : STRING;
END_VAR

IF x THEN
ELSIF y THEN
ELSIF z THEN
ELSIF 0 THEN
ELSIF 1 THEN

// These should be Ok
ELSIF (0 < 1) THEN
ELSIF (y < 0) THEN
ELSIF ((1 = 2) = 3) THEN
END_IF
END_FUNCTION
",
);

assert_snapshot!(diagnostic, @r###"
error: Expected a boolean, got `DINT`
┌─ <internal>:10:21
10 │ ELSIF y THEN
│ ^ Expected a boolean, got `DINT`

error: Expected a boolean, got `STRING`
┌─ <internal>:11:21
11 │ ELSIF z THEN
│ ^ Expected a boolean, got `STRING`

error: Expected a boolean, got `DINT`
┌─ <internal>:12:21
12 │ ELSIF 0 THEN
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think IF 1 and IF 0 are fine.. not because i like them but because 0 and 1 are common I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially also wanted to support them but I thought if someone is using 0 or 1 they could also just use FALSE or TRUE instead. But also I just replicated what Rust does, though comparing with C makes more sense generally speaking. Anyhow, do we proceed with supporting 0 and 1s here?

│ ^ Expected a boolean, got `DINT`

error: Expected a boolean, got `DINT`
┌─ <internal>:13:21
13 │ ELSIF 1 THEN
│ ^ Expected a boolean, got `DINT`

"###);
}