Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix array element type algorithm to avoid selecting never #6511

Merged
merged 5 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -1809,7 +1808,7 @@ impl ty::TyExpression {
}

fn type_check_array(
handler: &Handler,
_handler: &Handler,
mut ctx: TypeCheckContext,
contents: &[Expression],
span: Span,
Expand Down Expand Up @@ -1841,8 +1840,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 @@ -1863,44 +1862,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) {
xunilrj marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -1917,9 +1904,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 @@ -2793,7 +2786,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 @@ -2813,7 +2806,9 @@ mod tests {
ExperimentalFlags {
new_encoding: false,
},
)
)?;
expr.type_check_analyze(handler, &mut TypeCheckAnalysisContext::new(&engines))?;
Ok(expr)
}

#[test]
Expand Down Expand Up @@ -2874,21 +2869,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" }
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];
xunilrj marked this conversation as resolved.
Show resolved Hide resolved
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] = [];
}
xunilrj marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading