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

Fix the res running variable is not consistent with DiffTests #113

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

NeilKleistGao
Copy link
Member

Relative Issue: #65

Since every statement will change the res variable, I tried to add the type information into the context so that the correct type could be found. I also used a flag variable to indicate self-define res. I'm not sure expected results and it may need further discussion.

@NeilKleistGao NeilKleistGao force-pushed the consistent branch 3 times, most recently from 3b87645 to 2f3b510 Compare June 21, 2022 02:00
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.

Cool, thanks! I'd just a minor change to be done, as explained below.

shared/src/test/scala/mlscript/DiffTests.scala Outdated Show resolved Hide resolved
shared/src/test/scala/mlscript/DiffTests.scala Outdated Show resolved Hide resolved
shared/src/test/scala/mlscript/DiffTests.scala Outdated Show resolved Hide resolved
@NeilKleistGao NeilKleistGao reopened this Jun 23, 2022
@NeilKleistGao NeilKleistGao force-pushed the consistent branch 9 times, most recently from 1985094 to 55de2e2 Compare June 29, 2022 05:49
@NeilKleistGao NeilKleistGao marked this pull request as draft June 29, 2022 06:02
@NeilKleistGao NeilKleistGao force-pushed the consistent branch 3 times, most recently from ffeac9c to f407a9d Compare June 29, 2022 08:18
@NeilKleistGao NeilKleistGao marked this pull request as ready for review June 29, 2022 08:19
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.

The outputs look correct. But I'm having trouble figuring out if this implementation is doing the right thing. It looks like it's adding hacks here and there instead of cleanly solving the issue. @chengluyu can you please review and suggest a better way?

shared/src/test/diff/mlscript/Issue65.mls Outdated Show resolved Hide resolved
shared/src/test/diff/mlscript/Under.mls Show resolved Hide resolved
@@ -606,7 +605,14 @@ class JSTestBackend extends JSBackend {
case term: Term =>
try {
val body = translateTerm(term)(scope)
JSTestBackend.CodeQuery(scope.tempVars.emit(), (resultIdent := body) :: Nil)
val code = body match {
case JSIdent(name) if (name.indexOf('\'') > -1) => (resultIdent := JSIdent(s"globalThis[\"${name}\"]"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a huge hack. Making this logic rely on the fresh name encoding of the Scope class is totally brittle (what if we want to change it later?). You should add the correct logic earlier, nearer to the place where this thing is decided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait so this logic was not needed at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found something wrong in shared\src\test\diff\mlscript\BadInherit2.mls, where:

f'
//│ res: 1
//│    = A1 { Foo1: [Function (anonymous)] }

Initially, I thought it might be a mistake because f' should be 1, so I added this check to avoid it. However, I found it wrong in the original file:

f'
//│ res: 1
//│    = 0

So I think maybe it should not print 1 at all, and this check is redundant (or at least should be fixed in another pr).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see! This is actually an instance of #65 (comment)

It works without the tick:

def f_ = 1
//│ f_: 1
//│   = 1

f_
//│ res: 1
//│    = 1

We need to replace ticks in JS with some other symbol. This should be doable with a small modification to the scope class. Could you do it in this PR – or in another one if you prefer?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can fix it in another PR later. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool, please do!

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.

LGTM now!

@LPTK LPTK merged commit 0bb42a2 into hkust-taco:mlscript Jul 12, 2022
@LPTK
Copy link
Contributor

LPTK commented Jul 12, 2022

Thanks @NeilKleistGao

@NeilKleistGao NeilKleistGao deleted the consistent branch August 6, 2022 02:46
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.

3 participants