Object destructuring, this unbinding and checking

[lemueldls]

Merges into checker-improvements-153 for current class behavior

Fixes #98
Fixes #106

Working Tests

Assignable Destructuring

const o = { a: 1, b: { v: 2, c: 3 } };

let a, b, c, x;
({
  a,
  x = o.b.c++,
  b: { v: b, c = 7 },
} = o);

print_type({ a, b, c, x, o }); // { a: 1, b: 2, c: 4, x: 3, o: { a: 1, b: { v: 2, c: 4 } } }

this unbinding

const { f } = new (class {
  f() {
    return this;
  }
})();

print_type(f()); // undefined

const { toUpperCase } = "hi";

print_type(toUpperCase()); // undefined

TODO

• Object spread operator (requires enumerability)
  • See Traversing object properties
  • See Object.defineProperty()
  • See Object.getOwnPropertyDescriptors()
• Array desctructuring (requires iterability)
  • See Symbol.iterator

REVIEW

• Should enum Assignable abstract to use all ASTImplementation or just EznoParser? https://github.com/lemueldls/ezno/blob/be38fe2ec683cbfc97fc5ea357a94b5278a99d65/checker/src/features/assignments.rs#L10-L14
• Should enum ObjectDestructuringField keep the Name(..) variant, or share logic with Map { .. }? (similar to enum AssignableObjectDestructuringField) https://github.com/lemueldls/ezno/blob/be38fe2ec683cbfc97fc5ea357a94b5278a99d65/checker/src/features/assignments.rs#L24-L34

[kaleidawave]

Whoa this is legendary. Not one but two issues! (also good idea to base it on my WIP branch)

Bit busy to do a checkout and review line-by-line and test ATM.

But had a quick look and splitting up the methods is a great idea!

In terms of the object destructuring assignment that is great! Yes I think they are spreads and enumerability right? I know that I have the concept of a property being enumerable (in LocalInformation) but I haven't done anything with it yet. I don't think it is a priority for now.

The array destructuring is the other one. I think a few a weeks I realised that it is not as simple as const [a, b] = c being equal to const a = c[0], b = c[1]. You can leave it under todo!() for now. I will get iterators working at some point, but it is a bit complex for now.

The this in the class example is perfect and I complete!

The this in the toUpperCase is half working. I think it should emit a TypeCheckError at the call site (as at runtime it throws TypeError: String.prototype.toUpperCase called on null or undefined). That is a fault of my own. If you would like to add the fix for that you can add a subtype check in the calling.rs code here https://github.com/kaleidawave/ezno/blob/checker-improvements-153/checker/src/types/calling.rs#L925-L943 where lhs=get_constraint(free_this_id), rhs=value_of_this. Similar to the parameter checking code, if that fails then add an error (will require a new FunctionCallingError::MismatchedThis. You may also have to add a this: string parameter here:

ezno/checker/definitions/full.d.ts

Line 140 in a891b69

toUpperCase(): string;

I can do that part if you want. But if you could get an function calling error raised for invalid this then that would be amazing (and something TSC doesn't catch https://www.typescriptlang.org/play?#code/MYewdgzgLgBA3jKICqAHVBTATgYQIYQYwC+MAvDAEQAWAlpQNwBQToksAHuYiutvoQAUASmYB6MTCkwAegH4gA).

---

Additionally you can add 3 new tests (object des assignment, class this, string/internal this error) for these additions (see #100). You can put them in staging.md. (not sure why CI didn't run on PR) but you can see these tests passing with cargo test -p ezno-checker-specification. (not all tests will pass because this is not on the main branch. see https://github.com/kaleidawave/ezno/actions/runs/8366728527/job/22907563708 for the current ones failing)

[lemueldls]

Thanks! I should also mention one of the tests in #98 fail because this isn't automatically inferred:

The this in the toUpperCase is half working. I think it should emit a TypeCheckError at the call site (as at runtime it throws TypeError: String.prototype.toUpperCase called on null or undefined). That is a fault of my own. If you would like to add the fix for that you can add a subtype check in the calling.rs code here https://github.com/kaleidawave/ezno/blob/checker-improvements-153/checker/src/types/calling.rs#L925-L943 where lhs=get_constraint(free_this_id), rhs=value_of_this. Similar to the parameter checking code, if that fails then add an error (will require a new FunctionCallingError::MismatchedThis. You may also have to add a this: string parameter here:

I got toUpperCase to fail correctly doing exactly that, though I don't think adding this: string mattered anyway.

Explicitly defining this for the latter test does fail because of unbinding, but I also found that this as a parameter isn't checked on the object itself (b is set to type boolean here):

I am not sure if this as a parameter should be checked, considering some existing library authors might want this behavior for rebinding the this value of functions or arrow functions inside an object.

Maybe this could be checked during abstract interpretation wherever it is used, so any a method does not rely on this works, like so:

const { f } = new (class {
  f() {
    return 42;
  }
})();

print_type(f()); // should be `42`
//         ^? error

This example in particular fails only for classes, in this case, warn for non-static methods not using this?

I'll push the commits right now, along with the specification tests.

[lemueldls]

Oh? running cargo run -p ezno-checker -F ezno-parser --example cache ./checker/definitions/full.d.ts ./checker/definitions/internal.ts.d.bin fails now because of the this restrictions (something to do with generics?):

EDIT: Adding this: Array<T> to the push method fixed it.

I could try to implicitly define this for methods now, or it could be it's own issue.

[kaleidawave]

Awesome this looks ready to merge! I am a bit busy ATM, so will sort next weekend :) Excited to include the this checking into the next release!

[kaleidawave]

Hey, just going to finish and merge somethings in #126. Then will update this PR to point at the main branch (that way should keep things tidy and retain committer information). Will fix any other issues and conflicts that come up, then merge this afternoon 🎉

[lemueldls]

Hey! A few last things that cause some test to fail (that are kind of their own issues):

• All classes with no explicit constructor call the exact same function because of how FunctionId::AUTO_CONSTRUCTOR is (not yet) handled.
• When comparing classes to objects (class X {}; ({ n: 2 }) satisfies X), it does not throw an error anymore. This is new behavior because of de80d9, and causes satisfies number and satisfies boolean test to not throw, because they are internally empty classes, and are compared by key-values. This commit can be reverted if there are better ways of passing other tests without it, and be strict about classes being "narrowed down" to objects, but not the other way around.
• Because this values are kind of checked, test that use this: { ... } for methods fail, either because of extends support, or because the property trying to be access isn't defined in the top level of the class definition.

[kaleidawave]

All classes with no explicit constructor call the exact same function because of how FunctionId::AUTO_CONSTRUCTOR is (not yet) handled.

Ah yes, good investigation! FunctionId is based on the byte position of the start of a function and the source id. I think I added FunctionId::AUTO_CONSTRUCTOR because there wasn't a clear function associated with the constructor. I think removing FunctionId::AUTO_CONSTRUCTOR and changing the following to take the position (SpanWithSource) of the class declaration (and building a FunctionId) should fix any issues?

ezno/checker/src/types/functions.rs

Lines 72 to 78 in 0fa4fa8

	pub(crate) fn new_auto_constructor<T: crate::ReadFromFS, A: crate::ASTImplementation>(
	class_prototype: TypeId,
	extends: Option<TypeId>,
	properties: ClassPropertiesToRegister<A>,
	environment: &mut Environment,
	checking_data: &mut CheckingData<T, A>,
	) -> Self {

ezno/checker/src/types/functions.rs

Line 134 in 0fa4fa8

id: crate::FunctionId::AUTO_CONSTRUCTOR,

When comparing classes to objects (class X {}; ({ n: 2 }) satisfies X), ...

and

Because this values are kind of checked, test that use this: { ... }

Ahh I hadn't seen those new commits. One thing I was experimenting with a while ago was treating classes nominally (aka don't do property checking if they are on the LHS of subtype comparison, only look at constant base type and in some cases object prototypes). Should write up a issue around it (I know TS does allow structural checks when the a class is the LHS of subtyping, maybe there is a compromise where only string, boolean, number are treated as nominal under a flag).

Is there an example for adding structural subtyping for class checks? const x: { a: 2 } = new class X { a: 2 } will work with nominal class (aka X can be passed as this to a method expecting this: { a: number }) just not the other way round (e.g. const x: X = { a: 2 } wouldn't error)?. See #128

[kaleidawave]

Looks like I got bit overconfident in my git ability. Changing the target branch has brought in three commits from #126. Trying to figure it out RN: https://stackoverflow.com/questions/78293184/changing-target-branch-on-github-pr-has-added-additional-commits?noredirect=1#comment138028245_78293184

---

I think you have to run git rebase a34475060133e2149848f6abad43a8eeb7bf12bc lemueldls:object-destructuring --onto main, make a couple changes per commit (I did it with vscode) and once finished force push to remove the old commits. I can do the rebase but unfortunately can't push it (even force push it) because the fork isn't my repository. I could do a open a new PR and cherry pick from this?

In hindsight just merging into that other PR would have been a lot easier 😅 I have learned nothing from #44.

[lemueldls]

Oh wow, these are a lot of merge conflicts... Are you sure you need to force push? I just ran git merge origin/main, and I think that suffices. I won't have time today to fix all of these, but if you don't merge by tomorrow night, let me know if I can try sometime.

[kaleidawave]

I think your git merge origin/main works because your local hasn't updated to the latest main. Unfortunately I think the only way is the rebase and force push above. That should fix the merge conflicts while keeping this in this PR

It is a shame there is no undo button here

---

Alternatively I think I can fix it in another PR (#129) and just link back to this PR from there 🤷‍♂️

[lemueldls]

I think your git merge origin/main works because your local hasn't updated to the latest main.

I pulled from upstream and merged with my own repo's origin. What kind of issues are you getting from merging directly to main? GitHub's PR base branch shouldn't affect whatever you're doing with git (PR's are a GitHub concept).

I can do the rebase but unfortunately can't push it (even force push it) because the fork isn't my repository.

That's strange since I enabled contributor editing from the start. If that's causing issues I have some time now to try out fixing up some of the merge conflicts, since they look to be straight-forward to fix (I hope).

And I don't even know how you did this so silently, was this a force push?

[kaleidawave]

Awesome, good luck with the conflicts 😀. I will have a look tomorrow at those commands. Just worried about putting a commit that says there a 5k lines of changes but they are already under main 🤨. I think it is because the force push is to your forked repo which I don't have write access to? Maybe not sure

[lemueldls]

Also, what happened to staging.md on main?

[lemueldls]

Ok, done.

[kaleidawave]

During development while I am still working on a PR I like to separate the new features tests from the existing tests as it helps to isolate regressions. Once it is finished and I know they all work I merge them into the complete specification.md.

Once you know your new ones have passed, just before this is ready to merge you can put them into specification.md.

[lemueldls]

All specification tests are passing now (through temporary exceptions).

I could probably fix the CI job, Check checker without default features tomorrow night, since I think it'll involve changing a lot of function to use some A: crate::ASTImplementation until I can use EznoParser without a feature in some other crate.

[kaleidawave]

Awesome! Wow that's good that the conflicts have solved and the change count has come back down, I didn't think that was possible.

Yes the only place where ezno-parser can be used in the checker is in the synthesis directory. The rest of the library is meant to be agnostic so other tools with their own AST definitions can use type checking. Although there isn't any users of that atm it is a good thing to keep in the future. Unfortunately that means there might be some similar/duplicate data structures for somethings to provide and interim layer but that's fine. I think if you could do that as you last worked in it.

After that just some small clippy things which you can add ignores for if the resolution is not straightforward.

Also could you add a test for the new object destructuring assignment? That would round the passing tests up to a nice number!

[kaleidawave]

Looks good! Will merge tomorrow when at computer.

[kaleidawave]

Looks like nodejs was recently was burned by the destructuring this issue 👀 . nodejs/node#53934

Seems this case with destructuring isn't well know. Nothing on MDN about it https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment