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

Allow assertions on mutable object properties. #7252

Conversation

AndrolGenhald
Copy link
Collaborator

Follow up to #4619 to allow assertions on mutable properties as well.
All of the functionality is already there, we just have to not prevent it.

All of the functionality is already there, we just have to not prevent it.
@AndrolGenhald
Copy link
Collaborator Author

AndrolGenhald commented Dec 30, 2021

Also, thanks @SCIF for the property assertion support, I hadn't noticed that until today!
I only just noticed that you excluded this due to references, I'll add some tests for that and see what happens.

@SCIF
Copy link
Contributor

SCIF commented Dec 30, 2021

Hi @AndrolGenhald , thanks for this PR.

I have found another bug #6770 which, probably, is fixed by this PR, so could you please add tests of magic properties?

TBH, I didn't notice rememberPropertyAssignmentsAfterCall config directive at the time of initial implementation. But I was quite confused whether I should restrict the assertion scope with read-only properties and mentioned this in my initial PR. There was no a discussion about it so I designed a check this (pretty blunt) way. Honouring of rememberPropertyAssignmentsAfterCall would be much better way.

@AndrolGenhald
Copy link
Collaborator Author

@SCIF I'll take a look at that one too, currently digging through references and trying to add support for that. It doesn't look like Psalm really tracks references themselves yet, just whether or not they exist.

I'm not sure if rememberPropertyAssignmentsAfterCall should affect whether or not the assertions work. If the annotation is there I feel like it should work regardless, but I could see where it might be confusing. If rememberPropertyAssignmentsAfterCall === false does prevent the assertions from doing anything, the documentation should probably make that clear, or maybe it should just fail with the previous error you had, and change it to mention that config setting?

@AndrolGenhald
Copy link
Collaborator Author

After some thought, I think the solution I was working towards (adding a reference list to Union) isn't a good idea. I also thought about adding something to Context, but that's still going to require a lot of work to make sure the types stay in sync. I think the best solution is to refactor it so that references are stored differently, and actually point to the variable they reference somehow, but that's going to take a lot of work.

In the meantime, I'd prefer to just mark the failing tests as skipped and go ahead and get this merged. With how little most people use references I don't really expect it to be an issue in practice, even though it'd be nice if it worked correctly.

@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Jan 2, 2022
@weirdan weirdan merged commit 697db76 into vimeo:master Jan 2, 2022
@weirdan
Copy link
Collaborator

weirdan commented Jan 2, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature The PR will be included in 'Features' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants