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

Generate argument number checks for JS code #237

Merged
merged 12 commits into from
Nov 22, 2024

Conversation

CrescentonC
Copy link
Member

@CrescentonC CrescentonC commented Nov 19, 2024

I modified JSBuilder.scala such that now three places related to function/method codegen call the method handleFunction to handle the common logic. The handleFunction is overridden in JSBackendDiffMaker.scala such that argument number sanity checks are implemented using varargs in javascript.

I checked and also learned that in javascript arguments.length can also be used to determine how many arguments are actually passed into a function, but we generate arrow functions which do not have arguments.

@CrescentonC CrescentonC force-pushed the hkmc2-js-sanity-check branch from b6e7e92 to dab1ff2 Compare November 19, 2024 15:57
@CrescentonC CrescentonC marked this pull request as ready for review November 19, 2024 16:16
Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

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

Nicely done, thanks! But please address my comments.

hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/jvm/src/test/scala/hkmc2/JSBackendDiffMaker.scala Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/codegen/SanityCheck.mls Outdated Show resolved Hide resolved
hkmc2/shared/src/test/mlscript/codegen/SanityCheck.mls Outdated Show resolved Hide resolved
* remove unnecessary whitespace change
* change `handleFunction` to `setupFunction` and its parameter names
* rename `JSBuilderSanityCheck` to `JSBuilderSanityChecks` and make it a trait which takes an `instrument: Bool` parameter and extends `JSBuilder`
* use `TempSymbol` for vararg names
* rename test file name to `SanityChecks.mls` and remove unnecessarily redefined sanity-checked function
@CrescentonC
Copy link
Member Author

Thanks for the comments! I have made the changes except for the one that makes sanity checks default, because currently several tests have function calls with incorrect number of arguments, but without :re. Should I make the change to add :re to them and make sanity checks default?

Also previously I use const x = args[i] to assign arguments values from varargs, but I just saw #238 and realize that arguments can be mutated, so I change to use let.

@LPTK
Copy link
Contributor

LPTK commented Nov 20, 2024

currently several tests have function calls with incorrect number of arguments, but without :re. Should I make the change to add :re to them

Well, most are probably actual mistakes! Unless the intent was clearly to test what happens when the wrong number of arguments is passed, please just fix them by passing the correct number of arguments!

* remove val for the `instrument` parameter
* fix tests
@CrescentonC
Copy link
Member Author

Thanks for the comments! I have removed the val, and made the following changes to the tests and made sanity check on by default.

  • add an :re at line58 in ClassInFun.mls
  • add missing parameters at around line120 in IfThenElse.mls
  • add an :re at line40 in Juxtapositions.mls
  • add a :fixme at line 170 in Juxtapositions.mls

Copy link
Contributor

@LPTK LPTK left a comment

Choose a reason for hiding this comment

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

Great! Just one last touch. And the errors would be more helpful if they had the following format:

Function 'foo' expected 3 arguments but got 2.

@LPTK LPTK merged commit 06550da into hkust-taco:hkmc2 Nov 22, 2024
1 check passed
@LPTK LPTK deleted the hkmc2-js-sanity-check branch November 22, 2024 03:21
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.

2 participants