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

Support for GADTs #195

Closed
wants to merge 95 commits into from
Closed

Conversation

Meowcolm024
Copy link
Collaborator

@Meowcolm024 Meowcolm024 commented Nov 28, 2023

Add support for GADTs

  • GADT-reasoning in pattern matching (type annotation needed)
  • Typing for type selections
  • Parsing the wildcard type ? in type arguments
  • use as keyword for upcast (same as :)
  • test cases GADT[1-6].mls in gadt directory, and AsOp.mls
  • refactored wildcard type arguments
  • correct extruded type args

Some TODOs (added by @LPTK):

  • Merge branch local-gadt-wip after we figure out the new errors
  • Merge my local changes to simplify away compositions with ??X types into main branch before merging this PR

The PR has become way too big and it's difficult to keep track of what changes cause what results or regressions. Let's split it up and merge each of these things separately:

  • Typing for type selections
  • Parsing the wildcard type ? in type arguments, use as keyword for upcast (same as :)
  • Refactored wildcard type arguments, correct extruded type args
  • Rethink the way GADT reasoning is triggered: only introduce skolems when they correspond to user-annotated pattern variable type arguments (as in App[f, a]); and in this case don't try to refine these type arguments in the scrutinee.

@LPTK
Copy link
Contributor

LPTK commented Nov 28, 2023

Cool! Will try to take a look later. In the meantime, plz read this again and make sure all the changes are clean: https://github.com/hkust-taco/mlscript/blob/mlscript/CONTRIBUTING.md

@@ -1147,13 +1147,13 @@ abstract class NewParser(origin: Origin, tokens: Ls[Stroken -> Loc], newDefs: Bo
}
val lb = getTypeName("restricts")
val ub = getTypeName("extends")

// TODO update `TypeParamInfo` to use lb and ub
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

Comment on lines 244 to 251
ts2.forall {
case sk: SkolemTag => ts1(sk)
case tt: TraitTag => ts1(tt)
case Extruded(pol, sk) => !pol || ts1.exists { // find ? <: bot
case Extruded(true, _) => true
case _ => false
}
} && rt1 <:< rt2 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing looks very fishy. What is going on here? Why don't we use the normal set functions?

shared/src/main/scala/mlscript/ConstraintSolver.scala Outdated Show resolved Hide resolved
shared/src/test/diff/gadt/GADT5.mls Outdated Show resolved Hide resolved
shared/src/test/diff/gadt/Misc.mls Outdated Show resolved Hide resolved
shared/src/main/scala/mlscript/TyperHelpers.scala Outdated Show resolved Hide resolved
shared/src/main/scala/mlscript/TyperHelpers.scala Outdated Show resolved Hide resolved
shared/src/main/scala/mlscript/TyperHelpers.scala Outdated Show resolved Hide resolved
shared/src/main/scala/mlscript/TyperHelpers.scala Outdated Show resolved Hide resolved
shared/src/main/scala/mlscript/Typer.scala Outdated Show resolved Hide resolved
val cache: MutMap[Level, MutMap[TypeVarOrRigidVar->Bool, TypeVarOrRigidVar]],
) {
private var cycle: Int = 0
// TODO maybe find a better way
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what this achieves and why it's needed?

@LPTK LPTK changed the base branch from new-definition-typing to mlscript March 11, 2024 08:22
@Meowcolm024 Meowcolm024 mentioned this pull request Apr 9, 2024
@LPTK
Copy link
Contributor

LPTK commented Jan 21, 2025

Thanks again for the great work, @Meowcolm024! These changes will be useful for future reference, even though they are already obsolete (since we are moving to a new type checker implementation). So I will close this for now as it is not worth fixing the conflicts and cleaning up the history, similarly to #219

By the way, as I told you in person some time ago, you merged some commits on my WIP branch (the "W"-named commits) which were only temporary experimentation changes. These were not intended to be made part of your PR – or at least, not as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants