Skip to content

Commit

Permalink
fix array element type algorithm to avoid selecting never (#6511)
Browse files Browse the repository at this point in the history
## Description

This PR fixes the array element type selection algorithm to avoid
selecting `Never`.

## Checklist

- [ ] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] 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)
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] 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.
  • Loading branch information
xunilrj authored Sep 23, 2024
1 parent 1d50da1 commit 3f163c7
Show file tree
Hide file tree
Showing 11 changed files with 503 additions and 266 deletions.
74 changes: 45 additions & 29 deletions sway-core/src/language/ty/expression/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,35 +123,8 @@ impl TypeCheckAnalysis for TyExpression {
}
}
}
// Check all array items are the same
TyExpressionVariant::Array {
elem_type,
contents,
} => {
let array_elem_type = ctx.engines.te().get(*elem_type);
if !matches!(&*array_elem_type, TypeInfo::Never) {
let unify = crate::type_system::unify::unifier::Unifier::new(
ctx.engines,
"",
unify::unifier::UnifyKind::Default,
);
for element in contents {
let element_type = ctx.engines.te().get(element.return_type);

// If the element is never, we do not need to check
if matches!(&*element_type, TypeInfo::Never) {
continue;
}

unify.unify(
handler,
element.return_type,
*elem_type,
&element.span,
true,
)
}
}
TyExpressionVariant::Array { .. } => {
self.as_array_unify_elements(handler, ctx.engines);
}
_ => {}
}
Expand Down Expand Up @@ -500,4 +473,47 @@ impl TyExpression {
_ => None,
}
}

/// Unify elem_type with each element return type.
/// Must be called on arrays.
pub fn as_array_unify_elements(&self, handler: &Handler, engines: &Engines) {
let TyExpressionVariant::Array {
elem_type,
contents,
} = &self.expression
else {
unreachable!("Should only be called on Arrays")
};

let array_elem_type = engines.te().get(*elem_type);
if !matches!(&*array_elem_type, TypeInfo::Never) {
let unify = crate::type_system::unify::unifier::Unifier::new(
engines,
"",
unify::unifier::UnifyKind::Default,
);
for element in contents {
let element_type = engines.te().get(element.return_type);

// If the element is never, we do not need to check
if matches!(&*element_type, TypeInfo::Never) {
continue;
}

let h = Handler::default();
unify.unify(&h, element.return_type, *elem_type, &element.span, true);

// unification error points to type that failed
// we want to report the element type instead
if h.has_errors() {
handler.emit_err(CompileError::TypeError(TypeError::MismatchedType {
expected: format!("{:?}", engines.help_out(&array_elem_type)),
received: format!("{:?}", engines.help_out(element_type)),
help_text: String::new(),
span: element.span.clone(),
}));
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ use crate::{
use ast_node::declaration::{insert_supertraits_into_namespace, SupertraitOf};
use either::Either;
use indexmap::IndexMap;
use itertools::Itertools;
use rustc_hash::FxHashSet;
use std::collections::{HashMap, VecDeque};
use sway_ast::intrinsics::Intrinsic;
Expand Down Expand Up @@ -1947,7 +1946,7 @@ impl ty::TyExpression {
}

fn type_check_array(
handler: &Handler,
_handler: &Handler,
mut ctx: TypeCheckContext,
contents: &[Expression],
span: Span,
Expand Down Expand Up @@ -1979,8 +1978,8 @@ impl ty::TyExpression {
});
};

// start each element with the known array element type, or Unknown if it is to be inferred
// from the elements
// capture the expected array element type from context,
// otherwise, fallback to Unknown.
let initial_type = match &*ctx.engines().te().get(ctx.type_annotation()) {
TypeInfo::Array(element_type, _) => {
let element_type = (*ctx.engines().te().get(element_type.type_id)).clone();
Expand All @@ -2001,44 +2000,32 @@ impl ty::TyExpression {
.by_ref()
.with_help_text("")
.with_type_annotation(type_engine.insert(engines, initial_type.clone(), None));
Self::type_check(handler, ctx, expr)

// type_check_analyze unification will give the final error
let handler = Handler::default();
Self::type_check(&handler, ctx, expr)
.unwrap_or_else(|err| ty::TyExpression::error(err, span, engines))
})
.collect();

// choose the best type to be the array elem type
use itertools::FoldWhile::{Continue, Done};
let elem_type = typed_contents
.iter()
.fold_while(None, |last, current| match last {
None => Continue(Some(current.return_type)),
Some(last) => {
if last.is_concrete(engines, TreatNumericAs::Abstract) {
return Done(Some(last));
}

if current
.return_type
.is_concrete(engines, TreatNumericAs::Abstract)
{
return Done(Some(current.return_type));
}

let last_info = ctx.engines().te().get(last);
let current_info = ctx.engines().te().get(current.return_type);
match (&*last_info, &*current_info) {
(TypeInfo::Numeric, TypeInfo::UnsignedInteger(_)) => {
Done(Some(current.return_type))
}
_ => Continue(Some(last)),
}
}
})
.into_inner();
let elem_type = elem_type.unwrap_or_else(|| typed_contents[0].return_type);
// if the element type is still unknown, and all elements are Never,
// fallback to unit
let initial_type = if matches!(initial_type, TypeInfo::Unknown) {
let is_all_elements_never = typed_contents
.iter()
.all(|expr| matches!(&*engines.te().get(expr.return_type), TypeInfo::Never));
if is_all_elements_never {
TypeInfo::Tuple(vec![])
} else {
initial_type
}
} else {
initial_type
};

let elem_type = type_engine.insert(engines, initial_type.clone(), None);
let array_count = typed_contents.len();
Ok(ty::TyExpression {
let expr = ty::TyExpression {
expression: ty::TyExpressionVariant::Array {
elem_type,
contents: typed_contents,
Expand All @@ -2055,9 +2042,15 @@ impl ty::TyExpression {
Length::new(array_count, Span::dummy()),
),
None,
), // Maybe?
),
span,
})
};

// type_check_analyze unification will give the final error
let handler = Handler::default();
expr.as_array_unify_elements(&handler, ctx.engines);

Ok(expr)
}

fn type_check_array_index(
Expand Down Expand Up @@ -2940,7 +2933,7 @@ mod tests {
expr: &Expression,
) -> Result<ty::TyExpression, ErrorEmitted> {
let engines = Engines::default();
do_type_check(
let expr = do_type_check(
handler,
&engines,
expr,
Expand All @@ -2960,7 +2953,9 @@ mod tests {
ExperimentalFlags {
new_encoding: false,
},
)
)?;
expr.type_check_analyze(handler, &mut TypeCheckAnalysisContext::new(&engines))?;
Ok(expr)
}

#[test]
Expand Down Expand Up @@ -3021,21 +3016,14 @@ mod tests {
let _comp_res = do_type_check_for_boolx2(&handler, &expr);
let (errors, _warnings) = handler.consume();

assert!(errors.len() == 2);
assert!(errors.len() == 1);
assert!(matches!(&errors[0],
CompileError::TypeError(TypeError::MismatchedType {
expected,
received,
..
}) if expected == "bool"
&& received == "u64"));
assert!(matches!(&errors[1],
CompileError::TypeError(TypeError::MismatchedType {
expected,
received,
..
}) if expected == "[bool; 2]"
&& received == "[u64; 2]"));
}

#[test]
Expand Down
46 changes: 20 additions & 26 deletions sway-core/src/type_system/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -463,33 +463,27 @@ impl TypeId {
}))
}

pub(crate) fn is_concrete(
&self,
engines: &Engines,
numeric_non_concrete: TreatNumericAs,
) -> bool {
pub(crate) fn is_concrete(&self, engines: &Engines, treat_numeric_as: TreatNumericAs) -> bool {
let nested_types = (*self).extract_nested_types(engines);
!nested_types
.into_iter()
.any(|x| match numeric_non_concrete {
TreatNumericAs::Abstract => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
| TypeInfo::Numeric
),
TreatNumericAs::Concrete => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
),
})
!nested_types.into_iter().any(|x| match treat_numeric_as {
TreatNumericAs::Abstract => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
| TypeInfo::Numeric
),
TreatNumericAs::Concrete => matches!(
x,
TypeInfo::UnknownGeneric { .. }
| TypeInfo::Custom { .. }
| TypeInfo::Placeholder(..)
| TypeInfo::TraitType { .. }
| TypeInfo::TypeParam(..)
),
})
}

/// `check_type_parameter_bounds` does two types of checks. Lets use the example below for demonstrating the two checks:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[[package]]
name = "array_wrong_elements_types"
source = "member"
dependencies = [
"core",
"std",
]

[[package]]
name = "core"
source = "path+from-root-C125AFC4EC59A434"

[[package]]
name = "std"
source = "path+from-root-C125AFC4EC59A434"
dependencies = ["core"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[project]
authors = ["Fuel Labs <contact@fuel.sh>"]
license = "Apache-2.0"
name = "array_wrong_elements_types"
entry = "main.sw"
implicit-std = false

[dependencies]
core = { path = "../../../../../../sway-lib-core" }
std = { path = "../../../../../../sway-lib-std" }
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
script;

fn vec<T>() -> Vec<T> {
Vec::new()
}

fn main() {
// unexpected u16
let a: [u8;1] = [1u16];

// unexpected u16
let _ = [1u8, 1u16];
let a = [1, 2u8, 3u16, 4u32, 5u64];

// unexpected u8
let _ = [1, 1u16, a[0]];

// unexpected string slice
let _ = [1, "", 1u8, 1u16];

// unexpected u8
let _ = [return, 1u8, 1u16];

// unexpected u16
let _ = [1u8, return, 1u16];

// unexpected u16
let _ = [1, return, 1u8, 1u16];

// unexpected str
let _ = [1, "", 1u16];
let _ = [1, 2, "hello"];
let _ = [1, return, "", 1u16];
let _ = [1, "", return, 1u16];

// unexpected Vec<u16>
let _ = [Vec::new(), vec::<u8>(), vec::<u16>()];

// unexpected Option<u8>
let a = [None, Some(1), Some(1u8)];
let _b: Option<u16> = a[1];

// unexpected u8
let a = [8, 256u16, 8u8];
let b: u32 = a[2];

// Should not warn or error
let _ : [u8 ; 0] = [];
}
Loading

0 comments on commit 3f163c7

Please # to comment.