Merge pull request #422 from rtfeldman/optional-fields-touchups

optional fields improvements
This commit is contained in:
Richard Feldman 2020-07-22 22:08:38 -04:00 committed by GitHub
commit de39acd80c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 330 additions and 48 deletions

View File

@ -619,7 +619,7 @@ pub fn constrain_expr(
let mut rec_field_types = SendMap::default();
let label = field.clone();
rec_field_types.insert(label, RecordField::Required(field_type.clone()));
rec_field_types.insert(label, RecordField::Demanded(field_type.clone()));
let record_type = Type::Record(rec_field_types, Box::new(ext_type));
let record_expected = Expected::NoExpectation(record_type);
@ -665,7 +665,7 @@ pub fn constrain_expr(
let mut field_types = SendMap::default();
let label = field.clone();
field_types.insert(label, RecordField::Required(field_type.clone()));
field_types.insert(label, RecordField::Demanded(field_type.clone()));
let record_type = Type::Record(field_types, Box::new(ext_type));
let category = Category::Accessor(field.clone());
@ -1019,7 +1019,7 @@ fn constrain_def(env: &Env, def: &Def, body_con: Constraint) -> Constraint {
expr_type,
annotation_expected.clone(),
Category::Storage,
annotation.region,
Region::span_across(&annotation.region, &def.loc_expr.region),
));
// when a def is annotated, and it's body is a closure, treat this
@ -1031,7 +1031,7 @@ fn constrain_def(env: &Env, def: &Def, body_con: Constraint) -> Constraint {
(
Closure(fn_var, _symbol, _recursive, args, boxed),
Type::Function(arg_types, _),
) if true => {
) => {
let expected = annotation_expected;
let region = def.loc_expr.region;

View File

@ -222,6 +222,7 @@ fn to_type(solved_type: &SolvedType, free_vars: &mut FreeVars, var_store: &mut V
let field_val = match field {
Required(typ) => Required(to_type(&typ, free_vars, var_store)),
Optional(typ) => Optional(to_type(&typ, free_vars, var_store)),
Demanded(typ) => Demanded(to_type(&typ, free_vars, var_store)),
};
new_fields.insert(label.clone(), field_val);

View File

@ -227,24 +227,28 @@ pub fn constrain_pattern(
constrain_pattern(env, &loc_guard.value, loc_guard.region, expected, state);
RecordField::Required(pat_type)
RecordField::Demanded(pat_type)
}
DestructType::Optional(expr_var, loc_expr) => {
state.constraints.push(Constraint::Pattern(
region,
PatternCategory::PatternDefault,
Type::Variable(*expr_var),
PExpected::ForReason(
PReason::OptionalField,
pat_type.clone(),
loc_expr.region,
),
));
state.vars.push(*expr_var);
let expr_expected = Expected::ForReason(
Reason::RecordDefaultField(label.clone()),
pat_type.clone(),
loc_expr.region,
);
state.constraints.push(Constraint::Eq(
Type::Variable(*expr_var),
expr_expected.clone(),
Category::DefaultValue(label.clone()),
region,
));
state.vars.push(*expr_var);
let expr_con =
constrain_expr(env, loc_expr.region, &loc_expr.value, expr_expected);
state.constraints.push(expr_con);
@ -253,7 +257,7 @@ pub fn constrain_pattern(
}
DestructType::Required => {
// No extra constraints necessary.
RecordField::Required(pat_type)
RecordField::Demanded(pat_type)
}
};

View File

@ -259,7 +259,7 @@ fn constrain_pattern(
expected,
);
RecordField::Required(pat_type)
RecordField::Demanded(pat_type)
}
DestructType::Optional(expr_var, loc_expr) => {
let expr_expected = Expected::ForReason(
@ -292,7 +292,7 @@ fn constrain_pattern(
}
DestructType::Required => {
// No extra constraints necessary.
RecordField::Required(pat_type)
RecordField::Demanded(pat_type)
}
};
@ -1364,7 +1364,7 @@ pub fn constrain_expr(
let field_uniq_type = Bool::variable(field_uniq_var);
let field_type = attr_type(field_uniq_type, Type::Variable(*field_var));
field_types.insert(field.clone(), RecordField::Required(field_type.clone()));
field_types.insert(field.clone(), RecordField::Demanded(field_type.clone()));
let record_uniq_var = var_store.fresh();
let record_uniq_type = Bool::container(record_uniq_var, vec![field_uniq_var]);
@ -1421,7 +1421,7 @@ pub fn constrain_expr(
let field_uniq_type = Bool::variable(field_uniq_var);
let field_type = attr_type(field_uniq_type, Type::Variable(*field_var));
field_types.insert(field.clone(), RecordField::Required(field_type.clone()));
field_types.insert(field.clone(), RecordField::Demanded(field_type.clone()));
let record_uniq_var = var_store.fresh();
let record_uniq_type = Bool::container(record_uniq_var, vec![field_uniq_var]);
@ -1916,6 +1916,14 @@ fn annotation_to_attr_type(
use RecordField::*;
let lifted_field = match field {
Demanded(tipe) => {
let (new_vars, lifted_field) =
annotation_to_attr_type(var_store, &tipe, rigids, change_var_kind);
vars.extend(new_vars);
Demanded(lifted_field)
}
Required(tipe) => {
let (new_vars, lifted_field) =
annotation_to_attr_type(var_store, &tipe, rigids, change_var_kind);
@ -2542,6 +2550,7 @@ fn fix_mutual_recursive_alias_help_help(rec_var: Variable, attribute: &Type, int
let arg = match field {
RecordField::Required(arg) => arg,
RecordField::Optional(arg) => arg,
RecordField::Demanded(arg) => arg,
};
fix_mutual_recursive_alias_help(rec_var, attribute, arg);

View File

@ -409,6 +409,7 @@ impl<'a> Pattern<'a> {
/// different locations/whitespace
pub fn equivalent(&self, other: &Self) -> bool {
use Pattern::*;
match (self, other) {
(Identifier(x), Identifier(y)) => x == y,
(GlobalTag(x), GlobalTag(y)) => x == y,
@ -428,21 +429,13 @@ impl<'a> Pattern<'a> {
(RequiredField(x, inner_x), RequiredField(y, inner_y)) => {
x == y && inner_x.value.equivalent(&inner_y.value)
}
(OptionalField(x, _inner_x), OptionalField(y, _inner_y)) => {
(OptionalField(x, _), OptionalField(y, _))
| (OptionalField(x, _), Identifier(y))
| (Identifier(x), OptionalField(y, _)) => {
// optional record fields can be annotated as:
// { x, y } : { x : Int, y ? Bool }
// { x, y ? False } = rec
x == y
// TODO
//
// We can give an annotation like so
//
// { x, y } : { x : Int, y : Bool }
// { x, y } = rec
//
// But what about:
//
// { x, y ? False } : { x : Int, y ? Bool }
// { x, y ? False } = rec
//
// inner_x.value.equivalent(&inner_y.value)
}
(Nested(x), Nested(y)) => x.equivalent(y),

View File

@ -953,6 +953,7 @@ fn to_pattern_report<'b>(
}
PExpected::ForReason(reason, expected_type, region) => match reason {
PReason::OptionalField => unreachable!("this will never be reached I think"),
PReason::TypedArg { opt_name, index } => {
let name = match opt_name {
Some(n) => alloc.symbol_unqualified(n),
@ -1097,6 +1098,7 @@ fn add_pattern_category<'b>(
Record => alloc.reflow(" record values of type:"),
EmptyRecord => alloc.reflow(" an empty record:"),
PatternGuard => alloc.reflow(" a pattern guard of type:"),
PatternDefault => alloc.reflow(" an optional field of type:"),
Set => alloc.reflow(" sets of type:"),
Map => alloc.reflow(" maps of type:"),
Ctor(tag_name) => alloc.concat(vec![
@ -1151,6 +1153,7 @@ pub enum Problem {
TagTypo(TagName, Vec<TagName>),
TagsMissing(Vec<TagName>),
BadRigidVar(Lowercase, ErrorType),
OptionalRequiredMismatch(Lowercase),
}
fn problems_to_hint<'b>(
@ -1245,6 +1248,7 @@ fn to_comparison<'b>(
}
}
#[derive(Debug)]
pub enum Status {
Similar, // the structure is the same or e.g. record fields are different
Different(Vec<Problem>), // e.g. found Bool, expected Int
@ -1344,6 +1348,9 @@ pub fn to_doc<'b>(
RecordField::Required(v) => {
RecordField::Required(to_doc(alloc, Parens::Unnecessary, v))
}
RecordField::Demanded(v) => {
RecordField::Demanded(to_doc(alloc, Parens::Unnecessary, v))
}
},
)
})
@ -1643,6 +1650,7 @@ fn diff_record<'b>(
match t1 {
RecordField::Optional(_) => RecordField::Optional(diff.left),
RecordField::Required(_) => RecordField::Required(diff.left),
RecordField::Demanded(_) => RecordField::Demanded(diff.left),
},
),
right: (
@ -1651,11 +1659,31 @@ fn diff_record<'b>(
match t2 {
RecordField::Optional(_) => RecordField::Optional(diff.right),
RecordField::Required(_) => RecordField::Required(diff.right),
RecordField::Demanded(_) => RecordField::Demanded(diff.right),
},
),
status: diff.status,
status: {
match (&t1, &t2) {
(RecordField::Demanded(_), RecordField::Optional(_))
| (RecordField::Optional(_), RecordField::Demanded(_)) => match diff.status
{
Status::Similar => {
Status::Different(vec![Problem::OptionalRequiredMismatch(
field.clone(),
)])
}
Status::Different(mut problems) => {
problems.push(Problem::OptionalRequiredMismatch(field.clone()));
Status::Different(problems)
}
},
_ => diff.status,
}
},
}
};
let to_unknown_docs = |(field, tipe): &(Lowercase, RecordField<ErrorType>)| {
(
field.clone(),
@ -2006,6 +2034,9 @@ mod report_text {
let entry_to_doc =
|(field_name, field_type): (RocDocBuilder<'b>, RecordField<RocDocBuilder<'b>>)| {
match field_type {
RecordField::Demanded(field) => {
field_name.append(alloc.text(" : ")).append(field)
}
RecordField::Required(field) => {
field_name.append(alloc.text(" : ")).append(field)
}
@ -2414,5 +2445,13 @@ fn type_problem_to_pretty<'b>(
alloc.stack(vec![hint1, hint2])
}
},
OptionalRequiredMismatch(field) => alloc.hint().append(alloc.concat(vec![
alloc.reflow("To extract the "),
alloc.record_field(field),
alloc.reflow(
" field it must be non-optional, but the type says this field is optional. ",
),
alloc.reflow("Learn more about optional fields at TODO."),
])),
}
}

View File

@ -3524,10 +3524,172 @@ mod test_reporting {
)
}
#[test]
fn optional_record_invalid_let_binding() {
report_problem_as(
indoc!(
r#"
\rec ->
{ x, y } : { x : Int, y ? Bool }
{ x, y } = rec
{ x, y }
"#
),
indoc!(
r#"
-- TYPE MISMATCH ---------------------------------------------------------------
Something is off with the body of this definition:
2 > { x, y } : { x : Int, y ? Bool }
3 > { x, y } = rec
The body is a value of type:
{ x : Int, y : Bool }
But the type annotation says it should be:
{ x : Int, y ? Bool }
Hint: To extract the `.y` field it must be non-optional, but the type
says this field is optional. Learn more about optional fields at TODO.
"#
),
)
}
#[test]
fn optional_record_invalid_function() {
report_problem_as(
indoc!(
r#"
f : { x : Int, y ? Int } -> Int
f = \{ x, y } -> x + y
f
"#
),
indoc!(
r#"
-- TYPE MISMATCH ---------------------------------------------------------------
The 1st argument to `f` is weird:
2 f = \{ x, y } -> x + y
^^^^^^^^
The argument is a pattern that matches record values of type:
{ x : Int, y : Int }
But the annotation on `f` says the 1st argument should be:
{ x : Int, y ? Int }
Hint: To extract the `.y` field it must be non-optional, but the type
says this field is optional. Learn more about optional fields at TODO.
"#
),
)
}
#[test]
fn optional_record_invalid_when() {
report_problem_as(
indoc!(
r#"
f : { x : Int, y ? Int } -> Int
f = \r ->
when r is
{ x, y } -> x + y
f
"#
),
indoc!(
r#"
-- TYPE MISMATCH ---------------------------------------------------------------
The 1st pattern in this `when` is causing a mismatch:
4 { x, y } -> x + y
^^^^^^^^
The first pattern is trying to match record values of type:
{ x : Int, y : Int }
But the expression between `when` and `is` has the type:
{ x : Int, y ? Int }
Hint: To extract the `.y` field it must be non-optional, but the type
says this field is optional. Learn more about optional fields at TODO.
"#
),
)
}
#[test]
fn optional_record_invalid_access() {
report_problem_as(
indoc!(
r#"
f : { x : Int, y ? Int } -> Int
f = \r -> r.y
f
"#
),
indoc!(
r#"
-- TYPE MISMATCH ---------------------------------------------------------------
This expression is used in an unexpected way:
2 f = \r -> r.y
^^^
This `r` value is a:
{ x : Int, y ? Int }
But you are trying to use it as:
{ x : Int, y : Int }
Hint: To extract the `.y` field it must be non-optional, but the type
says this field is optional. Learn more about optional fields at TODO.
"#
),
)
}
// TODO field accessors give a parse error at the moment
// #[test]
// fn optional_record_invalid_accessor() {
// report_problem_as(
// indoc!(
// r#"
// f : { x : Int, y ? Int } -> Int
// f = \r -> .y r
//
// f
// "#
// ),
// indoc!(
// r#"
// -- TYPE MISMATCH ---------------------------------------------------------------
//
// "#
// ),
// )
// }
#[test]
fn guard_mismatch_with_annotation() {
// TODO improve this message
// The problem is the guard, and the message should point to that
report_problem_as(
indoc!(
r#"
@ -3563,9 +3725,6 @@ mod test_reporting {
#[test]
fn optional_field_mismatch_with_annotation() {
// TODO improve this message
// The problem is the pattern match default
// the message should point to that.
report_problem_as(
indoc!(
r#"

View File

@ -620,6 +620,7 @@ fn type_to_variable(
let field_var = match field_type {
Required(typ) => Required(type_to_variable(subs, rank, pools, cached, typ)),
Optional(typ) => Optional(type_to_variable(subs, rank, pools, cached, typ)),
Demanded(typ) => Demanded(type_to_variable(subs, rank, pools, cached, typ)),
};
field_vars.insert(field.clone(), field_var);
@ -1257,6 +1258,9 @@ fn deep_copy_var_help(
use RecordField::*;
let new_field = match field {
Demanded(var) => {
Demanded(deep_copy_var_help(subs, max_rank, pools, var))
}
Required(var) => {
Required(deep_copy_var_help(subs, max_rank, pools, var))
}

View File

@ -2669,4 +2669,20 @@ mod solve_expr {
"{ x : Num a, y ? Num a }* -> Num a",
);
}
#[test]
fn optional_field_let_with_signature() {
infer_eq_without_problem(
indoc!(
r#"
\rec ->
{ x, y } : { x : Int, y ? Bool }*
{ x, y ? False } = rec
{ x, y }
"#
),
"{ x : Int, y ? Bool }* -> { x : Int, y : Bool }",
);
}
}

View File

@ -416,6 +416,10 @@ fn write_flat_type(env: &Env, flat_type: FlatType, subs: &Subs, buf: &mut String
buf.push_str(" : ");
var
}
Demanded(var) => {
buf.push_str(" : ");
var
}
};
write_content(

View File

@ -129,6 +129,7 @@ impl SolvedType {
let solved_type = match field {
Optional(typ) => RecordField::Optional(Self::from_type(solved_subs, typ)),
Required(typ) => RecordField::Required(Self::from_type(solved_subs, typ)),
Demanded(typ) => RecordField::Demanded(Self::from_type(solved_subs, typ)),
};
solved_fields.push((label.clone(), solved_type));
@ -246,6 +247,7 @@ impl SolvedType {
let solved_type = match field {
Optional(var) => Optional(Self::from_var(subs, var)),
Required(var) => Required(Self::from_var(subs, var)),
Demanded(var) => Demanded(Self::from_var(subs, var)),
};
new_fields.push((label, solved_type));

View File

@ -616,6 +616,7 @@ fn occurs(
once(&ext_var).chain(vars_by_field.values().map(|field| match field {
RecordField::Optional(var) => var,
RecordField::Required(var) => var,
RecordField::Demanded(var) => var,
}));
short_circuit(subs, root_var, &new_seen, it)
}
@ -743,6 +744,9 @@ fn explicit_substitute(
Required(var) => {
Required(explicit_substitute(subs, from, to, *var, seen))
}
Demanded(var) => {
Demanded(explicit_substitute(subs, from, to, *var, seen))
}
};
}
subs.set_content(in_var, Structure(Record(vars_by_field, new_ext_var)));
@ -1017,6 +1021,7 @@ fn flat_type_to_err_type(
let err_type = match field_var {
Optional(var) => Optional(var_to_err_type(subs, state, var)),
Required(var) => Required(var_to_err_type(subs, state, var)),
Demanded(var) => Demanded(var_to_err_type(subs, state, var)),
};
err_fields.insert(field, err_type);

View File

@ -13,10 +13,20 @@ pub const TYPE_NUM: &str = "Num";
pub const TYPE_INTEGER: &str = "Integer";
pub const TYPE_FLOATINGPOINT: &str = "FloatingPoint";
///
/// Intuitively
///
/// - Demanded: only introduced by pattern matches, e.g. { x } ->
/// Cannot unify with an Optional field, but can unify with a Required field
/// - Required: introduced by record literals and type annotations.
/// Can unify with Optional and Demanded
/// - Optional: introduced by pattern matches and annotations.
/// Can unify with Required, but not with Demanded
#[derive(PartialEq, Eq, Clone)]
pub enum RecordField<T> {
Optional(T),
Required(T),
Demanded(T),
}
impl<T: Copy> Copy for RecordField<T> {}
@ -26,8 +36,9 @@ impl<T: fmt::Debug> fmt::Debug for RecordField<T> {
use RecordField::*;
match self {
Optional(typ) => typ.fmt(f),
Required(typ) => typ.fmt(f),
Optional(typ) => write!(f, "Optional({:?})", typ),
Required(typ) => write!(f, "Required({:?})", typ),
Demanded(typ) => write!(f, "Demanded({:?})", typ),
}
}
}
@ -39,6 +50,7 @@ impl<T> RecordField<T> {
match self {
Optional(t) => t,
Required(t) => t,
Demanded(t) => t,
}
}
@ -50,6 +62,7 @@ impl<T> RecordField<T> {
match self {
Optional(t) => Optional(f(t)),
Required(t) => Required(f(t)),
Demanded(t) => Demanded(f(t)),
}
}
}
@ -61,6 +74,7 @@ impl RecordField<Type> {
match self {
Optional(typ) => typ.substitute(substitutions),
Required(typ) => typ.substitute(substitutions),
Demanded(typ) => typ.substitute(substitutions),
}
}
@ -70,6 +84,7 @@ impl RecordField<Type> {
match self {
Optional(typ) => typ.substitute_alias(rep_symbol, actual),
Required(typ) => typ.substitute_alias(rep_symbol, actual),
Demanded(typ) => typ.substitute_alias(rep_symbol, actual),
}
}
@ -85,6 +100,7 @@ impl RecordField<Type> {
match self {
Optional(typ) => typ.instantiate_aliases(region, aliases, var_store, introduced),
Required(typ) => typ.instantiate_aliases(region, aliases, var_store, introduced),
Demanded(typ) => typ.instantiate_aliases(region, aliases, var_store, introduced),
}
}
@ -94,6 +110,7 @@ impl RecordField<Type> {
match self {
Optional(typ) => typ.contains_symbol(rep_symbol),
Required(typ) => typ.contains_symbol(rep_symbol),
Demanded(typ) => typ.contains_symbol(rep_symbol),
}
}
pub fn contains_variable(&self, rep_variable: Variable) -> bool {
@ -102,6 +119,7 @@ impl RecordField<Type> {
match self {
Optional(typ) => typ.contains_variable(rep_variable),
Required(typ) => typ.contains_variable(rep_variable),
Demanded(typ) => typ.contains_variable(rep_variable),
}
}
}
@ -187,7 +205,11 @@ impl fmt::Debug for Type {
let mut any_written_yet = false;
for (label, field_type) in fields {
write!(f, "{:?} : {:?}", label, field_type)?;
match field_type {
RecordField::Optional(_) => write!(f, "{:?} ? {:?}", label, field_type)?,
RecordField::Required(_) => write!(f, "{:?} : {:?}", label, field_type)?,
RecordField::Demanded(_) => write!(f, "{:?} : {:?}", label, field_type)?,
}
if any_written_yet {
write!(f, ", ")?;
@ -714,6 +736,7 @@ fn symbols_help(tipe: &Type, accum: &mut ImSet<Symbol>) {
match field {
Optional(arg) => symbols_help(arg, accum),
Required(arg) => symbols_help(arg, accum),
Demanded(arg) => symbols_help(arg, accum),
}
});
}
@ -756,6 +779,7 @@ fn variables_help(tipe: &Type, accum: &mut ImSet<Variable>) {
match field {
Optional(x) => variables_help(x, accum),
Required(x) => variables_help(x, accum),
Demanded(x) => variables_help(x, accum),
};
}
variables_help(ext, accum);
@ -843,6 +867,7 @@ pub enum PReason {
index: Index,
},
PatternGuard,
OptionalField,
}
#[derive(Debug, Clone, PartialEq, Eq)]
@ -922,6 +947,7 @@ pub enum PatternCategory {
Record,
EmptyRecord,
PatternGuard,
PatternDefault,
Set,
Map,
Ctor(TagName),
@ -1103,6 +1129,10 @@ fn write_error_type_help(
buf.push_str(" : ");
content
}
Demanded(content) => {
buf.push_str(" : ");
content
}
};
write_error_type_help(home, interns, content, buf, Parens::Unnecessary);
@ -1243,6 +1273,10 @@ fn write_debug_error_type_help(error_type: ErrorType, buf: &mut String, parens:
buf.push_str(" : ");
content
}
Demanded(content) => {
buf.push_str(" : ");
content
}
};
write_debug_error_type_help(content, buf, Parens::Unnecessary);

View File

@ -327,13 +327,24 @@ fn unify_shared_fields(
let num_shared_fields = shared_fields.len();
for (name, (actual, expected)) in shared_fields {
let problems = unify_pool(subs, pool, actual.into_inner(), expected.into_inner());
let local_problems = unify_pool(subs, pool, actual.into_inner(), expected.into_inner());
if problems.is_empty() {
if local_problems.is_empty() {
use RecordField::*;
// If either field is Required, both are Required.
// Unification of optional fields
//
// Demanded does not unify with Optional
// Unifying Required with Demanded => Demanded
// Unifying Optional with Required => Required
// Unifying X with X => X
let actual = match (actual, expected) {
(Demanded(_), Optional(_)) | (Optional(_), Demanded(_)) => {
continue;
}
(Demanded(val), Required(_))
| (Required(val), Demanded(_))
| (Demanded(val), Demanded(_)) => Demanded(val),
(Required(val), Required(_)) => Required(val),
(Required(val), Optional(_)) => Required(val),
(Optional(val), Required(_)) => Required(val),
@ -614,13 +625,14 @@ fn unify_shared_tags(
}
}
fn has_no_required_fields<'a, I, T>(fields: &mut I) -> bool
fn has_only_optional_fields<'a, I, T>(fields: &mut I) -> bool
where
I: Iterator<Item = &'a RecordField<T>>,
T: 'a,
{
fields.all(|field| match field {
RecordField::Required(_) => false,
RecordField::Demanded(_) => false,
RecordField::Optional(_) => true,
})
}
@ -638,11 +650,11 @@ fn unify_flat_type(
match (left, right) {
(EmptyRecord, EmptyRecord) => merge(subs, ctx, Structure(left.clone())),
(Record(fields, ext), EmptyRecord) if has_no_required_fields(&mut fields.values()) => {
(Record(fields, ext), EmptyRecord) if has_only_optional_fields(&mut fields.values()) => {
unify_pool(subs, pool, *ext, ctx.second)
}
(EmptyRecord, Record(fields, ext)) if has_no_required_fields(&mut fields.values()) => {
(EmptyRecord, Record(fields, ext)) if has_only_optional_fields(&mut fields.values()) => {
unify_pool(subs, pool, ctx.first, *ext)
}