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

Optional Arguments #220

Draft
wants to merge 150 commits into
base: mlscript
Choose a base branch
from
Draft

Conversation

noordahx
Copy link

Draft PR:

  • Resolved merge conflicts of original PR
  • Tests failed at OptionalArgs.mls
fun f1(a: Int, b: Int) = a + b
//│ ╔══[ERROR] Type mismatch in operator application:
//│ ║  l.17: 	fun f1(a: Int, b: Int) = a + b
//│ ║        	                         ^^^^^
//│ ╟── type `()` is not an instance of type `Int`
//│ ║  l.17: 	fun f1(a: Int, b: Int) = a + b
//│ ║        	          ^^^
//│ ╟── but it flows into reference with expected type `Int`
//│ ║  l.17: 	fun f1(a: Int, b: Int) = a + b
//│ ╙──      	                         ^
//│ TEST CASE FAILURE: There was an unexpected type error
//│ fun f1: (a: Int | (), b: Int | ()) -> (Int | error)

- Rename functions from `visitX` to `traverseX`.
- Add Boolean condition test cases.
- Use dollar symbol in generated variables.
- Add `TupleCase` which currently is commented.
This commit was cherry-picked from a future commit which changed
many files.
- Very minor but important: should use post-processed term as the
  desugared term.
- Add incomplete AVL tree test cases which reveals many problems.
- Cache operator split partial terms for efficiency.
- `Traceable` now supports filtering debugging by topics.
- Skip speicalization Boolean condition scrutinees.
We used to generate let bindings which destruct class parameters
during normalization. This works fine but it exposes some
inflexibility. For example, in the previous commit, I added a case
where free variables from a absorbed case branch are shadowed.
If we moving let bindings generation to desugaring stage, the
problem can be solved easily. This change also paves the road for
duplicated branch lifting.
Normalization stage is greatly simplified from now!
also support traversing let bindings
This is a refactor and not test changes were found.
Finally! I made this. In this commit, we track and store the
information of scrutinee through the context of per-UCS-term;
at the same time, we decouple the scrutinee and term symbol.
This has the following advantages.

1. The symbol corresponding to each variable can store information
   from different UCS terms.
2. The error location of the scrutinee is more precise.
3. This enables coverage checking to take advantage of previously
   collected patterns, eliminating the need to traverse the term
   again.

In addition, there are a few minor changes.

1. All functions used for pretty printing have been extracted into
   a separate package, namely `mlscript.ucs.display`.
2. Common operations about `Var` have been extracted into an
   implicit class in `DesugarUCS` base class.
@LPTK
Copy link
Contributor

LPTK commented May 24, 2024

Hi @noordahx, thanks for the PR draft. Do you need some assistance in moving it forward?

Comment on lines 18 to 26
//│ ╔══[ERROR] Type mismatch in operator application:
//│ ║ l.17: fun f1(a: Int, b: Int) = a + b
//│ ║ ^^^^^
//│ ╟── type `()` is not an instance of type `Int`
//│ ║ l.17: fun f1(a: Int, b: Int) = a + b
//│ ║ ^^^
//│ ╟── but it flows into reference with expected type `Int`
//│ ║ l.17: fun f1(a: Int, b: Int) = a + b
//│ ╙── ^
Copy link
Author

Choose a reason for hiding this comment

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

Hi @LPTK , I am having trouble with function parameter type parsing (seems all of the parameter types are '()' by default)

Copy link
Author

Choose a reason for hiding this comment

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

NewParser.scala "?" parser -> line

Copy link
Contributor

Choose a reason for hiding this comment

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

That code does not make much sense to me. You may have mismerged the conflicts. The code is computing (argName, argOpt) by consuming an identifier, ?, and : tokens. And then it redundantly checks optionality with isOptional – the logic is all messed up. I'm guessing the () might come from consuming the type tokens and then parsing an empty expression (which produces undefined, aka ()), or it could be the later logic to make things optional that's broken. It might be better to start the parsing change from scratch.

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

Successfully merging this pull request may close these issues.

None yet

5 participants