Adding dictionary constructor to subsumption rule #1998
Conversation
core/src/typecheck/mod.rs
Outdated
| @@ -2416,6 +2416,9 @@ pub fn subsumption( | |||
| Ok(()) | |||
| } | |||
| (TypeF::Array(a), TypeF::Array(b)) => subsumption(state, ctxt, *a.clone(), *b.clone()), | |||
| (TypeF::Dict { type_fields: a, .. }, TypeF::Dict { type_fields: b, .. }) => { | |||
| subsumption(state, ctxt.clone(), *a.clone(), *b.clone()) | |||
There was a problem hiding this comment.
Why do we need to clone everywhere here? couldn't we just match on inferred_inst and not &inferred_inst and take ownership of the subcomponents?
There was a problem hiding this comment.
Because this line,
(_,_) => checked.unify(inferred_inst, state, &ctxt)and
else {
checked.unify(inferred_inst, state, &ctxt)
}I need to pattern match on &inferred_inst to not move the ownership. If I try to pattern match on inferred_inst, I can't refer to inferred_inst anymore because the ownership has moved.
I found an alternative with using only one pattern matching but it is a bit long and unreadable i think.
match (inferred_inst, &checked) {
(
UnifType::Concrete {
typ: TypeF::Record(rrows),
..
},
UnifType::Concrete {
typ: TypeF::Dict { type_fields, .. },
..
},
) => {
for row in rrows.iter() {
match row {
GenericUnifRecordRowsIteratorItem::Row(a) => {
subsumption(state, ctxt.clone(), a.typ.clone(), *type_fields.clone())?
}
GenericUnifRecordRowsIteratorItem::TailUnifVar { id, .. } =>
// We don't need to perform any variable level checks when unifying a free
// unification variable with a ground type
// We close the tail because there is no garanty that
// { a : Number, b : Number, _ : a?} <= { _ : Number}
{
state
.table
.assign_rrows(id, UnifRecordRows::concrete(RecordRowsF::Empty))
}
GenericUnifRecordRowsIteratorItem::TailConstant(id) => {
Err(UnifError::WithConst {
var_kind: VarKindDiscriminant::RecordRows,
expected_const_id: id,
inferred: checked.clone(),
})?
}
_ => (),
}
}
Ok(())
}
(
UnifType::Concrete {
typ: TypeF::Array(a),
..
},
UnifType::Concrete {
typ: TypeF::Array(b),
..
},
)
| (
UnifType::Concrete {
typ: TypeF::Dict { type_fields: a, .. },
..
},
UnifType::Concrete {
typ: TypeF::Dict { type_fields: b, .. },
..
},
) => subsumption(state, ctxt.clone(), *a, *b.clone()),
(inferred_inst, _) => checked.unify(inferred_inst, state, &ctxt),
}I don't really see a middle ground between the two alternatives.
There was a problem hiding this comment.
I think this one is fine, although I don't see why you still need to take &checked as a reference (which causes you to clone it in the UnifError::WithConst.
It's true that let-type has the bad side effect of consuming the scrutinee. If you really want to avoid repeating the UnifTypeConcrete (IMHO again, it's not that bad), you can use a match for the outer level as well:
match (inferred_inst, checked) {
(
UnifType::Concrete {
typ: typ1,
var_levels_data: data1,
},
UnifType::Concrete {
typ: typ2,
var_levels_data: data2,
},
) => match (typ1, typ2) {
(
TypeF::Record(rrows),
TypeF::Dict {
type_fields,
flavour,
},
) => {
for row in rrows.iter() {
match row {
GenericUnifRecordRowsIteratorItem::Row(a) => {
subsumption(state, ctxt.clone(), a.typ.clone(), *type_fields.clone())?
}
GenericUnifRecordRowsIteratorItem::TailUnifVar { id, .. } =>
// We don't need to perform any variable level checks when unifying a free
// unification variable with a ground type
// We close the tail because there is no garanty that
// { a : Number, b : Number, _ : a?} <= { _ : Number}
{
state
.table
.assign_rrows(id, UnifRecordRows::concrete(RecordRowsF::Empty))
}
GenericUnifRecordRowsIteratorItem::TailConstant(id) => {
let checked = UnifType::Concrete {
typ: TypeF::Dict {
type_fields: type_fields.clone(),
flavour: flavour.clone(),
},
var_levels_data: data2,
};
Err(UnifError::WithConst {
var_kind: VarKindDiscriminant::RecordRows,
expected_const_id: id,
inferred: checked,
})?
}
_ => (),
}
}
Ok(())
}
(TypeF::Array(a), TypeF::Array(b)) => subsumption(state, ctxt, *a, *b),
(TypeF::Dict { type_fields: a, .. }, TypeF::Dict { type_fields: b, .. }) => {
subsumption(state, ctxt.clone(), *a, *b)
}
(typ1, typ2) => {
let inferred_inst = UnifType::Concrete {
typ: typ1,
var_levels_data: data1,
};
let checked = UnifType::Concrete {
typ: typ2,
var_levels_data: data2,
};
checked.unify(inferred_inst, state, &ctxt)
}
},
(inferred_inst, checked) => checked.unify(inferred_inst, state, &ctxt),
}The annoying thing is that you have to reconstruct checked and all when you need them back, which is a bit cumbersome. So all in all I'm personally leaning to your one big pattern matching solution.
The thing is, cloning types isn't free, so it's not a question of style. In general we shouldn't clone things just because it's easier to write, unless the clone is arguably invisible or we don't care anymore about performance (say in the error path).
There was a problem hiding this comment.
I changed &checked with checked in the pattern match.
But since with the pattern match, I can't refer to checked in the error construction. I made a clone before the pattern match and I need to clone in the construction of the error because it is in a loop. I don't think cloning in the error is a problem for performance because it is thrown immediately.
let checked_for_err = &checked.clone();
match (inferred_inst, checked) {
(
UnifType::Concrete {
typ: TypeF::Record(rrows),
..
},
UnifType::Concrete {
typ: TypeF::Dict { type_fields, .. },
..
},
) => {
for row in rrows.iter() {
match row {
GenericUnifRecordRowsIteratorItem::Row(a) => {
subsumption(state, ctxt.clone(), a.typ.clone(), *type_fields.clone())?
}
GenericUnifRecordRowsIteratorItem::TailUnifVar { id, .. } =>
// We don't need to perform any variable level checks when unifying a free
// unification variable with a ground type
// We close the tail because there is no garanty that
// { a : Number, b : Number, _ : a?} <= { _ : Number}
{
state
.table
.assign_rrows(id, UnifRecordRows::concrete(RecordRowsF::Empty))
}
GenericUnifRecordRowsIteratorItem::TailConstant(id) => {
Err(UnifError::WithConst {
var_kind: VarKindDiscriminant::RecordRows,
expected_const_id: id,
inferred: checked_for_err.clone(),
})?
}
_ => (),
}
}
Ok(())
}
(
UnifType::Concrete {
typ: TypeF::Array(a),
..
},
UnifType::Concrete {
typ: TypeF::Array(b),
..
},
)
| (
UnifType::Concrete {
typ: TypeF::Dict { type_fields: a, .. },
..
},
UnifType::Concrete {
typ: TypeF::Dict { type_fields: b, .. },
..
},
) => subsumption(state, ctxt.clone(), *a, *b),
(inferred_inst, checked) => checked.unify(inferred_inst, state, &ctxt),
}There was a problem hiding this comment.
You can look at my example. I rebuild checked when needed for the error reporting, so that it saves a clone() if we don't take the error path. Your solution is reasonable as well, that's just one clone (but we still do one more clone that in my proposal)
core/tests/integration/inputs/typecheck/dictionary_subtyping.ncl
Outdated
Show resolved
Hide resolved
Co-authored-by: Yann Hamdaoui <yann.hamdaoui@gmail.com>
|
@Eckaos ping me when you're done, so that I can merge this. |
|
I think i'm done ! @yannham |
No description provided.