Fix issues with overlapping argument name and default value, using alias and default value together #5989

mununki · 2023-02-07T18:06:09Z

This PR fixes #5977, #5979

, using alias with default value together

mununki · 2023-02-07T18:07:02Z

jscomp/build_tests/react_ppx/src/recursive_component_test.bs.js

+function make$1(_props) {
+  while(true) {
+    _props = {};
+    continue ;
+  };


Not sure why this js output diff has made after running make test

It should not happen. Try make clean and make test again.

cristianoc · 2023-02-07T18:55:39Z

jscomp/build_tests/react_ppx/src/alias_default_value_test.res

@@ -0,0 +1,18 @@
+@@jsxConfig({version: 4})


Can you add this under jscomp/test instead? So it's compiler any time make lib is run, and you get normal error messages instead of a failed build test.

It would look like this:

@@bs.config({ flags: ["-bs-jsx", "4"], }) module C0 = { @react.component let make = (~a=2, ~b=a + 2) => { React.int(a + b) } }

To me it gives "The module or file props can't be found.".

And same error if I write it in some other project that installs this version of the compiler.

Notice @@bs.config, as @@jsxVersion would not be picked up by a single bsc invocation used to run tests.

cristianoc · 2023-02-07T18:58:03Z

res_syntax/tests/ppx/react/expected/defaultValueProp.res.txt

+  let make = (props: props<'a, 'b>) => {
+    let b = switch props.b {
+    | Some(b) => b
+    | None => a * 2


If you copy paste this, you'll get a a not found.
The oder should be: first let a then b.
Also, it should not be let ... and but let ... let or it won't compile.

Actually, a should refer to the outside scope of make function. The original code is this:

@react.component let make = (~a=2, ~b=a*2) => <div />

So, a in (a* 2) is referring to out of this make function.

This is quite tricky than I guess. It has to be affected by the order, but it should not.

let make = (props) => { let a = 2 // 1 let b = a * 2 // <- a should be reference to the outside of make function, but it refers to 1

We transform the optional labelled argument to nolabel argument props. So there no where to handle the default value that is the root cause of this issue, I guess.

cristianoc · 2023-02-07T18:59:31Z

res_syntax/tests/ppx/react/expected/defaultValueProp.res.txt

+  type props<'a, 'b> = {a?: 'a, b?: 'b}
+  @react.component
+  let make = (props: props<'a, 'b>) => {
+    let b = switch props.b {


Given the error message I'm getting "The module or file props can't be found." this is probably a path A.foo where A happens to be props and not a field access, which it should be.

Field access is Pexp_field.

Oops! Correct.

cristianoc

Left a few comments.
First, to move the test so you get normal user-level error messages.
Then the first error indicated props.b is not done right.
Once that's fixed you should get "a not found".

mununki · 2023-02-08T00:14:27Z

Left a few comments. First, to move the test so you get normal user-level error messages. Then the first error indicated props.b is not done right. Once that's fixed you should get "a not found".

Fixed now.

mununki · 2023-02-08T02:08:33Z

module C0 = {
  let a = 1 // 2
  type props<'a, 'b> = {a?: 'a, b?: 'b}

  let make = (props: props<'a, 'b>) => {
    let b = switch props.b {
    | Some(b) => b
    | None => a * 2 // 1
    }
    and a = switch props.a {
    | Some(a) => a
    | None => 2
    }

    React.int(a + b)
  }
  let make = {
    let \"DefaultValueProp$C0" = (props: props<_>) => make(props)

    \"DefaultValueProp$C0"
  }
}

Due to the list of value bindings, 1 is referring to 2. It doesn't matter the order:

module C0 = {
  let a = 1 // 2
  type props<'a, 'b> = {a?: 'a, b?: 'b}

  let make = (props: props<'a, 'b>) => {
    let a = switch props.a {
    | Some(a) => a
    | None => 2
    }
    and b = switch props.b {
    | Some(b) => b
    | None => a * 2 // 1
    }

    React.int(a + b)
  }
  let make = {
    let \"DefaultValueProp$C0" = (props: props<_>) => make(props)

    \"DefaultValueProp$C0"
  }
}

cristianoc

Still does not apply the fix I was mentioning.
See how this compiles:

let foo = (~a=2, ~b=a * 2) => a + b

but this does not:

module C0 = {
  @react.component
  let make = (~a=2, ~b=a * 2) => {
    React.int(a + b)
  }
}

It's not true that a comes from the outside. And the order does matter. First let a then let b.
That's how default args work.

mununki · 2023-02-08T12:54:05Z

Still does not apply the fix I was mentioning. See how this compiles:
let foo = (~a=2, ~b=a * 2) => a + b
but this does not:
module C0 = {
  @react.component
  let make = (~a=2, ~b=a * 2) => {
    React.int(a + b)
  }
}
It's not true that a comes from the outside. And the order does matter. First let a then let b. That's how default args work.

Sorry, I didn't know how the default value works as same name of argument exactly before:

let f = (~a=2, ~b=a*2, ()) => a + b
// vs.
let f = (~b=a*2, ~a=2, ()) => a + b // error: can't find a

I fixed it edcf127

cristianoc

Looks great.
Added a couple of comments to the examples.

cristianoc · 2023-02-08T13:00:17Z

jscomp/test/alias_default_value_test.res

+})
+
+module C0 = {
+  let a = 1


Can you remove this line. as it's not needed.

Got it 6c0a0bb

cristianoc · 2023-02-08T13:00:52Z

res_syntax/tests/ppx/react/defaultValueProp.res

@@ -0,0 +1,10 @@
+module C0 = {
+  let a = 1


Can you remove this line, as it's not needed.

Got it 6c0a0bb

mununki · 2023-02-08T13:14:43Z

Now I understand that it is self-evident because it is curried.

let f = (~a=1) => { (~b=a*2) => { a + b } }

mununki added 3 commits February 8, 2023 02:57

test result diff

c673fc8

add tests

a415dbe

fix issue overlapping argument with default value

9916513

, using alias with default value together

mununki commented Feb 7, 2023

View reviewed changes

cristianoc reviewed Feb 7, 2023

View reviewed changes

cristianoc requested changes Feb 7, 2023

View reviewed changes

mununki added 2 commits February 8, 2023 09:01

fix build error with Pexp_field

6abbae7

relocate tests

875834c

fix test

ec49a4b

cristianoc requested changes Feb 8, 2023

View reviewed changes

fix handling default value

edcf127

cristianoc approved these changes Feb 8, 2023

View reviewed changes

mununki added 4 commits February 8, 2023 22:15

fix tests

6c0a0bb

update changelog

ba92bfa

fix tests

d36ce45

fix tests

91ead84

mununki merged commit 1a20692 into master Feb 8, 2023

mununki mentioned this pull request Feb 8, 2023

Fix issue overlapping argument with default value and alias with default value rescript-lang/syntax#734

Merged

mununki deleted the fix-alias-default-value-v4 branch May 6, 2023 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with overlapping argument name and default value, using alias and default value together #5989

Fix issues with overlapping argument name and default value, using alias and default value together #5989

mununki commented Feb 7, 2023

mununki Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023 •

edited

Loading

mununki Feb 7, 2023

mununki Feb 7, 2023

mununki Feb 7, 2023

cristianoc Feb 7, 2023

cristianoc Feb 7, 2023

mununki Feb 7, 2023

cristianoc left a comment

mununki commented Feb 8, 2023

mununki commented Feb 8, 2023

cristianoc left a comment

mununki commented Feb 8, 2023

cristianoc left a comment •

edited

Loading

cristianoc Feb 8, 2023

mununki Feb 8, 2023

cristianoc Feb 8, 2023

mununki Feb 8, 2023

mununki commented Feb 8, 2023

Fix issues with overlapping argument name and default value, using alias and default value together #5989

Fix issues with overlapping argument name and default value, using alias and default value together #5989

Conversation

mununki commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

mununki commented Feb 8, 2023

mununki commented Feb 8, 2023

cristianoc left a comment

Choose a reason for hiding this comment

mununki commented Feb 8, 2023

cristianoc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mununki commented Feb 8, 2023

cristianoc Feb 7, 2023 •

edited

Loading

cristianoc left a comment •

edited

Loading