Skip to content

Commit

Permalink
Allow pure statements that contain early returns
Browse files Browse the repository at this point in the history
  • Loading branch information
@smores56
smores56 committed Nov 21, 2024
1 parent 6a3db1e commit d449d83
Show file tree
Hide file tree
Showing 3 changed files with 283 additions and 9 deletions.
100 changes: 100 additions & 0 deletions crates/compiler/can/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,106 @@ impl Expr {
Self::TypedHole(_) | Self::RuntimeError(..) => Category::Unknown,
}
}

pub fn contains_any_early_returns(&self) -> bool {
match self {
Self::Num { .. }
| Self::Int { .. }
| Self::Float { .. }
| Self::Str { .. }
| Self::IngestedFile { .. }
| Self::SingleQuote { .. }
| Self::Var { .. }
| Self::AbilityMember { .. }
| Self::ParamsVar { .. }
| Self::Closure(..)
| Self::EmptyRecord
| Self::RecordAccessor(_)
| Self::ZeroArgumentTag { .. }
| Self::OpaqueWrapFunction(_) => false,
Self::Return { .. } => true,
Self::TypedHole(_) | Self::RuntimeError(..) => false,
Self::List { loc_elems, .. } => loc_elems
.iter()
.any(|elem| elem.value.contains_any_early_returns()),
Self::When {
loc_cond, branches, ..
} => {
loc_cond.value.contains_any_early_returns()
|| branches.iter().any(|branch| {
branch
.guard
.as_ref()
.is_some_and(|guard| guard.value.contains_any_early_returns())
|| branch.value.value.contains_any_early_returns()
})
}
Self::If {
branches,
final_else,
..
} => {
final_else.value.contains_any_early_returns()
|| branches.iter().any(|(cond, then)| {
cond.value.contains_any_early_returns()
|| then.value.contains_any_early_returns()
})
}
Self::LetRec(defs, expr, _cycle_mark) => {
expr.value.contains_any_early_returns()
|| defs
.iter()
.any(|def| def.loc_expr.value.contains_any_early_returns())
}
Self::LetNonRec(def, expr) => {
def.loc_expr.value.contains_any_early_returns()
|| expr.value.contains_any_early_returns()
}
Self::Call(_func, args, _called_via) => args
.iter()
.any(|(_var, arg_expr)| arg_expr.value.contains_any_early_returns()),
Self::RunLowLevel { args, .. } | Self::ForeignCall { args, .. } => args
.iter()
.any(|(_var, arg_expr)| arg_expr.contains_any_early_returns()),
Self::Tuple { elems, .. } => elems
.iter()
.any(|(_var, loc_elem)| loc_elem.value.contains_any_early_returns()),
Self::Record { fields, .. } => fields
.iter()
.any(|(_field_name, field)| field.loc_expr.value.contains_any_early_returns()),
Self::RecordAccess { loc_expr, .. } => loc_expr.value.contains_any_early_returns(),
Self::TupleAccess { loc_expr, .. } => loc_expr.value.contains_any_early_returns(),
Self::RecordUpdate { updates, .. } => {
updates.iter().any(|(_field_name, field_update)| {
field_update.loc_expr.value.contains_any_early_returns()
})
}
Self::ImportParams(_module_id, _region, params) => params
.as_ref()
.is_some_and(|(_var, p)| p.contains_any_early_returns()),
Self::Tag { arguments, .. } => arguments
.iter()
.any(|(_var, arg)| arg.value.contains_any_early_returns()),
Self::OpaqueRef { argument, .. } => argument.1.value.contains_any_early_returns(),
Self::Crash { msg, .. } => msg.value.contains_any_early_returns(),
Self::Dbg {
loc_message,
loc_continuation,
..
} => {
loc_message.value.contains_any_early_returns()
|| loc_continuation.value.contains_any_early_returns()
}
Self::Expect {
loc_condition,
loc_continuation,
..
} => {
loc_condition.value.contains_any_early_returns()
|| loc_continuation.value.contains_any_early_returns()
}
}
}
}

/// Stores exhaustiveness-checking metadata for a closure argument that may
Expand Down
29 changes: 20 additions & 9 deletions crates/compiler/constrain/src/expr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3394,12 +3394,18 @@ fn constrain_let_def(
)
});

// Ignored def must be effectful, otherwise it's dead code
let effectful_constraint = Constraint::ExpectEffectful(
fx_var,
ExpectEffectfulReason::Ignored,
def.loc_pattern.region,
);
let effectful_constraint = if def.loc_expr.value.contains_any_early_returns() {
// If the statement has early returns, it doesn't need to be effectful to
// potentially affect the output of the containing function
Constraint::True
} else {
// If there are no early returns, it must be effectful or else it's dead code
Constraint::ExpectEffectful(
fx_var,
ExpectEffectfulReason::Ignored,
def.loc_pattern.region,
)
};

let enclosing_fx_constraint = constraints.fx_call(
fx_var,
Expand Down Expand Up @@ -3486,9 +3492,14 @@ fn constrain_stmt_def(
generalizable,
);

// Stmt expr must be effectful, otherwise it's dead code
let effectful_constraint =
Constraint::ExpectEffectful(fx_var, ExpectEffectfulReason::Stmt, region);
let effectful_constraint = if def.loc_expr.value.contains_any_early_returns() {
// If the statement has early returns, it doesn't need to be effectful to
// potentially affect the output of the containing function
Constraint::True
} else {
// If there are no early returns, it must be effectful or else it's dead code
Constraint::ExpectEffectful(fx_var, ExpectEffectfulReason::Stmt, region)
};

let fx_call_kind = match fn_name {
None => FxCallKind::Stmt,
Expand Down
163 changes: 163 additions & 0 deletions crates/compiler/load/tests/test_reporting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14655,6 +14655,169 @@ All branches in an `if` must have the same type!
"###
);

test_report!(
try_in_bare_statement,
indoc!(
r#"
app [main!] { pf: platform "../../../../../crates/cli/tests/test-projects/test-platform-effects-zig/main.roc" }
import pf.Effect
validateNum = \num ->
if num > 5 then
Ok {}
else
Err TooBig
main! = \{} ->
Effect.putLine! "hello"
# this returns {}, so it's ignored
try validateNum 10
# this returns a value, so we are incorrectly
# dropping the parsed value
try List.get [1, 2, 3] 5
Ok {}
"#
),
@r###"
── IGNORED RESULT in /code/proj/Main.roc ───────────────────────────────────────
The result of this expression is ignored:
19│ try List.get [1, 2, 3] 5
^^^^^^^^^^^^^^^^^^^^^^^^
Standalone statements are required to produce an empty record, but the
type of this one is:
Num *
If you still want to ignore it, assign it to `_`, like this:
_ = File.delete! "data.json"
"###
);

test_report!(
try_with_ignored_output,
indoc!(
r#"
app [main!] { pf: platform "../../../../../crates/cli/tests/test-projects/test-platform-effects-zig/main.roc" }
import pf.Effect
main! = \{} ->
Effect.putLine! "hello"
# not ignored, warning
try List.get [1, 2, 3] 5
# ignored, OK
_ = try List.get [1, 2, 3] 5
_ignored = try List.get [1, 2, 3] 5
Ok {}
"#
),
@r###"
── IGNORED RESULT in /code/proj/Main.roc ───────────────────────────────────────
The result of this expression is ignored:
9│ try List.get [1, 2, 3] 5
^^^^^^^^^^^^^^^^^^^^^^^^
Standalone statements are required to produce an empty record, but the
type of this one is:
Num *
If you still want to ignore it, assign it to `_`, like this:
_ = File.delete! "data.json"
"###
);

test_report!(
no_early_return_in_bare_statement,
indoc!(
r#"
app [main!] { pf: platform "../../../../../crates/cli/tests/test-projects/test-platform-effects-zig/main.roc" }
import pf.Effect
main! = \{} ->
Effect.putLine! "hello"
Num.toStr 123
Ok {}
"#
),
@r#"
── IGNORED RESULT in /code/proj/Main.roc ───────────────────────────────────────
The result of this call to `Num.toStr` is ignored:
8│ Num.toStr 123
^^^^^^^^^
Standalone statements are required to produce an empty record, but the
type of this one is:
Str
If you still want to ignore it, assign it to `_`, like this:
_ = File.delete! "data.json"
── LEFTOVER STATEMENT in /code/proj/Main.roc ───────────────────────────────────
This statement does not produce any effects:
8│ Num.toStr 123
^^^^^^^^^^^^^
Standalone statements are only useful if they call effectful
functions.
Did you forget to use its result? If not, feel free to remove it.
"#
);

test_report!(
no_early_return_in_ignored_statement,
indoc!(
r#"
app [main!] { pf: platform "../../../../../crates/cli/tests/test-projects/test-platform-effects-zig/main.roc" }
import pf.Effect
main! = \{} ->
Effect.putLine! "hello"
_ignored = Num.toStr 123
Ok {}
"#
),
@r"
── UNNECESSARY DEFINITION in /code/proj/Main.roc ───────────────────────────────
This assignment doesn't introduce any new variables:
8│ _ignored = Num.toStr 123
^^^^^^^^
Since it doesn't call any effectful functions, this assignment cannot
affect the program's behavior. If you don't need to use the value on
the right-hand side, consider removing the assignment.
"
);

test_report!(
leftover_statement,
indoc!(
Expand Down

0 comments on commit d449d83

Please sign in to comment.