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.

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

Equality (==) does not work properly inflight #4118

Closed
garysassano opened this issue Sep 7, 2023 · 10 comments · Fixed by #5554
Closed

Equality (==) does not work properly inflight #4118

garysassano opened this issue Sep 7, 2023 · 10 comments · Fixed by #5554
Assignees
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler

Comments

@garysassano
Copy link
Collaborator

garysassano commented Sep 7, 2023

I tried this:

app.w

let j = Json { hello: 123, world: [1, 2, 3] };
assert(Json.values(j) == [Json 123, Json [1, 2, 3]]);

test "inflight" {
  let j = Json { hello: 123, world: [1, 2, 3] };
  assert(Json.values(j) == [Json 123, Json [1, 2, 3]]);
}

I run this command:

wing test app.w

This happened:

The preflight code assertion passes, but the inflight assertion fails:

fail  main.wsim » root/env0/test:inflight
      runtime error: assertion failed: Json.values(j) == [Json 123, Json [1, 2, 3]]
         --> ../../.volta/tools/image/packages/winglang/lib/node_modules/winglang/node_modules/@winglang/sdk/src/helpers.ts:27:11
         | 
         | export function assert(condition: any, message: string): asserts condition {
         |   if (!condition) {
      27 |     throw new Error("assertion failed: " + message);
         |           ^
      at assert (/Users/chrisr/.volta/tools/image/packages/winglang/lib/node_modules/winglang/node_modules/@winglang/sdk/src/helpers.ts:27:11)
      at handle (/Users/chrisr/dev/wing-test/main.w:6:3)
      at handler (/Users/chrisr/dev/wing-test/target/test/main.wsim/.wing/handler_c8054fcc.js:17:25)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

I expected this:

I expected Wing assertion to work, because the compiled JS code doesn't throw any error when pasted into Node.js REPL.

preflight.js

const j = ({"hello": 123,"world": [1, 2, 3]});
{((cond) => {if (!cond) throw new Error("assertion failed: Json.values(j) == [Json 123, Json [1, 2, 3]]")})((((a,b) => { try { return require('assert').deepStrictEqual(a,b) === undefined; } catch { return false; } })((Object.values(j)),[123, [1, 2, 3]])))};

Is there a workaround?

No response

Component

Compiler

Wing Version

No response

Node.js Version

No response

Platform(s)

No response

Anything else?

No response

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@garysassano garysassano added the 🐛 bug Something isn't working label Sep 7, 2023
@monadabot monadabot added this to Wing Sep 7, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New - not properly defined in Wing Sep 7, 2023
@garysassano
Copy link
Collaborator Author

Found about this with @hasanaburayyan while doing some tests.

@garysassano garysassano added the 🛠️ compiler Compiler label Sep 7, 2023
@staycoolcall911 staycoolcall911 moved this from 🆕 New - not properly defined to 🤝 Backlog - handoff to owners in Wing Sep 10, 2023
@ekeren
Copy link

ekeren commented Sep 10, 2023

@Chriscbr FYI

@Chriscbr
Copy link
Contributor

Chriscbr commented Sep 10, 2023

It appears to be an issue with how Object.values and equality behave in a VM:

let code = `
const j = {};
const values = Object.values(j);
const expected = [];
console.log(Object.getPrototypeOf(values) === Object.getPrototypeOf(expected));
console.log({}.constructor.prototype === Object.prototype);
require('assert').deepStrictEqual(values, expected);
console.log("done");
`
eval(code);

let vm = require("vm").runInNewContext(code, {
    require,
    Object,
    console,
});
console.log(vm);

error:

node vm.js
true
true
done
false
false
node:assert:124
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal:

[]

    at evalmachine.<anonymous>:7:19
    at Script.runInContext (node:vm:141:12)
    at Script.runInNewContext (node:vm:146:17)
    at Object.runInNewContext (node:vm:300:38)
    at Object.<anonymous> (/Users/chrisr/dev/wing-test/vm.js:12:24)
    at Module._compile (node:internal/modules/cjs/loader:1254:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1308:10)
    at Module.load (node:internal/modules/cjs/loader:1117:32)
    at Module._load (node:internal/modules/cjs/loader:958:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: [],
  expected: [],
  operator: 'deepStrictEqual'
}

@ekeren
Copy link

ekeren commented Sep 10, 2023

I don't think it ever worked

@Chriscbr
Copy link
Contributor

I updated the example in my code above -- it's quite interesting that {}.constructor.prototype === Object.prototype works outside of the VM but not inside it.

In either case, I seems assert.deepStrictEqual is doing a little bit more work than we need it to (according to the NodeJS docs it compares the prototypes of objects -- but I don't think this check needed for our language's equality semantics).

What if we compile a == b into something like $stdlib._equals(a, b), and define our own custom equality logic in the SDK?

@hasanaburayyan
Copy link
Contributor

@Chriscbr freaking dope catch, I did not think about testing this in a vm. I was pretty baffled at why the hell there was a difference.

@staycoolcall911
Copy link
Contributor

I think @Chriscbr's solution makes sense - we should implement our own equality logic instead of the current solution, which uses deepStrictEqual.

@staycoolcall911 staycoolcall911 added this to the Containers milestone Sep 12, 2023
@hasanaburayyan hasanaburayyan self-assigned this Sep 18, 2023
mergify bot pushed a commit that referenced this issue Oct 2, 2023
This is the first time I am writing tests, so admittedly my tests are not that versatile. But as I also believe that std tests should be straightforward, I do not totally mind it. Since `keys()` and `values()` methods of maps seems to return an array that fails to assert "is the same as another array of same elements" (which seems to be a [bug](#4118)), I had to iterate over the array values and confirm it works as expected.

closes: #4363

## Checklist

- [ ] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [ ] Description explains motivation and solution
- [ ] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@Chriscbr Chriscbr changed the title Assertion failed for code that works once compiled to JS Equality behaves differently in preflight and inflight Jan 24, 2024
@Chriscbr
Copy link
Contributor

It looks like the code shared in the original post works since we're executing preflight code inside of a worker thread - but the assertion still fails for inflight code:

test "inflight" {
  let j = Json { hello: 123, world: [1, 2, 3] };
  assert(Json.values(j) == [Json 123, Json [1, 2, 3]]);
}

I've updated the issue title to reflect this.

@Chriscbr Chriscbr changed the title Equality behaves differently in preflight and inflight Equality (==) does not work properly inflight Jan 24, 2024
@MarkMcCulloh
Copy link
Contributor

See #4725, which I think would be an ideal solution for this issue

@mergify mergify bot closed this as completed in #5554 Mar 6, 2024
@mergify mergify bot closed this as completed in 845360a Mar 6, 2024
@github-project-automation github-project-automation bot moved this from 🤝 Backlog - handoff to owners to ✅ Done in Wing Mar 6, 2024
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.59.43.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛠️ compiler Compiler
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants