Skip to content

Commit

Permalink
Add record constructor to subtyping (#2007)
Browse files Browse the repository at this point in the history 
* Initial draft without Constant and TailVar

* Restrict visibility of RemoveErrorRow to super

* Create trait Subsume for UnifType and UnifRecordRows

* Fix problem about {a: Type}<: {a:Type;b:Type} that was not accepted

* Copy comments (general comment on subsumption and closing a record when there is a record/dictionary subsumption)

* Fix problem (it was error reporting...)

* Rename trait

Co-authored-by: Yann Hamdaoui <[email protected]>

* Correct spelling in comments

Co-authored-by: Yann Hamdaoui <[email protected]>

* Rename trait impl

* Rename trait

* Update comments

* Remove subsumption function from typecheck/mod.rs

* Add test

* Add documentation

* Rename trait (for clippy)

* Add tests

* Modify snapshot lsp

* Update comments core/src/typecheck/subtyping.rs

Co-authored-by: Yann Hamdaoui <[email protected]>

* Update doc/manual/typing.md

Co-authored-by: Yann Hamdaoui <[email protected]>

* Modify comment

* Modify comment

* Revert lsp snapshot

* Cosmetic improvements

This commit performs minor cosmetic improvements in the code handling
subtyping, mostly around comments and function interfaces.

* Update snapshot test for LSP

The support of subtyping for record types makes some types to be
instantiated earlier than before. Given the way terms are visited, when
writing something like `let f = std.array.map in ..`, `std.array.map`
used to be inferred to be of a polymorphic type `forall a b. ...` and
then only was instantiated (because record access is inferred, while the
bound expression is checked). With the new implementation, it is
instantiated as part of the subsumption rule, meaning that it'll appear
differently on the LSP.

All in all, both version of the data shown by the LSP (before this
change and after) are meaningful, in some sense. The polymorphic type is
still shown when it's part of the metadata anyway, in addition to the
inferred monomorphic type. This also doesn't change which expressions
are accepted or not. It's more of an artifact of when we visit terms,
before or after instantiation and how those visits are intertwined with
`check` and `infer`. It's not easy to revert to the previous state of
affairs, and it's in fact not necessarily a good thing either: in the
example above, the LSP would also show a polymorphic type for `f` - the
one of `map` - while in fact `f` has a monomorphic type, so you couldn't
use it in different contexts (say on an `Array Number` and later on a
`Array String`). The current version is showing the real monomorphic
nature of `f`.

---------

Co-authored-by: Yann Hamdaoui <[email protected]>
Co-authored-by: Yann Hamdaoui <[email protected]>
  • Loading branch information
@Eckaos @yannham
3 people authored Sep 6, 2024
1 parent 44aef16 commit 273ae0f
Show file tree
Hide file tree
Showing 8 changed files with 305 additions and 108 deletions.
112 changes: 9 additions & 103 deletions core/src/typecheck/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ pub mod reporting;
#[macro_use]
pub mod mk_uniftype;
pub mod eq;
pub mod subtyping;
pub mod unif;

use eq::{SimpleTermEnvironment, TermEnvironment};
Expand All @@ -90,6 +91,8 @@ use operation::{get_bop_type, get_nop_type, get_uop_type};
use pattern::{PatternTypeData, PatternTypes};
use unif::*;

use self::subtyping::SubsumedBy;

/// The max depth parameter used to limit the work performed when inferring the type of the stdlib.
const INFER_RECORD_MAX_DEPTH: u8 = 4;

Expand Down Expand Up @@ -2166,8 +2169,9 @@ fn check<V: TypecheckVisitor>(
| Term::Annotated(..) => {
let inferred = infer(state, ctxt.clone(), visitor, rt)?;

// We call to `subsumption` to perform the switch from infer mode to checking mode.
subsumption(state, ctxt, inferred, ty)
// We apply the subsumption rule when switching from infer mode to checking mode.
inferred
.subsumed_by(ty, state, ctxt)
.map_err(|err| err.into_typecheck_err(state, rt.pos))
}
Term::Enum(id) => {
Expand Down Expand Up @@ -2363,102 +2367,6 @@ fn check<V: TypecheckVisitor>(
}
}

/// Change from inference mode to checking mode, and apply a potential subsumption rule.
///
/// Currently, there is record/dictionary subtyping, if we are not in this case we fallback to perform
/// polymorphic type instantiation with unification variable on the left (on the inferred type),
/// and then simply performs unification (put differently, the subtyping relation when it is not
/// a record/dictionary subtyping is the equality
/// relation).
///
/// The type instantiation corresponds to the zero-ary case of application in the current
/// specification (which is based on [A Quick Look at Impredicativity][quick-look], although we
/// currently don't support impredicative polymorphism).
///
/// In the future, this function might implement a other non-trivial subsumption rule.
///
/// [quick-look]: https://www.microsoft.com/en-us/research/uploads/prod/2020/01/quick-look-icfp20-fixed.pdf
pub fn subsumption(
state: &mut State,
mut ctxt: Context,
inferred: UnifType,
checked: UnifType,
) -> Result<(), UnifError> {
let inferred_inst = instantiate_foralls(state, &mut ctxt, inferred, ForallInst::UnifVar);
let checked = checked.into_root(state.table);
match (inferred_inst, checked) {
(
UnifType::Concrete {
typ: TypeF::Record(rrows),
..
},
UnifType::Concrete {
typ:
TypeF::Dict {
type_fields,
flavour,
},
var_levels_data,
},
) => {
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,
},
var_levels_data,
};
Err(UnifError::WithConst {
var_kind: VarKindDiscriminant::RecordRows,
expected_const_id: id,
inferred: checked,
})?
}
_ => (),
}
}
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),
}
}

fn check_field<V: TypecheckVisitor>(
state: &mut State,
ctxt: Context,
Expand Down Expand Up @@ -2489,7 +2397,9 @@ fn check_field<V: TypecheckVisitor>(
field.value.as_ref(),
)?;

subsumption(state, ctxt, inferred, ty).map_err(|err| err.into_typecheck_err(state, pos))
inferred
.subsumed_by(ty, state, ctxt)
.map_err(|err| err.into_typecheck_err(state, pos))
}
}

Expand Down Expand Up @@ -2573,10 +2483,6 @@ fn infer_with_annot<V: TypecheckVisitor>(
// An empty value is a record field without definition. We don't check anything, and infer
// its type to be either the first annotation defined if any, or `Dyn` otherwise.
// We can only hit this case for record fields.
//
// TODO: we might have something to do with the visitor to clear the current metadata. It
// looks like it may be unduly attached to the next field definition, which is not
// critical, but still a bug.
_ => {
let inferred = annot
.first()
Expand Down
Loading

0 comments on commit 273ae0f

Please sign in to comment.