rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

Pattern matching for chars are broken

Open amiralies opened this issue 3 years ago • 17 comments

let isA = c =>
  switch c {
  | 'a' => true
  }

produces this

// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';


function isA(c) {
  return true;
}

exports.isA = isA;
/* No side effect */

without warnigs.

amiralies avatar Jul 15 '22 22:07 amiralies

Indeed thanks for reporting. Moving to the syntax repo.

cristianoc avatar Jul 16 '22 00:07 cristianoc

What makes this a syntax problem? Is this also unicode related?

IwanKaramazow avatar Jul 16 '22 05:07 IwanKaramazow

No quotes are added in the generated code.

cristianoc avatar Jul 16 '22 05:07 cristianoc

Which means it's probably a compiler problem. And might need to transfer this back.

cristianoc avatar Jul 16 '22 05:07 cristianoc

Not sure what the issue is. The generated code looks file. Please reopen with an example that illustrates the issue.

cristianoc avatar Jul 17 '22 04:07 cristianoc

The above isA function is certainly includes a partial matching. It should return true when the input is 'a' and throw Match_failure on other inputs. The generated code for that block is something like this in 9.1.4

function isA(c) {
  if (c === 97) {
    return true;
  }
  throw {
        RE_EXN_ID: "Match_failure",
        _1: [
          "Main.res",
          2,
          2
        ],
        Error: new Error()
      };
}

amiralies avatar Jul 17 '22 07:07 amiralies

Got it. Have you observed anything except char? Is that the only type leading to this issue?

cristianoc avatar Jul 17 '22 07:07 cristianoc

So the No side effect seems like the clue. Perhaps the code is transformed before it even reached the type checker, which would definitely complain.

cristianoc avatar Jul 17 '22 07:07 cristianoc

It seems to be only for chars, e.g. not strings or integers. That's peculiar.

cristianoc avatar Jul 17 '22 07:07 cristianoc

Got it. Have you observed anything except char? Is that the only type leading to this issue?

Didn't see this happening with other types. just char and char interval (which is the same from typechecker pov)

amiralies avatar Jul 17 '22 07:07 amiralies

It seems to be only for chars, e.g. not strings or integers. That's peculiar.

Did something change with Unicode support? I remember chars now accepting codepoints

IwanKaramazow avatar Jul 17 '22 08:07 IwanKaramazow

I've chased down pattern matching to receiving an integer constant, and presumably expecting a char constant.

I think there's no way to change the type of char without adapting the pattern matching also. As the notion of exhaustive matching would change.

@IwanKaramazow do you have any thoughts?

cristianoc avatar Jul 17 '22 13:07 cristianoc

@amiralies is it true that the only issue you have observed is the detection of exhaustiveness?

cristianoc avatar Jul 17 '22 13:07 cristianoc

Perhaps one possibility would be to make char be an alias to int, and try to get int exhaustive matching to kick in. But is that even right?

cristianoc avatar Jul 17 '22 13:07 cristianoc

@amiralies is it true that the only issue you have observed is the detection of exhaustiveness?

Yes. I'm not sure if there are other problems or no. But it seems unused match detection is working somehow

ie

 switch c {
| 'a' => true
| 'a' => false
}

shows a warning

amiralies avatar Jul 17 '22 13:07 amiralies

Seems like it's a lot more involved cf. the original doc: https://docs.google.com/document/d/1V99iVjIncN7seNe-oECa_PU7Sx4ajz8xIH-qDg1vS6Y/edit

IwanKaramazow avatar Jul 17 '22 17:07 IwanKaramazow

I've moved it to milestone v10.1 so there's a little time to figure it out.

cristianoc avatar Jul 18 '22 01:07 cristianoc

Perhaps one possibility would be to make char be an alias to int, and try to get int exhaustive matching to kick in. But is that even right?

This is correct and seems cleaner. Maybe type char = private int ?

bobzhang avatar Oct 23 '22 08:10 bobzhang

Perhaps one possibility would be to make char be an alias to int, and try to get int exhaustive matching to kick in. But is that even right?

This is correct and seems cleaner. Maybe type char = private int ?

That would be nice, though not sure how one would do that in practice. char is used early on in the compiler, as a predefined type, so not sure where type char = private int would go to make it globally visible.

cristianoc avatar Oct 23 '22 09:10 cristianoc

Specifically the constant 'x' would need to have type char, which is not the same as int, but the compiler internally somewhere would need to consider it as int (for pattern matching mechanics) but also not as int (for unification).

cristianoc avatar Oct 23 '22 09:10 cristianoc