Skip to content

Commit 5da71fa

Browse files
authored
Warn about and Remove statements without effect (#9474)
In code-blocks, only the final expressions value is actually used. All other expressions must have side effects or are otherwise useless. This usually indicates an error by the programmer, so emit a warning to notify the programmer about the issue. Closes #4954 Example from the associated issue: This Slint code ```slint FocusScope { key-pressed(event) => { if (event.text == "a") { accept } reject } } ``` Looks like it would return accept, but in fact, the statement has no effect. Changelog: The slint compiler now emits a warning if a statement is without effect
1 parent 277aa92 commit 5da71fa

File tree

4 files changed

+206
-12
lines changed

4 files changed

+206
-12
lines changed

internal/compiler/passes/resolving.rs

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ use smol_str::{SmolStr, ToSmolStr};
2020
use std::collections::{BTreeMap, HashMap};
2121
use std::rc::Rc;
2222

23+
mod remove_noop;
24+
2325
/// This represents a scope for the Component, where Component is the repeated component, but
2426
/// does not represent a component in the .slint file
2527
#[derive(Clone)]
@@ -211,13 +213,26 @@ impl Expression {
211213
let mut statements_or_exprs = node
212214
.children()
213215
.filter_map(|n| match n.kind() {
214-
SyntaxKind::Expression => Some(Self::from_expression_node(n.into(), ctx)),
215-
SyntaxKind::ReturnStatement => Some(Self::from_return_statement(n.into(), ctx)),
216-
SyntaxKind::LetStatement => Some(Self::from_let_statement(n.into(), ctx)),
216+
SyntaxKind::Expression => {
217+
Some((n.clone(), Self::from_expression_node(n.into(), ctx)))
218+
}
219+
SyntaxKind::ReturnStatement => {
220+
Some((n.clone(), Self::from_return_statement(n.into(), ctx)))
221+
}
222+
SyntaxKind::LetStatement => {
223+
Some((n.clone(), Self::from_let_statement(n.into(), ctx)))
224+
}
217225
_ => None,
218226
})
219227
.collect::<Vec<_>>();
220228

229+
remove_noop::remove_from_codeblock(&mut statements_or_exprs, ctx.diag);
230+
231+
let mut statements_or_exprs = statements_or_exprs
232+
.into_iter()
233+
.map(|(_node, statement_or_expr)| statement_or_expr)
234+
.collect::<Vec<_>>();
235+
221236
let exit_points_and_return_types = statements_or_exprs
222237
.iter()
223238
.enumerate()
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
// Copyright © SixtyFPS GmbH <[email protected]>
2+
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
3+
4+
use crate::{diagnostics::BuildDiagnostics, expression_tree::Expression, parser::SyntaxNode};
5+
6+
/// Remove all expressions that are proven to have no effect from the given Expressions.
7+
///
8+
/// This function assumes that the given Expressions will form the an [Expression::CodeBlock], and
9+
/// will therefore not modify the last Expression in the Vec, as that forms the result value of the
10+
/// CodeBlock itself.
11+
pub fn remove_from_codeblock(
12+
code_block: &mut Vec<(SyntaxNode, Expression)>,
13+
diagnostics: &mut BuildDiagnostics,
14+
) {
15+
if code_block.len() > 1 {
16+
// In a code block, only the last expression returns a value.
17+
// Therefore all other expressions inside the block are only useful if they have side
18+
// effects.
19+
//
20+
// Remove all expressions without side effects (except for the last one) and emit a
21+
// warning.
22+
//
23+
// Note: Iterate over the indices in reverse, so that all to-be-iterated indices remain
24+
// valid when removing items from the vector.
25+
for index in (0..(code_block.len() - 1)).rev() {
26+
let (node, expression) = &code_block[index];
27+
if without_side_effects(expression) {
28+
diagnostics.push_warning("Expression has no effect!".to_owned(), node);
29+
code_block.remove(index);
30+
}
31+
}
32+
}
33+
}
34+
35+
/// Returns whether the expression is certain to be without side effects.
36+
/// This function is conservative and may still return `false`, even if a given expression
37+
/// is without side effects.
38+
/// It is only guaranteed that if this function returns `true`, the expression definitely does not
39+
/// contain side effects.
40+
fn without_side_effects(expression: &Expression) -> bool {
41+
match expression {
42+
Expression::Condition { condition, true_expr, false_expr } => {
43+
without_side_effects(condition)
44+
&& without_side_effects(true_expr)
45+
&& without_side_effects(false_expr)
46+
}
47+
Expression::NumberLiteral(_, _) => true,
48+
Expression::StringLiteral(_) => true,
49+
Expression::BoolLiteral(_) => true,
50+
Expression::CodeBlock(expressions) => expressions.iter().all(without_side_effects),
51+
Expression::FunctionParameterReference { .. } => true,
52+
// Invalid and uncompiled expressions are unknown at this point, so default to
53+
// `false`, because they may have side-efffects.
54+
Expression::Invalid => false,
55+
Expression::Uncompiled(_) => false,
56+
// A property reference may cause re-evaluation of a property, which may result in
57+
// side effects
58+
Expression::PropertyReference(_) => false,
59+
Expression::ElementReference(_) => false,
60+
Expression::RepeaterIndexReference { .. } => true,
61+
Expression::RepeaterModelReference { .. } => true,
62+
Expression::StoreLocalVariable { .. } => false,
63+
Expression::ReadLocalVariable { .. } => true,
64+
Expression::StructFieldAccess { base, name: _ } => without_side_effects(&*base),
65+
Expression::ArrayIndex { array, index } => {
66+
without_side_effects(&*array) && without_side_effects(&*index)
67+
}
68+
// Note: This assumes that the cast itself does not have any side effects, which may not be
69+
// the case if custom casting rules are implemented.
70+
Expression::Cast { from, to: _ } => without_side_effects(from),
71+
// Note: Calling a *pure* function is without side effects, however
72+
// just from the expression, the purity of the function is not known.
73+
// We would need to resolve the function to determine its purity.
74+
Expression::FunctionCall { .. } => false,
75+
Expression::SelfAssignment { .. } => false,
76+
Expression::BinaryExpression { lhs, rhs, .. } => {
77+
without_side_effects(&*lhs) && without_side_effects(&*rhs)
78+
}
79+
Expression::UnaryOp { sub, op: _ } => without_side_effects(&*sub),
80+
Expression::ImageReference { .. } => true,
81+
Expression::Array { element_ty: _, values } => values.iter().all(without_side_effects),
82+
Expression::Struct { ty: _, values } => values.values().all(without_side_effects),
83+
Expression::PathData(_) => true,
84+
Expression::EasingCurve(_) => true,
85+
Expression::LinearGradient { angle, stops } => {
86+
without_side_effects(&angle)
87+
&& stops
88+
.iter()
89+
.all(|(start, end)| without_side_effects(start) && without_side_effects(end))
90+
}
91+
Expression::RadialGradient { stops } => stops
92+
.iter()
93+
.all(|(start, end)| without_side_effects(start) && without_side_effects(end)),
94+
Expression::ConicGradient { stops } => stops
95+
.iter()
96+
.all(|(start, end)| without_side_effects(start) && without_side_effects(end)),
97+
Expression::EnumerationValue(_) => true,
98+
// A return statement is never without side effects, as an important "side effect" is that
99+
// the current function stops at this point.
100+
Expression::ReturnStatement(_) => false,
101+
Expression::LayoutCacheAccess { .. } => false,
102+
Expression::ComputeLayoutInfo(_, _) => false,
103+
Expression::SolveLayout(_, _) => false,
104+
Expression::MinMax { ty: _, op: _, lhs, rhs } => {
105+
without_side_effects(lhs) && without_side_effects(rhs)
106+
}
107+
Expression::DebugHook { .. } => false,
108+
Expression::EmptyComponentFactory => false,
109+
}
110+
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Copyright © SixtyFPS GmbH <[email protected]>
2+
// SPDX-License-Identifier: GPL-3.0-only OR LicenseRef-Slint-Royalty-free-2.0 OR LicenseRef-Slint-Software-3.0
3+
4+
struct MyStruct {
5+
foo: int
6+
}
7+
8+
enum MyEnum {
9+
One,
10+
Two,
11+
Three
12+
}
13+
14+
export component Test {
15+
function useless_if(cond: bool) {
16+
if (cond) {
17+
// ^warning{Expression has no effect!}
18+
if (!cond) {
19+
// ^warning{Expression has no effect!}
20+
43
21+
}
22+
42
23+
}
24+
else {
25+
41
26+
}
27+
"hello world";
28+
// ^warning{Expression has no effect!}
29+
123;
30+
// ^warning{Expression has no effect!}
31+
true;
32+
// ^warning{Expression has no effect!}
33+
MyEnum.One;
34+
// ^warning{Expression has no effect!}
35+
{ x: 32, };
36+
// ^warning{Expression has no effect!}
37+
{ x: another_function(), };
38+
39+
let x = true;
40+
x;
41+
// ^warning{Expression has no effect!}
42+
true && false;
43+
// ^warning{Expression has no effect!}
44+
[1, 2, 3][4];
45+
// ^warning{Expression has no effect!}
46+
+1;
47+
// ^warning{Expression has no effect!}
48+
@image-url("../../../../../logo/slint-logo-full-dark.png");
49+
// ^warning{Expression has no effect!}
50+
51+
another_function();
52+
53+
[1, 2, 3][another_function()];
54+
// FIXME: The above could return an "Expression has no effect" warning
55+
// on the [1, 2, 3] part, as well as the indexing, as only the call
56+
// to another_function could have an effect.
57+
58+
make_struct().foo;
59+
// FIXME: The above could return an "Expression has no effect" warning
60+
// on the .foo part, as only the call to make_struct() could have an effect,
61+
// not the member access itself.
62+
63+
12
64+
}
65+
66+
function another_function() -> int { 1 }
67+
68+
function make_struct() -> MyStruct {
69+
{
70+
foo: 5,
71+
}
72+
}
73+
74+
}

tests/cases/issues/issue_4942_no_else_value.slint

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ export component TestCase {
88
if (cond) {
99
45
1010
}
11+
// FIXME: Removing the following debug statement will cause a failure in C++ with this warning:
12+
// error: unused parameter 'arg_0' [-Werror=unused-parameter]
13+
// 115 | inline auto TestCase::fn_issue4942 (bool arg_0) const -> int{
14+
debug(cond);
1115
12
1216
}
1317

@@ -17,15 +21,6 @@ export component TestCase {
1721

1822
/*
1923

20-
FIXME: The C++ test currently fails with this warning
21-
(see also https://github.com/slint-ui/slint/issues/4954)
22-
//ignore: cpp
23-
24-
(This is the warning, for reference)
25-
statement has no effect [-Werror=unused-value]
26-
| return [&]{ [&]() -> void { if (arg_0) { 45; } else { ; }}();return 12; }();
27-
| ^~
28-
2924
```cpp
3025
auto handle = TestCase::create();
3126
const TestCase &instance = *handle;

0 commit comments

Comments
 (0)