Skip to content

feat(validation): Assignment suggestions for = operator #1049

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Dec 19, 2023
Merged

feat(validation): Assignment suggestions for = operator #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b16c67b
Init
volsa Dec 6, 2023
84958c7
Revert "Init"
volsa Dec 7, 2023
503072e
Merge branch 'master' of github.com:PLC-lang/rusty into assignment-su…
volsa Dec 7, 2023
3f0d892
Detection seems to be working?
volsa Dec 7, 2023
065b674
Accept tests
volsa Dec 11, 2023
f849a33
documentation
volsa Dec 11, 2023
f2b8814
Add test
volsa Dec 11, 2023
65edee2
Rename
volsa Dec 11, 2023
bfbca8a
Delete unreferenced snapshot
volsa Dec 11, 2023
bf222bf
fmt
volsa Dec 11, 2023
5868db1
Merge branch 'master' into assignment-suggestion
volsa Dec 18, 2023
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
7 changes: 7 additions & 0 deletions compiler/plc_diagnostics/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,13 @@ impl Diagnostic {
err_no: ErrNo::var__invalid_enum_variant,
}
}

pub fn assignment_instead_of_equal(range: SourceLocation) -> Diagnostic {
Diagnostic::ImprovementSuggestion {
message: "This statement has no effect, did you mean to use `:=`?".to_string(),
range: vec![range],
}
}
}

// CFC related diagnostics
Expand Down
8 changes: 6 additions & 2 deletions ...codegen__tests__debug_tests__expression_debugging__repeat_conditions_location_marked.snap
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did these tests change? Nothing in the commit affects codegen/debugging

Copy link
Member Author

@volsa volsa Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Adding a type-hint onto the condition of control statements does because we then enter the cast_if_needed here afaik

rusty/src/codegen/generators/expression_generator.rs

Line 182 in 1e88fe5

Ok(cast_if_needed!(self, target_type, actual_type, v, self.annotations.get(expression)))

Copy link
Member

@mhasel mhasel Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My guess is because the condition expressions now receive a type-hint, we hit the cast_if_needed call in generate_expression were previously we returned the value as-is

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to add some additional context, this issue will go away once we implement #1041

Copy link
Member Author

Choose a reason for hiding this comment

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

Anything blocking this PR @ghaith @mhasel ? Arguably the IR got uglier but at the top of my head I'm not sure how else to implement this feature without type-hints.

Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ entry:
condition_check: ; preds = %while_body
%load_myFunc = load i32, i32* %myFunc, align 4, !dbg !12
%tmpVar = icmp sgt i32 %load_myFunc, 10, !dbg !12
%tmpVar1 = xor i1 %tmpVar, true, !dbg !12
br i1 %tmpVar1, label %while_body, label %continue, !dbg !12
%0 = zext i1 %tmpVar to i8, !dbg !12
%1 = icmp ne i8 %0, 0, !dbg !12
%tmpVar1 = xor i1 %1, true, !dbg !12
%2 = zext i1 %tmpVar1 to i8, !dbg !12
%3 = icmp ne i8 %2, 0, !dbg !12
br i1 %3, label %while_body, label %continue, !dbg !12

while_body: ; preds = %entry, %condition_check
store i32 1, i32* %myFunc, align 4, !dbg !11
Expand Down
4 changes: 3 additions & 1 deletion ..._codegen__tests__debug_tests__expression_debugging__while_conditions_location_marked.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ entry:
condition_check: ; preds = %entry, %while_body
%load_myFunc = load i32, i32* %myFunc, align 4, !dbg !12
%tmpVar = icmp sgt i32 %load_myFunc, 1, !dbg !12
br i1 %tmpVar, label %while_body, label %continue, !dbg !12
%0 = zext i1 %tmpVar to i8, !dbg !12
%1 = icmp ne i8 %0, 0, !dbg !12
br i1 %1, label %while_body, label %continue, !dbg !12

while_body: ; preds = %condition_check
store i32 1, i32* %myFunc, align 4, !dbg !11
Expand Down
24 changes: 14 additions & 10 deletions ...s/snapshots/rusty__codegen__tests__code_gen_tests__if_with_expression_generator_test.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,26 @@ entry:
%b1 = getelementptr inbounds %prg, %prg* %0, i32 0, i32 1
%load_x = load i32, i32* %x, align 4
%tmpVar = icmp sgt i32 %load_x, 1
br i1 %tmpVar, label %3, label %1
%1 = zext i1 %tmpVar to i8
%2 = icmp ne i8 %1, 0
br i1 %2, label %5, label %3

condition_body: ; preds = %3
condition_body: ; preds = %5
%load_x1 = load i32, i32* %x, align 4
br label %continue

continue: ; preds = %condition_body, %3
continue: ; preds = %condition_body, %5
ret void

1: ; preds = %entry
3: ; preds = %entry
%load_b1 = load i8, i8* %b1, align 1
%2 = icmp ne i8 %load_b1, 0
br label %3

3: ; preds = %1, %entry
%4 = phi i1 [ %tmpVar, %entry ], [ %2, %1 ]
br i1 %4, label %condition_body, label %continue
%4 = icmp ne i8 %load_b1, 0
br label %5

5: ; preds = %3, %entry
%6 = phi i1 [ %2, %entry ], [ %4, %3 ]
%7 = zext i1 %6 to i8
%8 = icmp ne i8 %7, 0
br i1 %8, label %condition_body, label %continue
}

4 changes: 3 additions & 1 deletion ...n/tests/snapshots/rusty__codegen__tests__code_gen_tests__returning_early_in_function.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ entry:
%load_n = load i8, i8* %n, align 1
%1 = sext i8 %load_n to i32
%tmpVar = icmp slt i32 %1, 10
br i1 %tmpVar, label %condition_body, label %continue
%2 = zext i1 %tmpVar to i8
%3 = icmp ne i8 %2, 0
br i1 %3, label %condition_body, label %continue

condition_body: ; preds = %entry
%smaller_than_ten_ret = load i16, i16* %smaller_than_ten, align 2
Expand Down
4 changes: 3 additions & 1 deletion ...s/snapshots/rusty__codegen__tests__code_gen_tests__returning_early_in_function_block.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ entry:
%load_n = load i8, i8* %n, align 1
%1 = sext i8 %load_n to i32
%tmpVar = icmp slt i32 %1, 10
br i1 %tmpVar, label %condition_body, label %continue
%2 = zext i1 %tmpVar to i8
%3 = icmp ne i8 %2, 0
br i1 %3, label %condition_body, label %continue

condition_body: ; preds = %entry
ret void
Expand Down
8 changes: 6 additions & 2 deletions ...degen/tests/snapshots/rusty__codegen__tests__code_gen_tests__while_loop_with_if_exit.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,19 @@ entry:
condition_check: ; preds = %entry, %continue3
%load_x = load i32, i32* %x, align 4
%tmpVar = icmp slt i32 %load_x, 20
br i1 %tmpVar, label %while_body, label %continue
%1 = zext i1 %tmpVar to i8
%2 = icmp ne i8 %1, 0
br i1 %2, label %while_body, label %continue

while_body: ; preds = %condition_check
%load_x1 = load i32, i32* %x, align 4
%tmpVar2 = add i32 %load_x1, 1
store i32 %tmpVar2, i32* %x, align 4
%load_x4 = load i32, i32* %x, align 4
%tmpVar5 = icmp sge i32 %load_x4, 10
br i1 %tmpVar5, label %condition_body, label %continue3
%3 = zext i1 %tmpVar5 to i8
%4 = icmp ne i8 %3, 0
br i1 %4, label %condition_body, label %continue3

continue: ; preds = %condition_body, %condition_check
ret void
Expand Down
4 changes: 3 additions & 1 deletion ...sts/snapshots/rusty__codegen__tests__code_gen_tests__while_with_expression_statement.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ condition_check: ; preds = %entry, %while_body
%load_x = load i8, i8* %x, align 1
%1 = zext i8 %load_x to i32
%tmpVar = icmp eq i32 %1, 0
br i1 %tmpVar, label %while_body, label %continue
%2 = zext i1 %tmpVar to i8
%3 = icmp ne i8 %2, 0
br i1 %3, label %while_body, label %continue

while_body: ; preds = %condition_check
%load_x1 = load i8, i8* %x, align 1
Expand Down
190 changes: 89 additions & 101 deletions src/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,12 @@ pub struct VisitorContext<'s> {
/// e.g. true for `a.b.c` if either a,b or c is declared in a constant block
constant: bool,

/// true the visitor entered a body (so no declarations)
/// true if the visitor entered a body (so no declarations)
in_body: bool,

/// true if the visitor entered a control statement
in_control: bool,

pub id_provider: IdProvider,

// what's the current strategy for resolving
Expand All @@ -87,80 +90,54 @@ pub struct VisitorContext<'s> {
impl<'s> VisitorContext<'s> {
/// returns a copy of the current context and changes the `current_qualifier` to the given qualifier
fn with_qualifier(&self, qualifier: String) -> VisitorContext<'s> {
VisitorContext {
pou: self.pou,
qualifier: Some(qualifier),
lhs: self.lhs,
constant: false,
in_body: self.in_body,
id_provider: self.id_provider.clone(),
resolve_strategy: self.resolve_strategy.clone(),
}
let mut ctx = self.clone();
ctx.qualifier = Some(qualifier);
ctx.constant = false;
ctx
}

/// returns a copy of the current context and changes the `current_pou` to the given pou
fn with_pou(&self, pou: &'s str) -> VisitorContext<'s> {
VisitorContext {
pou: Some(pou),
qualifier: self.qualifier.clone(),
lhs: self.lhs,
constant: false,
in_body: self.in_body,
id_provider: self.id_provider.clone(),
resolve_strategy: self.resolve_strategy.clone(),
}
let mut ctx = self.clone();
ctx.pou = Some(pou);
ctx.constant = false;
ctx
}

/// returns a copy of the current context and changes the `lhs_pou` to the given pou
fn with_lhs(&self, lhs_pou: &'s str) -> VisitorContext<'s> {
VisitorContext {
pou: self.pou,
qualifier: self.qualifier.clone(),
lhs: Some(lhs_pou),
constant: false,
in_body: self.in_body,
id_provider: self.id_provider.clone(),
resolve_strategy: self.resolve_strategy.clone(),
}
let mut ctx = self.clone();
ctx.lhs = Some(lhs_pou);
ctx.constant = false;
ctx
}

/// returns a copy of the current context and changes the `is_call` to true
fn with_const(&self, const_state: bool) -> VisitorContext<'s> {
VisitorContext {
pou: self.pou,
qualifier: self.qualifier.clone(),
lhs: self.lhs,
constant: const_state,
in_body: self.in_body,
id_provider: self.id_provider.clone(),
resolve_strategy: self.resolve_strategy.clone(),
}
let mut ctx = self.clone();
ctx.constant = const_state;
ctx
}

// returns a copy of the current context and sets the in_body field to true
fn enter_body(&self) -> Self {
VisitorContext {
pou: self.pou,
qualifier: self.qualifier.clone(),
lhs: self.lhs,
constant: self.constant,
in_body: true,
id_provider: self.id_provider.clone(),
resolve_strategy: self.resolve_strategy.clone(),
}
let mut ctx = self.clone();
ctx.in_body = true;
ctx
}

fn enter_control(&self) -> Self {
let mut ctx = self.clone();
ctx.in_control = true;
ctx
}

// returns a copy of the current context and sets the resolve_strategy field to the given strategies
fn with_resolving_strategy(&self, resolve_strategy: Vec<ResolvingScope>) -> Self {
VisitorContext {
pou: self.pou,
qualifier: self.qualifier.clone(),
lhs: self.lhs,
constant: self.constant,
in_body: true,
id_provider: self.id_provider.clone(),
resolve_strategy,
}
let mut ctx = self.clone();
ctx.in_body = true;
ctx.resolve_strategy = resolve_strategy;
ctx
}

fn is_in_a_body(&self) -> bool {
Expand Down Expand Up @@ -762,6 +739,7 @@ impl<'i> TypeAnnotator<'i> {
in_body: false,
id_provider,
resolve_strategy: ResolvingScope::default_scopes(),
in_control: false,
};

for global_variable in unit.global_vars.iter().flat_map(|it| it.variables.iter()) {
Expand Down Expand Up @@ -1244,54 +1222,56 @@ impl<'i> TypeAnnotator<'i> {
self.visit_statement(ctx, expr);
self.inherit_annotations(statement, expr);
}
AstStatement::ControlStatement(AstControlStatement::If(stmt), ..) => {
stmt.blocks.iter().for_each(|b| {
self.visit_statement(ctx, b.condition.as_ref());
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
});
stmt.else_block.iter().for_each(|e| self.visit_statement(ctx, e));
}
AstStatement::ControlStatement(AstControlStatement::ForLoop(stmt), ..) => {
visit_all_statements!(self, ctx, &stmt.counter, &stmt.start, &stmt.end);
if let Some(by_step) = &stmt.by_step {
self.visit_statement(ctx, by_step);
}
//Hint annotate start, end and step with the counter's real type
if let Some(type_name) = self
.annotation_map
.get_type(&stmt.counter, self.index)
.map(typesystem::DataType::get_name)
{
let annotation = StatementAnnotation::value(type_name);
self.annotation_map.annotate_type_hint(&stmt.start, annotation.clone());
self.annotation_map.annotate_type_hint(&stmt.end, annotation.clone());
if let Some(by_step) = &stmt.by_step {
self.annotation_map.annotate_type_hint(by_step, annotation);
AstStatement::ControlStatement(control) => {
match control {
AstControlStatement::If(stmt) => {
stmt.blocks.iter().for_each(|b| {
self.visit_statement(&ctx.enter_control(), b.condition.as_ref());
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
});
stmt.else_block.iter().for_each(|e| self.visit_statement(ctx, e));
}
}
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
}
AstStatement::ControlStatement(AstControlStatement::WhileLoop(stmt), ..)
| AstStatement::ControlStatement(AstControlStatement::RepeatLoop(stmt), ..) => {
self.visit_statement(ctx, &stmt.condition);
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
}
AstStatement::ControlStatement(AstControlStatement::Case(stmt), ..) => {
self.visit_statement(ctx, &stmt.selector);
let selector_type = self.annotation_map.get_type(&stmt.selector, self.index).cloned();
stmt.case_blocks.iter().for_each(|b| {
self.visit_statement(ctx, b.condition.as_ref());
if let Some(selector_type) = &selector_type {
self.update_expected_types(selector_type, b.condition.as_ref());
AstControlStatement::ForLoop(stmt) => {
visit_all_statements!(self, ctx, &stmt.counter, &stmt.start, &stmt.end);
if let Some(by_step) = &stmt.by_step {
self.visit_statement(ctx, by_step);
}
//Hint annotate start, end and step with the counter's real type
if let Some(type_name) = self
.annotation_map
.get_type(&stmt.counter, self.index)
.map(typesystem::DataType::get_name)
{
let annotation = StatementAnnotation::value(type_name);
self.annotation_map.annotate_type_hint(&stmt.start, annotation.clone());
self.annotation_map.annotate_type_hint(&stmt.end, annotation.clone());
if let Some(by_step) = &stmt.by_step {
self.annotation_map.annotate_type_hint(by_step, annotation);
}
}
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
}
AstControlStatement::WhileLoop(stmt) | AstControlStatement::RepeatLoop(stmt) => {
self.visit_statement(&ctx.enter_control(), &stmt.condition);
stmt.body.iter().for_each(|s| self.visit_statement(ctx, s));
}
AstControlStatement::Case(stmt) => {
self.visit_statement(ctx, &stmt.selector);
let selector_type = self.annotation_map.get_type(&stmt.selector, self.index).cloned();
stmt.case_blocks.iter().for_each(|b| {
self.visit_statement(ctx, b.condition.as_ref());
if let Some(selector_type) = &selector_type {
self.update_expected_types(selector_type, b.condition.as_ref());
}
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
});
stmt.else_block.iter().for_each(|s| self.visit_statement(ctx, s));
}
b.body.iter().for_each(|s| self.visit_statement(ctx, s));
});
stmt.else_block.iter().for_each(|s| self.visit_statement(ctx, s));
}
}

AstStatement::CaseCondition(condition, ..) => self.visit_statement(ctx, condition),
_ => {
self.visit_statement_expression(ctx, statement);
}
_ => self.visit_statement_expression(ctx, statement),
}
}

Expand Down Expand Up @@ -1399,7 +1379,15 @@ impl<'i> TypeAnnotator<'i> {
};

if let Some(statement_type) = statement_type {
self.annotate(statement, StatementAnnotation::value(statement_type));
self.annotate(statement, StatementAnnotation::value(statement_type.clone()));

// https://github.com/PLC-lang/rusty/issues/939: We rely on type-hints in order
// to identify `=` operations that have no effect (e.g. `foo = bar;`) hence
// type-hint the conditions of control statements to eliminate false-positives.
if ctx.in_control {
self.annotation_map
.annotate_type_hint(statement, StatementAnnotation::value(statement_type))
}
}
}
AstStatement::UnaryExpression(data, ..) => {
Expand Down Expand Up @@ -1435,7 +1423,7 @@ impl<'i> TypeAnnotator<'i> {
visit_all_statements!(self, ctx, &data.start, &data.end);
}
AstStatement::Assignment(data, ..) => {
self.visit_statement(ctx, &data.right);
self.visit_statement(&ctx.enter_control(), &data.right);
if let Some(lhs) = ctx.lhs {
//special context for left hand side
self.visit_statement(&ctx.with_pou(lhs).with_lhs(lhs), &data.left);
Expand Down
Loading