Fix TyExpression::deterministically_aborts(), and eliminate its use during IR generation
#5438
Labels
bug
Something isn't working
compiler: frontend
Everything to do with type checking, control flow analysis, and everything between parsing and IRgen
compiler: ir
IRgen and sway-ir including optimization passes
Comments
jjcnn
added
bug
7 tasks
|
The function |
7 tasks
IGI-111
pushed a commit
that referenced
this issue
Jan 29, 2024
## Description This PR refactors how `Value` is used during IR conversion. The PR introduces `TerminatorValue` which forces the converter to handle diverging expressions properly. There were checks for divergence scattered around the code before, but it's now been made more systematic and thus more robust. Additionally, this change allows for early returns from some functions. For instance, if the condition of a `while` diverges, then we don't need to generate code for the body of the loop. In fact, if an expression does diverge then the algorithm assumes that a terminator value (e.g., a `branch` or a `return`) is returned, but this was not always the case, and led to a number of spurious checks that probably didn't quite work. The heavy lifting is done by the macro `return_on_termination_or_extract`, which acts as destructor of `TerminatorValue`s. Most changes are straightforward, but do review the functions `compile_lazy_op`, `compile_fn_call`, `compile_if`, `compile_while_loop`, `compile_var_decl` and `compile_array_expr` extra carefully, as they contain changes to the conversion algorithm. Mostly the changes are small, but they need an extra pair of eyes. In a few cases the algorithm was and still is a bit convoluted, but that's because the typechecker unhelpfully assigns `Unknown` instead of `Never` to diverging expressions, so the first sub-expression needs to be compiled and divergence checked before we can generate IR for anything that relies on the type of the expression. Finally, I have taken the liberty of removing `Value::is_diverging()` and using its alias `Value::is_terminator()` everywhere. The concept of divergence lives in the source language, whereas the concept of terminators lives in the IR, so `is_terminator` seems to be more appropriate at this stage of the compilation. Fixes #2051. Partial fix of #5438 (the call to `deterministically_aborts()` is removed). ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Igor Rončević <[email protected]>
19 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The function
TyExpression::deterministically_aborts()almost certainly computes the wrong result in some cases. The most obvious example isreturn break, which does not abort, but for whichdeterministically_aborts()returnstrue.Additionally,
deterministically_aborts()is only supposed to be used during typechecking, but it is currently being used when generating IR for variable declarations as well. During IR generation it should be sufficient to useValue::is_diverging(), but removing the single use causes an internal compiler error.Edit: #5496 has removed the call to
deterministically_aborts()from IR generation, so that part has been fixed.The text was updated successfully, but these errors were encountered: