Skip to content

Commit aa577c2

Browse files
authored
feat(validation): Assignment suggestions for = operator (#1049)
This commit adds a validation to identify binary expressions using the `=` (equal) operator with no effects. For example the following statement `foo = bar;` does nothing and the user probably meant to use a `:=` here, i.e. `foo := bar;`. Resolves #939
1 parent a3c0055 commit aa577c2

13 files changed

+223
-121
lines changed

compiler/plc_diagnostics/src/diagnostics.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -744,6 +744,13 @@ impl Diagnostic {
744744
err_no: ErrNo::var__invalid_enum_variant,
745745
}
746746
}
747+
748+
pub fn assignment_instead_of_equal(range: SourceLocation) -> Diagnostic {
749+
Diagnostic::ImprovementSuggestion {
750+
message: "This statement has no effect, did you mean to use `:=`?".to_string(),
751+
range: vec![range],
752+
}
753+
}
747754
}
748755

749756
// CFC related diagnostics

src/codegen/tests/debug_tests/snapshots/rusty__codegen__tests__debug_tests__expression_debugging__repeat_conditions_location_marked.snap

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,12 @@ entry:
1515
condition_check: ; preds = %while_body
1616
%load_myFunc = load i32, i32* %myFunc, align 4, !dbg !12
1717
%tmpVar = icmp sgt i32 %load_myFunc, 10, !dbg !12
18-
%tmpVar1 = xor i1 %tmpVar, true, !dbg !12
19-
br i1 %tmpVar1, label %while_body, label %continue, !dbg !12
18+
%0 = zext i1 %tmpVar to i8, !dbg !12
19+
%1 = icmp ne i8 %0, 0, !dbg !12
20+
%tmpVar1 = xor i1 %1, true, !dbg !12
21+
%2 = zext i1 %tmpVar1 to i8, !dbg !12
22+
%3 = icmp ne i8 %2, 0, !dbg !12
23+
br i1 %3, label %while_body, label %continue, !dbg !12
2024

2125
while_body: ; preds = %entry, %condition_check
2226
store i32 1, i32* %myFunc, align 4, !dbg !11

src/codegen/tests/debug_tests/snapshots/rusty__codegen__tests__debug_tests__expression_debugging__while_conditions_location_marked.snap

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ entry:
1515
condition_check: ; preds = %entry, %while_body
1616
%load_myFunc = load i32, i32* %myFunc, align 4, !dbg !12
1717
%tmpVar = icmp sgt i32 %load_myFunc, 1, !dbg !12
18-
br i1 %tmpVar, label %while_body, label %continue, !dbg !12
18+
%0 = zext i1 %tmpVar to i8, !dbg !12
19+
%1 = icmp ne i8 %0, 0, !dbg !12
20+
br i1 %1, label %while_body, label %continue, !dbg !12
1921

2022
while_body: ; preds = %condition_check
2123
store i32 1, i32* %myFunc, align 4, !dbg !11

src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__if_with_expression_generator_test.snap

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,22 +15,26 @@ entry:
1515
%b1 = getelementptr inbounds %prg, %prg* %0, i32 0, i32 1
1616
%load_x = load i32, i32* %x, align 4
1717
%tmpVar = icmp sgt i32 %load_x, 1
18-
br i1 %tmpVar, label %3, label %1
18+
%1 = zext i1 %tmpVar to i8
19+
%2 = icmp ne i8 %1, 0
20+
br i1 %2, label %5, label %3
1921

20-
condition_body: ; preds = %3
22+
condition_body: ; preds = %5
2123
%load_x1 = load i32, i32* %x, align 4
2224
br label %continue
2325

24-
continue: ; preds = %condition_body, %3
26+
continue: ; preds = %condition_body, %5
2527
ret void
2628

27-
1: ; preds = %entry
29+
3: ; preds = %entry
2830
%load_b1 = load i8, i8* %b1, align 1
29-
%2 = icmp ne i8 %load_b1, 0
30-
br label %3
31-
32-
3: ; preds = %1, %entry
33-
%4 = phi i1 [ %tmpVar, %entry ], [ %2, %1 ]
34-
br i1 %4, label %condition_body, label %continue
31+
%4 = icmp ne i8 %load_b1, 0
32+
br label %5
33+
34+
5: ; preds = %3, %entry
35+
%6 = phi i1 [ %2, %entry ], [ %4, %3 ]
36+
%7 = zext i1 %6 to i8
37+
%8 = icmp ne i8 %7, 0
38+
br i1 %8, label %condition_body, label %continue
3539
}
3640

src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__returning_early_in_function.snap

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ entry:
1414
%load_n = load i8, i8* %n, align 1
1515
%1 = sext i8 %load_n to i32
1616
%tmpVar = icmp slt i32 %1, 10
17-
br i1 %tmpVar, label %condition_body, label %continue
17+
%2 = zext i1 %tmpVar to i8
18+
%3 = icmp ne i8 %2, 0
19+
br i1 %3, label %condition_body, label %continue
1820

1921
condition_body: ; preds = %entry
2022
%smaller_than_ten_ret = load i16, i16* %smaller_than_ten, align 2

src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__returning_early_in_function_block.snap

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ entry:
1515
%load_n = load i8, i8* %n, align 1
1616
%1 = sext i8 %load_n to i32
1717
%tmpVar = icmp slt i32 %1, 10
18-
br i1 %tmpVar, label %condition_body, label %continue
18+
%2 = zext i1 %tmpVar to i8
19+
%3 = icmp ne i8 %2, 0
20+
br i1 %3, label %condition_body, label %continue
1921

2022
condition_body: ; preds = %entry
2123
ret void

src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__while_loop_with_if_exit.snap

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,19 @@ entry:
1717
condition_check: ; preds = %entry, %continue3
1818
%load_x = load i32, i32* %x, align 4
1919
%tmpVar = icmp slt i32 %load_x, 20
20-
br i1 %tmpVar, label %while_body, label %continue
20+
%1 = zext i1 %tmpVar to i8
21+
%2 = icmp ne i8 %1, 0
22+
br i1 %2, label %while_body, label %continue
2123

2224
while_body: ; preds = %condition_check
2325
%load_x1 = load i32, i32* %x, align 4
2426
%tmpVar2 = add i32 %load_x1, 1
2527
store i32 %tmpVar2, i32* %x, align 4
2628
%load_x4 = load i32, i32* %x, align 4
2729
%tmpVar5 = icmp sge i32 %load_x4, 10
28-
br i1 %tmpVar5, label %condition_body, label %continue3
30+
%3 = zext i1 %tmpVar5 to i8
31+
%4 = icmp ne i8 %3, 0
32+
br i1 %4, label %condition_body, label %continue3
2933

3034
continue: ; preds = %condition_body, %condition_check
3135
ret void

src/codegen/tests/snapshots/rusty__codegen__tests__code_gen_tests__while_with_expression_statement.snap

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ condition_check: ; preds = %entry, %while_body
1818
%load_x = load i8, i8* %x, align 1
1919
%1 = zext i8 %load_x to i32
2020
%tmpVar = icmp eq i32 %1, 0
21-
br i1 %tmpVar, label %while_body, label %continue
21+
%2 = zext i1 %tmpVar to i8
22+
%3 = icmp ne i8 %2, 0
23+
br i1 %3, label %while_body, label %continue
2224

2325
while_body: ; preds = %condition_check
2426
%load_x1 = load i8, i8* %x, align 1

src/resolver.rs

Lines changed: 89 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,12 @@ pub struct VisitorContext<'s> {
7575
/// e.g. true for `a.b.c` if either a,b or c is declared in a constant block
7676
constant: bool,
7777

78-
/// true the visitor entered a body (so no declarations)
78+
/// true if the visitor entered a body (so no declarations)
7979
in_body: bool,
8080

81+
/// true if the visitor entered a control statement
82+
in_control: bool,
83+
8184
pub id_provider: IdProvider,
8285

8386
// what's the current strategy for resolving
@@ -87,80 +90,54 @@ pub struct VisitorContext<'s> {
8790
impl<'s> VisitorContext<'s> {
8891
/// returns a copy of the current context and changes the `current_qualifier` to the given qualifier
8992
fn with_qualifier(&self, qualifier: String) -> VisitorContext<'s> {
90-
VisitorContext {
91-
pou: self.pou,
92-
qualifier: Some(qualifier),
93-
lhs: self.lhs,
94-
constant: false,
95-
in_body: self.in_body,
96-
id_provider: self.id_provider.clone(),
97-
resolve_strategy: self.resolve_strategy.clone(),
98-
}
93+
let mut ctx = self.clone();
94+
ctx.qualifier = Some(qualifier);
95+
ctx.constant = false;
96+
ctx
9997
}
10098

10199
/// returns a copy of the current context and changes the `current_pou` to the given pou
102100
fn with_pou(&self, pou: &'s str) -> VisitorContext<'s> {
103-
VisitorContext {
104-
pou: Some(pou),
105-
qualifier: self.qualifier.clone(),
106-
lhs: self.lhs,
107-
constant: false,
108-
in_body: self.in_body,
109-
id_provider: self.id_provider.clone(),
110-
resolve_strategy: self.resolve_strategy.clone(),
111-
}
101+
let mut ctx = self.clone();
102+
ctx.pou = Some(pou);
103+
ctx.constant = false;
104+
ctx
112105
}
113106

114107
/// returns a copy of the current context and changes the `lhs_pou` to the given pou
115108
fn with_lhs(&self, lhs_pou: &'s str) -> VisitorContext<'s> {
116-
VisitorContext {
117-
pou: self.pou,
118-
qualifier: self.qualifier.clone(),
119-
lhs: Some(lhs_pou),
120-
constant: false,
121-
in_body: self.in_body,
122-
id_provider: self.id_provider.clone(),
123-
resolve_strategy: self.resolve_strategy.clone(),
124-
}
109+
let mut ctx = self.clone();
110+
ctx.lhs = Some(lhs_pou);
111+
ctx.constant = false;
112+
ctx
125113
}
126114

127115
/// returns a copy of the current context and changes the `is_call` to true
128116
fn with_const(&self, const_state: bool) -> VisitorContext<'s> {
129-
VisitorContext {
130-
pou: self.pou,
131-
qualifier: self.qualifier.clone(),
132-
lhs: self.lhs,
133-
constant: const_state,
134-
in_body: self.in_body,
135-
id_provider: self.id_provider.clone(),
136-
resolve_strategy: self.resolve_strategy.clone(),
137-
}
117+
let mut ctx = self.clone();
118+
ctx.constant = const_state;
119+
ctx
138120
}
139121

140122
// returns a copy of the current context and sets the in_body field to true
141123
fn enter_body(&self) -> Self {
142-
VisitorContext {
143-
pou: self.pou,
144-
qualifier: self.qualifier.clone(),
145-
lhs: self.lhs,
146-
constant: self.constant,
147-
in_body: true,
148-
id_provider: self.id_provider.clone(),
149-
resolve_strategy: self.resolve_strategy.clone(),
150-
}
124+
let mut ctx = self.clone();
125+
ctx.in_body = true;
126+
ctx
127+
}
128+
129+
fn enter_control(&self) -> Self {
130+
let mut ctx = self.clone();
131+
ctx.in_control = true;
132+
ctx
151133
}
152134

153135
// returns a copy of the current context and sets the resolve_strategy field to the given strategies
154136
fn with_resolving_strategy(&self, resolve_strategy: Vec<ResolvingScope>) -> Self {
155-
VisitorContext {
156-
pou: self.pou,
157-
qualifier: self.qualifier.clone(),
158-
lhs: self.lhs,
159-
constant: self.constant,
160-
in_body: true,
161-
id_provider: self.id_provider.clone(),
162-
resolve_strategy,
163-
}
137+
let mut ctx = self.clone();
138+
ctx.in_body = true;
139+
ctx.resolve_strategy = resolve_strategy;
140+
ctx
164141
}
165142

166143
fn is_in_a_body(&self) -> bool {
@@ -762,6 +739,7 @@ impl<'i> TypeAnnotator<'i> {
762739
in_body: false,
763740
id_provider,
764741
resolve_strategy: ResolvingScope::default_scopes(),
742+
in_control: false,
765743
};
766744

767745
for global_variable in unit.global_vars.iter().flat_map(|it| it.variables.iter()) {
@@ -1244,54 +1222,56 @@ impl<'i> TypeAnnotator<'i> {
12441222
self.visit_statement(ctx, expr);
12451223
self.inherit_annotations(statement, expr);
12461224
}
1247-
AstStatement::ControlStatement(AstControlStatement::If(stmt), ..) => {
1248-
stmt.blocks.iter().for_each(|b| {
1249-
self.visit_statement(ctx, b.condition.as_ref());
1250-
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
1251-
});
1252-
stmt.else_block.iter().for_each(|e| self.visit_statement(ctx, e));
1253-
}
1254-
AstStatement::ControlStatement(AstControlStatement::ForLoop(stmt), ..) => {
1255-
visit_all_statements!(self, ctx, &stmt.counter, &stmt.start, &stmt.end);
1256-
if let Some(by_step) = &stmt.by_step {
1257-
self.visit_statement(ctx, by_step);
1258-
}
1259-
//Hint annotate start, end and step with the counter's real type
1260-
if let Some(type_name) = self
1261-
.annotation_map
1262-
.get_type(&stmt.counter, self.index)
1263-
.map(typesystem::DataType::get_name)
1264-
{
1265-
let annotation = StatementAnnotation::value(type_name);
1266-
self.annotation_map.annotate_type_hint(&stmt.start, annotation.clone());
1267-
self.annotation_map.annotate_type_hint(&stmt.end, annotation.clone());
1268-
if let Some(by_step) = &stmt.by_step {
1269-
self.annotation_map.annotate_type_hint(by_step, annotation);
1225+
AstStatement::ControlStatement(control) => {
1226+
match control {
1227+
AstControlStatement::If(stmt) => {
1228+
stmt.blocks.iter().for_each(|b| {
1229+
self.visit_statement(&ctx.enter_control(), b.condition.as_ref());
1230+
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
1231+
});
1232+
stmt.else_block.iter().for_each(|e| self.visit_statement(ctx, e));
12701233
}
1271-
}
1272-
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
1273-
}
1274-
AstStatement::ControlStatement(AstControlStatement::WhileLoop(stmt), ..)
1275-
| AstStatement::ControlStatement(AstControlStatement::RepeatLoop(stmt), ..) => {
1276-
self.visit_statement(ctx, &stmt.condition);
1277-
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
1278-
}
1279-
AstStatement::ControlStatement(AstControlStatement::Case(stmt), ..) => {
1280-
self.visit_statement(ctx, &stmt.selector);
1281-
let selector_type = self.annotation_map.get_type(&stmt.selector, self.index).cloned();
1282-
stmt.case_blocks.iter().for_each(|b| {
1283-
self.visit_statement(ctx, b.condition.as_ref());
1284-
if let Some(selector_type) = &selector_type {
1285-
self.update_expected_types(selector_type, b.condition.as_ref());
1234+
AstControlStatement::ForLoop(stmt) => {
1235+
visit_all_statements!(self, ctx, &stmt.counter, &stmt.start, &stmt.end);
1236+
if let Some(by_step) = &stmt.by_step {
1237+
self.visit_statement(ctx, by_step);
1238+
}
1239+
//Hint annotate start, end and step with the counter's real type
1240+
if let Some(type_name) = self
1241+
.annotation_map
1242+
.get_type(&stmt.counter, self.index)
1243+
.map(typesystem::DataType::get_name)
1244+
{
1245+
let annotation = StatementAnnotation::value(type_name);
1246+
self.annotation_map.annotate_type_hint(&stmt.start, annotation.clone());
1247+
self.annotation_map.annotate_type_hint(&stmt.end, annotation.clone());
1248+
if let Some(by_step) = &stmt.by_step {
1249+
self.annotation_map.annotate_type_hint(by_step, annotation);
1250+
}
1251+
}
1252+
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
1253+
}
1254+
AstControlStatement::WhileLoop(stmt) | AstControlStatement::RepeatLoop(stmt) => {
1255+
self.visit_statement(&ctx.enter_control(), &stmt.condition);
1256+
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
1257+
}
1258+
AstControlStatement::Case(stmt) => {
1259+
self.visit_statement(ctx, &stmt.selector);
1260+
let selector_type = self.annotation_map.get_type(&stmt.selector, self.index).cloned();
1261+
stmt.case_blocks.iter().for_each(|b| {
1262+
self.visit_statement(ctx, b.condition.as_ref());
1263+
if let Some(selector_type) = &selector_type {
1264+
self.update_expected_types(selector_type, b.condition.as_ref());
1265+
}
1266+
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
1267+
});
1268+
stmt.else_block.iter().for_each(|s| self.visit_statement(ctx, s));
12861269
}
1287-
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
1288-
});
1289-
stmt.else_block.iter().for_each(|s| self.visit_statement(ctx, s));
1270+
}
12901271
}
1272+
12911273
AstStatement::CaseCondition(condition, ..) => self.visit_statement(ctx, condition),
1292-
_ => {
1293-
self.visit_statement_expression(ctx, statement);
1294-
}
1274+
_ => self.visit_statement_expression(ctx, statement),
12951275
}
12961276
}
12971277

@@ -1399,7 +1379,15 @@ impl<'i> TypeAnnotator<'i> {
13991379
};
14001380

14011381
if let Some(statement_type) = statement_type {
1402-
self.annotate(statement, StatementAnnotation::value(statement_type));
1382+
self.annotate(statement, StatementAnnotation::value(statement_type.clone()));
1383+
1384+
// https://github.com/PLC-lang/rusty/issues/939: We rely on type-hints in order
1385+
// to identify `=` operations that have no effect (e.g. `foo = bar;`) hence
1386+
// type-hint the conditions of control statements to eliminate false-positives.
1387+
if ctx.in_control {
1388+
self.annotation_map
1389+
.annotate_type_hint(statement, StatementAnnotation::value(statement_type))
1390+
}
14031391
}
14041392
}
14051393
AstStatement::UnaryExpression(data, ..) => {
@@ -1435,7 +1423,7 @@ impl<'i> TypeAnnotator<'i> {
14351423
visit_all_statements!(self, ctx, &data.start, &data.end);
14361424
}
14371425
AstStatement::Assignment(data, ..) => {
1438-
self.visit_statement(ctx, &data.right);
1426+
self.visit_statement(&ctx.enter_control(), &data.right);
14391427
if let Some(lhs) = ctx.lhs {
14401428
//special context for left hand side
14411429
self.visit_statement(&ctx.with_pou(lhs).with_lhs(lhs), &data.left);

0 commit comments

Comments
 (0)