Skip to content

Commit 8788257

Browse files
jjcnnJoshuaBattyIGI-111
authored
Respect variable type ascription when initializer has type Never (#6911)
## Description Fixes #6391. This PR fixes an issue with variable declarations whose initializers diverge because they contain `return`, `continue` or `break` expressions. In this case the initializer typechecks to `Never`, and the variable is inferred to have type `Never` in the subsequent (unreachable) code: ``` let mut x = return; x = 42; // Illegal, since x has type Never ``` The problem is that this is the case even when the variable declaration has a type ascription: ``` let mut x : u32 = return; x = 42; // Should be legal, since x has the type ascription u32 ``` As part of the fix there is another slight change of behavior for code blocks that diverge. Until now the type of a code block has been determined based on whether it contains a diverging expression: ``` match { return; true } // Considered to have type Never because of `return;` { // Type Never does not have a constructor } ``` We now determine the type of a code block based on its implicit return: ``` match { return; true } { // Type Boolean has two constructors true => ..., false => ..., } ``` In principle this is a breaking change, but the only change in behavior is in code blocks that contain a `return`, `break` or `continue`, and then contains dead code after that expression, so I can't imagine anyone will be affected by it. ## 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). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [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: Joshua Batty <joshpbatty@gmail.com> Co-authored-by: IGI-111 <igi-111@protonmail.com>
1 parent ac95d2d commit 8788257

File tree

8 files changed

+74
-24
lines changed

8 files changed

+74
-24
lines changed

sway-core/src/language/ty/declaration/declaration.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -734,7 +734,7 @@ impl TyDecl {
734734
let type_engine = engines.te();
735735
let decl_engine = engines.de();
736736
let type_id = match self {
737-
TyDecl::VariableDecl(decl) => decl.body.return_type,
737+
TyDecl::VariableDecl(decl) => decl.return_type,
738738
TyDecl::FunctionDecl(FunctionDecl { decl_id, .. }) => {
739739
let decl = decl_engine.get_function(decl_id);
740740
decl.return_type.type_id

sway-core/src/semantic_analysis/ast_node/declaration/variable.rs

+29-22
Original file line numberDiff line numberDiff line change
@@ -49,37 +49,44 @@ impl ty::TyVariableDecl {
4949
None,
5050
)
5151
.unwrap_or_else(|err| type_engine.id_of_error_recovery(err));
52+
5253
let mut ctx = ctx
5354
.with_type_annotation(type_ascription.type_id)
5455
.with_help_text(
5556
"Variable declaration's type annotation does not match up \
56-
with the assigned expression's type.",
57+
with the assigned expression's type.",
5758
);
59+
5860
let result = ty::TyExpression::type_check(handler, ctx.by_ref(), &var_decl.body);
5961
let body = result
6062
.unwrap_or_else(|err| ty::TyExpression::error(err, var_decl.name.span(), engines));
6163

62-
// TODO: Integers shouldn't be anything special. RHS expressions should be written in
63-
// a way to always use the context provided from the LHS, and if the LHS is
64-
// an integer, RHS should properly unify or type check should fail.
65-
// Remove this special case as a part of the initiative of improving type inference.
66-
// Integers are special in the sense that we can't only rely on the type of `body`
67-
// to get the type of the variable. The type of the variable *has* to follow
68-
// `type_ascription` if `type_ascription` is a concrete integer type that does not
69-
// conflict with the type of `body` (i.e. passes the type checking above).
70-
let return_type = match &*type_engine.get(type_ascription.type_id) {
71-
TypeInfo::UnsignedInteger(_) => type_ascription.type_id,
72-
_ => match &*type_engine.get(body.return_type) {
73-
// If RHS type check ends up in an error we want to use the
74-
// provided type ascription as the variable type. E.g.:
75-
// let v: Struct<u8> = Struct<u64> { x: 0 }; // `v` should be "Struct<u8>".
76-
// let v: ExistingType = non_existing_identifier; // `v` should be "ExistingType".
77-
// let v = <some error>; // `v` will remain "{unknown}".
78-
// TODO: Refine and improve this further. E.g.,
79-
// let v: Struct { /* MISSING FIELDS */ }; // Despite the error, `v` should be of type "Struct".
80-
TypeInfo::ErrorRecovery(_) => type_ascription.type_id,
81-
_ => body.return_type,
82-
},
64+
// Determine the type of the variable going forward. Typically this is the type of the RHS,
65+
// but in some cases we need to use the type ascription instead.
66+
// TODO: We should not have these special cases. The typecheck expressions should be written
67+
// in a way to always use the context provided by the LHS, and use the unified type of LHS
68+
// and RHS as the return type of the RHS. Remove this special case as a part of the
69+
// initiative of improving type inference.
70+
let return_type = match (&*type_engine.get(type_ascription.type_id), &*type_engine.get(body.return_type)) {
71+
// Integers: We can't rely on the type of the RHS to give us the correct integer
72+
// type, so the type of the variable *has* to follow `type_ascription` if
73+
// `type_ascription` is a concrete integer type that does not conflict with the type
74+
// of `body` (i.e. passes the type checking above).
75+
(TypeInfo::UnsignedInteger(_), _) |
76+
// Never: If the RHS resolves to Never, then any code following the declaration is
77+
// unreachable. If the variable is used later on, then it should be treated as
78+
// having the same type as the type ascription.
79+
(_, TypeInfo::Never) |
80+
// If RHS type check ends up in an error we want to use the
81+
// provided type ascription as the variable type. E.g.:
82+
// let v: Struct<u8> = Struct<u64> { x: 0 }; // `v` should be "Struct<u8>".
83+
// let v: ExistingType = non_existing_identifier; // `v` should be "ExistingType".
84+
// let v = <some error>; // `v` will remain "{unknown}".
85+
// TODO: Refine and improve this further. E.g.,
86+
// let v: Struct { /* MISSING FIELDS */ }; // Despite the error, `v` should be of type "Struct".
87+
(_, TypeInfo::ErrorRecovery(_)) => type_ascription.type_id,
88+
// In all other cases we use the type of the RHS.
89+
_ => body.return_type,
8390
};
8491

8592
if !ctx.code_block_first_pass() {

sway-core/src/semantic_analysis/ast_node/expression/typed_expression.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -2427,7 +2427,7 @@ impl ty::TyExpression {
24272427
));
24282428
}
24292429

2430-
break (name, variable_decl.body.return_type);
2430+
break (name, variable_decl.return_type);
24312431
}
24322432
TyDecl::ConstantDecl(constant_decl) => {
24332433
let constant_decl =

test/src/e2e_vm_tests/test_programs/should_pass/language/diverging_exprs/src/main.sw

+1
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ fn diverge_in_match_condition() -> u64 {
8888
return 5;
8989
true
9090
} {
91+
_ => 42,
9192
}
9293
}
9394

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
[[package]]
2+
name = "core"
3+
source = "path+from-root-DFCFC85C94C0C9BD"
4+
5+
[[package]]
6+
name = "std"
7+
source = "path+from-root-DFCFC85C94C0C9BD"
8+
dependencies = ["core"]
9+
10+
[[package]]
11+
name = "unify_never"
12+
source = "member"
13+
dependencies = ["std"]
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
[project]
2+
authors = ["Fuel Labs <contact@fuel.sh>"]
3+
license = "Apache-2.0"
4+
name = "unify_never"
5+
entry = "main.sw"
6+
7+
[dependencies]
8+
std = { path = "../../../../reduced_std_libs/sway-lib-std-assert" }
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
script;
2+
3+
// Test of unification of the Never type
4+
5+
fn test_1(){
6+
let mut v1: u32 = 1;
7+
v1 = return;
8+
}
9+
10+
fn test_2(){
11+
let mut v2: u32 = return;
12+
v2 = 1;
13+
}
14+
15+
16+
fn main() {
17+
test_1();
18+
test_2();
19+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
category = "compile"
2+
expected_warnings = 1

0 commit comments

Comments
 (0)