-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
3b87645
to
2f3b510
Compare
There was a problem hiding this 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.
2f3b510
to
4377e08
Compare
1985094
to
55de2e2
Compare
ffeac9c
to
f407a9d
Compare
There was a problem hiding this 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?
@@ -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}\"]")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool, please do!
0cd39d5
to
46e9287
Compare
e99c62b
to
6373798
Compare
6373798
to
de66f55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now!
Thanks @NeilKleistGao |
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-defineres
. I'm not sure expected results and it may need further discussion.