Closed lemueldls closed 2 months ago
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: https://github.com/kaleidawave/ezno/blob/a891b69ba49da1eeb0bf106ee2af6fb4363a0714/checker/definitions/full.d.ts#L140
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)
Thanks! I should also mention one of the tests in #98 fail because this
isn't automatically inferred:
The
this
in thetoUpperCase
is half working. I think it should emit aTypeCheckError
at the call site (as at runtime it throwsTypeError: 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 wherelhs=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 newFunctionCallingError::MismatchedThis
. You may also have to add athis: 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.
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.
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!
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 ๐
Hey! A few last things that cause some test to fail (that are kind of their own issues):
FunctionId::AUTO_CONSTRUCTOR
is (not yet) handled.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.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.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?
https://github.com/kaleidawave/ezno/blob/0fa4fa8830baa9506ad1ee2080137ca625a3cd63/checker/src/types/functions.rs#L72-L78
https://github.com/kaleidawave/ezno/blob/0fa4fa8830baa9506ad1ee2080137ca625a3cd63/checker/src/types/functions.rs#L134
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
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.
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.
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 ๐คทโโ๏ธ
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?
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
Also, what happened to staging.md
on main
?
Ok, done.
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.
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.
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!
Looks good! Will merge tomorrow when at computer.
Fixes #98 Fixes #106
Working Tests
Assignable Destructuring
this
unbindingTODO
REVIEW
enum Assignable
abstract to use allASTImplementation
or justEznoParser
? https://github.com/lemueldls/ezno/blob/be38fe2ec683cbfc97fc5ea357a94b5278a99d65/checker/src/features/assignments.rs#L10-L14enum ObjectDestructuringField
keep theName(..)
variant, or share logic withMap { .. }
? (similar toenum AssignableObjectDestructuringField
) https://github.com/lemueldls/ezno/blob/be38fe2ec683cbfc97fc5ea357a94b5278a99d65/checker/src/features/assignments.rs#L24-L34