search

sanctuary-js / sanctuary-def

Run-time type system for JavaScript
MIT License
294 stars 23 forks source link

support explicit subtyping #236

Closed davidchambers closed 5 years ago

davidchambers commented 5 years ago

Before:

var FiniteNumber = $.NullaryType
  ('FiniteNumber')
  ('https://github.com/sanctuary-js/sanctuary-def/tree/v0.19.0#FiniteNumber')
  (function(x) { return ValidNumber._test (x) && isFinite (x); });

After:

var FiniteNumber = $.NullaryType
  ('FiniteNumber')
  ('https://github.com/sanctuary-js/sanctuary-def/tree/v0.19.0#FiniteNumber')
  ([ValidNumber])
  (isFinite);

The primary reason for this change is to make defining subtypes more convenient. In the future the relationships this change exposes could be used to make type checking more efficient. If a value is not a member of $.Number, say, we know it is not a member of any of the subtypes of $.Number either.

davidchambers commented 5 years ago

:warning: Before this pull request is merged, we must decide what to do with record types.

Currently, record types are shown in { name :: type, name :: type } form in type signatures and error messages. In some cases this makes it possible to indicate which field is incorrectly typed.

Record types can now subtype other record types. We could make code changes to ensure that $.RecordType ([$.RecordType ([]) ({x: X})]) ({y: Y}) behaves identically to $.RecordType ([]) ({x: X, y: Y}). This special case would not apply to record types defined as subtypes of non-record types. This is an edge case, admittedly, but imagine reading an error message stating that {y: 0} is not a member of { y :: Number } (due to an unacknowledged dependency). Confusing.

One option is to leave $.RecordType as it exists on master, only capable of defining anonymous record types without supertypes. We would add a second type constructor, $.NamedRecordType, that would also take name, url, and supertypes. Types defined via this constructor would be referred to by name in type signatures and error messages, avoiding the problem @JAForbes noted in #138.

Avaq commented 5 years ago

This seems like the first in a series of steps towards the overly ambitious goals I set for myself with #162. Maybe it's time to close that PR, seeing as the branch has fallen quite far behind, and incremental steps probably will get us there faster.

davidchambers commented 5 years ago

Aha! I thought we had discussed this. I searched issues but not pull requests, so failed to find #162.

davidchambers commented 5 years ago

Let's merge #238 before taking further action here.

davidchambers commented 5 years ago

This pull request is now ready to be reviewed. :)