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

Excess property checks (including the effects of spreads) #42

Closed
HiperMaximus opened this issue Jun 27, 2023 · 5 comments · Fixed by #139
Closed

Excess property checks (including the effects of spreads) #42

HiperMaximus opened this issue Jun 27, 2023 · 5 comments · Fixed by #139
Labels
enhancement New feature or request good-first-issue PRs welcome 🙏 subtyping Related to comparing two types

Comments

@HiperMaximus
Copy link

It would be nice to allow Ezno to have more guards and checks than Typescript. One of those is performing excess property checks on spreading (which Typescript doesn't do), link to the issue: microsoft/TypeScript#39998

I'm pretty sure there are other common footguns that Typescript has which would be nice to avoid here with a more strict typing. Great work!

@kaleidawave kaleidawave added enhancement New feature or request subtyping Related to comparing two types labels Jun 27, 2023
@kaleidawave kaleidawave changed the title Suggestion: allow for Ezno to be stricter than Typescript Excess property checks (including the effects of spreads) Jun 27, 2023
@kaleidawave
Copy link
Owner

This is a good suggestion. I haven't implemented excess property checks or spreading into a object yet but when I get around to it I will have a look at catching this!

@kaleidawave kaleidawave added the good-first-issue PRs welcome 🙏 label Apr 19, 2024
@kaleidawave kaleidawave pinned this issue Apr 19, 2024
@kaleidawave
Copy link
Owner

kaleidawave commented Apr 19, 2024

I think this can be implemented here by raising a warning if expected_type != TypeId::ANY_TYPE and if the expected type does not have a property (aka the get_property == None)

let property_expecting = get_property_unbound(
expected,
Publicity::Public,
&key,
&checking_data.types,
environment,
)
.ok()
.and_then(|l| if let Logical::Pure(l) = l { Some(l.as_get_type()) } else { None })
.unwrap_or(TypeId::ANY_TYPE);

I also think because of this line here that it would work for spread expressions!! 😁

let spread = synthesise_expression(spread, environment, checking_data, expected);

@PatrickLaflamme
Copy link
Contributor

took a look at implementing this. Looks like this approach would result in new excess property warnings in these test cases:

In all three cases there is technically an excess property on the assignment, but curious to get your thoughts on whether that's actually what's desired here.

@kaleidawave
Copy link
Owner

kaleidawave commented Apr 30, 2024

Awesome! Thanks for taking a look @PatrickLaflamme

Yes I understand that Objects Checks and Assignment will raise new warnings for excess properties which is wanted. Once this feature is added you can add a new bullet point to the markdown.

Unsure about the Getters one though, I don’t an excess property there? If you have it working then you can put in a draft PR and I may be able to understand that one :)

@PatrickLaflamme
Copy link
Contributor

PatrickLaflamme commented May 1, 2024

My mistake, it's actually Generic Extends, not Getters.

Agree getters would be a concern.

I'll get my PR opened up in the next day or two.

Update: Just working through it with my employer to get permission to contribute. I have the permission, just need to get set up per their requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good-first-issue PRs welcome 🙏 subtyping Related to comparing two types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants