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.

Sign up for GitHub

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

Jump to bottom

Negation #7138

Merged
merged 11 commits into from
Oct 31, 2024
Merged

Negation #7138

merged 11 commits into from
Oct 31, 2024

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Oct 31, 2024 •
edited
Loading

Better logic to simplify negation.

For comparison,

@unboxed
type t = | @as(null) Null | @as(undefined) Undefined | B(bool) | S(string) | I(int)

let check_null_eq_typeof = x =>
  switch x {
  | B(_) if x == Null => 3
  | _ => 4
  }

let check_null_neq_typeof = x =>
  switch x {
  | B(_) if x != Null => 3
  | _ => 4
  }

on latest playground generates:

function check_null_eq_typeof(x) {
  if (typeof x !== "number" && typeof x !== "string" && typeof x !== "boolean" || !(typeof x === "boolean" && x === null)) {
    return 4;
  } else {
    return 3;
  }
}

function check_null_neq_typeof(x) {
  if (typeof x !== "number" && typeof x !== "string" && typeof x !== "boolean" || !(typeof x === "boolean" && x !== null)) {
    return 4;
  } else {
    return 3;
  }
}

while here it generates:

function check_null_eq_typeof(x) {
  return 4;
}

function check_null_neq_typeof(x) {
  if (typeof x !== "boolean") {
    return 4;
  } else {
    return 3;
  }
}

For reference, the code without untagged variants is the following:

function check_null_eq_typeof(x) {
  if (typeof x !== "object" || !(x.TAG === "B" && x === "Null")) {
    return 4;
  } else {
    return 3;
  }
}

function check_null_neq_typeof(x) {
  if (typeof x !== "object" || !(x.TAG === "B" && x !== "Null")) {
    return 4;
  } else {
    return 3;
  }
}

github-actions[bot]
github-actions bot reviewed Oct 31, 2024
View reviewed changes
Copy link

@github-actions github-actions bot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Syntax Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: e07ee33 Previous: e1b7fb7 Ratio
Print Napkinscript.res - time/run 81.46559703999999 ms 77.00100409999999 ms 1.06

This comment was automatically generated by workflow using github-action-benchmark.

@cristianoc cristianoc requested a review from zth October 31, 2024 12:25
zth
zth approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@zth zth left a comment

Choose a reason for hiding this comment

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

So good! Seems you need to rebuild the Core tests since some generated code there changed as well here.

@cknitt
Copy link
Member

cknitt commented Oct 31, 2024

The change from the Core tests does not look correct though?

-  tmp = (tNull === null || tNull === undefined) && tNull === null ? true : false;
+  tmp = tNull === null || tNull === undefined ? true : false;

@cknitt
Copy link
Member

cknitt commented Oct 31, 2024

BTW, can we omit the ? true : false? 🙂

@cristianoc
Copy link
Collaborator Author

The change from the Core tests does not look correct though?

-  tmp = (tNull === null || tNull === undefined) && tNull === null ? true : false;
+  tmp = tNull === null || tNull === undefined ? true : false;

Updating it.
Currently need to touch source files at different directory levels, to get these to be rebuilt.

@cristianoc
Copy link
Collaborator Author

@cknitt please check again.

@cristianoc
Copy link
Collaborator Author

BTW, can we omit the ? true : false? 🙂

Here: #7139

cknitt
cknitt reviewed Oct 31, 2024
View reviewed changes
tests/tests/src/UntaggedVariants.mjs
@@ -386,9 +386,9 @@ function check$1(s) {
return;
}
let match = s[0];
if (match === undefined || match === null || match === true) {
if (match === true) {
let match$1 = s[1];
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't this actually incorrect before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it was incorrect

tests/tests/src/UntaggedVariants.mjs
let match$1 = s[1];
if (match$1 === undefined || match$1 === null || match$1 === false) {
if (match$1 === false) {
let match$2 = s[2];
if (match$2 === undefined || match$2 === null || match$2 === false || match$2 === true) {
console.log("Nope...");
Copy link
Member

Choose a reason for hiding this comment

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

Can't comment further below for some reason, and probably out of scope of this PR, but

          if (match$3 === undefined || match$3 === null || match$3 === false || match$3 === true) {
            console.log("Nope...");
            return;
          }
          if (typeof match$3 === "string" && match$3 === "My name is") {

could just be

          if (match$3 === "My name is") {

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can you move this to an issue? after this PR
to keep things from growing too much

@cknitt cknitt mentioned this pull request Oct 31, 2024
Merged
@cristianoc cristianoc merged commit 41e2fd6 into master Oct 31, 2024
20 checks passed
@cristianoc cristianoc deleted the negation branch October 31, 2024 15:08
@cknitt cknitt mentioned this pull request Oct 31, 2024
Closed
@tsnobip
Copy link
Contributor

tsnobip commented Nov 1, 2024

Better logic to simplify negation.

For comparison,

@unboxed
type t = | @as(null) Null | @as(undefined) Undefined | B(bool) | S(string) | I(int)

let check_null_eq_typeof = x =>
  switch x {
  | B(_) if x == Null => 3
  | _ => 4
  }

let check_null_neq_typeof = x =>
  switch x {
  | B(_) if x != Null => 3
  | _ => 4
  }

@cristianoc Couldn't we raise a compiler warning in such cases because the if guards are incorrect here? I have no idea of how feasible/complex this would be to implement though.

@cristianoc
Copy link
Collaborator Author

It's just a trick to test logical simplifications.
Not sure how common this case is to warrant a dedicated warning.
If it turns out to be common, it can be considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@cknitt cknitt cknitt left review comments

@zth zth zth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cristianoc @cknitt @tsnobip @zth