Skip to content

Conversation

@chengluyu
Copy link
Member

@chengluyu chengluyu commented Dec 24, 2021

This PR fix #49 and add rename operations to Scope. Besides, this PR simplify and improve generated code.

Fix incorrect class name shadowing

Source MLscript

class Test: { x: int }
def Test x = Test { x }
def x = Test 0
def Test a = Test (a + 1)
def y = Test 0
def f Test = Test
def f x = 0

Generated JavaScript

(() => {
function prettyPrint(value) {
  switch (typeof value) {
    case "number":
    case "boolean":
    case "function":
      return value.toString();
    case "string":
      return '"' + value + '"';
    default:
      return value.constructor.name + " " + JSON.stringify(value, undefined, 2);
  }
}
const results = [];
class Test {
  constructor(fields) {
    this.x = fields.x;
  }
}
const Test1 = (x) => new Test({ x: x });
results.push(Test1);
const x = Test1(0);
results.push(x);
const Test2 = (a) => Test1(a + 1);
results.push(Test2);
const y = Test2(0);
results.push(y);
const f = (Test) => Test;
results.push(f);
const f1 = (x) => 0;
results.push(f1);
return results.map(prettyPrint);
})()

Translate pattern matches to ?:

Source MLscript

class A: { a: int }
class B: { b: int }

foo x = case x of { _ -> 1 }
foo x = case x of {
    | A -> x.a
    | _ -> 0
  }
def foo x = case id x of
  { A -> x.a
  | B -> x.b
  | _ -> 1
  }

Generated JavaScript

(() => {
// prettyPrint omitted
const results = [];
class A {
  constructor(fields) {
    this.a = fields.a;
  }
}
class B {
  constructor(fields) {
    this.b = fields.b;
  }
}
const foo = (x) => (x, 1);
results.push(foo);
const foo1 = (x) => (x instanceof A ? x.a : 0);
results.push(foo1);
const foo2 = (x) => {
  let temp;
  return temp = id(x), temp instanceof A ? x.a : temp instanceof B ? x.b : 1;
};
results.push(foo2);
return results.map(prettyPrint);
})()

Note that we cache the result with a temporary variable if there are more than two case branches in order to prevent duplicated evaluation.

@chengluyu chengluyu changed the title Fix re-declaration semantics and improve generated code Fix re-declaration semantics and improve generated code (close #49) Dec 24, 2021
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.

You implementation has at least one small oversight:

def f: (int & 'a) -> (int & 'a) -> 'a
def f a b = if gt a b then a else b

f 1 2

f x y = x

f 1 3

yields:

val f: (int & 'a) -> (int & 'a) -> 'a = (a) => (b) => a > b ? a : b
val f: (int & 'a) -> (int & 'a) -> 'a = (x) => (y) => x
val res: 1 | 2 = 2
val f: 'a -> anything -> 'a = no value
val res: 1 = 1

@chengluyu
Copy link
Member Author

You implementation has at least one small oversight:

def f: (int & 'a) -> (int & 'a) -> 'a
def f a b = if gt a b then a else b

f 1 2

f x y = x

f 1 3

yields:

val f: (int & 'a) -> (int & 'a) -> 'a = (a) => (b) => a > b ? a : b
val f: (int & 'a) -> (int & 'a) -> 'a = (x) => (y) => x
val res: 1 | 2 = 2
val f: 'a -> anything -> 'a = no value
val res: 1 = 1

The output on my side is different from yours.

val f: (int & 'a) -> (int & 'a) -> 'a = (a) => (b) => (a > b ? a : b)
val f: (int & 'a) -> (int & 'a) -> 'a = 2
val res: 1 | 2 = (x) => (y) => x
val f: 'a -> anything -> 'a = 1
val res: 1 = no value

I just pushed the fix. Type check results and execution results should be aligned now.

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 example input in local_testing.html crashes with:

Runtime error occurred:
SyntaxError: Identifier 'temp' has already been declared

I minimized it to:

class Left[A]: { value: A }

def testVal = Left{ value = 1 }
def res = case testVal of
  { Left -> testVal.value
  }

PS: You should be doing these basic tests on your own, not wait that I do them myself.

@chengluyu
Copy link
Member Author

The example input in local_testing.html crashes with:

Runtime error occurred:
SyntaxError: Identifier 'temp' has already been declared

I minimized it to:

class Left[A]: { value: A }

def testVal = Left{ value = 1 }
def res = case testVal of
  { Left -> testVal.value
  }

PS: You should be doing these basic tests on your own, not wait that I do them myself.

Thanks for pointing it out! I did not consider this case after introducing 8d6800f. I should try to keep this PR atomic. 😅

@chengluyu chengluyu mentioned this pull request Jan 2, 2022
10 tasks
@chengluyu chengluyu marked this pull request as draft January 2, 2022 11:34
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.

Okay, this LGTM now. I think we should merge it.

@chengluyu chengluyu marked this pull request as ready for review January 3, 2022 06:10
@chengluyu chengluyu changed the title Fix re-declaration semantics and improve generated code (close #49) Fix re-declaration semantics and refine generated code (close #49) Jan 3, 2022
@chengluyu chengluyu merged commit f8bbd28 into mlscript Jan 3, 2022
@chengluyu chengluyu deleted the scope-rename branch January 3, 2022 06:11
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.

Wrong semantics of re-declarations

3 participants