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

New annotation: @psalm-if-this-is on methods #3782

Merged
merged 35 commits into from Oct 4, 2021
Merged

Conversation

olleharstedt
Copy link
Contributor

@olleharstedt olleharstedt commented Jul 8, 2020

As discussed: #3740

Todo in another PR: psalm-self-out should change to psalm-this-out.

Ignore the commit history, it's because I'm using the same fork for all my PRs. Or I'm stupid. 😄

@olleharstedt
Copy link
Contributor Author

Not sure if the types should be compared with equality or with something else?

@muglug
Copy link
Collaborator

muglug commented Jul 8, 2020

Not sure if the types should be compared with equality or with something else?

You should instead use TypeAnalyzer::isContainedBy, which would in turn mean that the condition in the tests @psalm-if-this-is I is always met (so you'll have to create a new "invalid" testcase).

@olleharstedt
Copy link
Contributor Author

Not sure if the types should be compared with equality or with something else?

You should instead use TypeAnalyzer::isContainedBy, which would in turn mean that the condition in the tests @psalm-if-this-is I is always met (so you'll have to create a new "invalid" testcase).

👍

@olleharstedt
Copy link
Contributor Author

@muglug Even when using isContainedBy, it does not check the interface; the failing test-case still fails. Missing functionality? The test ReconcilerTest::providerTestTypeIsContainedBy also does not check interfaces, only subclassing.

@danog
Copy link
Collaborator

danog commented Jan 29, 2021

This is seriously useful, can't wait to see it merged! :)

@olleharstedt
Copy link
Contributor Author

This is seriously useful, can't wait to see it merged! :)

Didn't work on it much lately. Maybe should give it another go...

@orklah
Copy link
Collaborator

orklah commented Aug 4, 2021

Hi @olleharstedt! Do you intend to work on that again?

@olleharstedt
Copy link
Contributor Author

Hi @olleharstedt! Do you intend to work on that again?

Maybe, if there's interest. Alternatively help someone else take over. I was waiting for muglug to respond to my comment, but now I've learned he's not involved in Vimeo and Psalm anymore. I don't know even know if anyone is merging or reviewing pull requests at the moment.

I'd probably have to start a new PR, I assume lots have changed the last year...

@weirdan
Copy link
Collaborator

weirdan commented Aug 4, 2021

I don't know even know if anyone is merging

That would be me.

or reviewing pull requests at the moment.

Me and @orklah (and anyone interested in doing that, really).

@olleharstedt
Copy link
Contributor Author

I don't know even know if anyone is merging

That would be me.

or reviewing pull requests at the moment.

Me and @orklah (and anyone interested in doing that, really).

Oh, ok! Well, this feature kind of need alias control to be tight. You want to close old PRs?

@orklah
Copy link
Collaborator

orklah commented Aug 4, 2021

You want to close old PRs?

No rush, I just wanted to clean up PR is they were abandoned, but we can keep this open if needed

@olleharstedt
Copy link
Contributor Author

You want to close old PRs?

No rush, I just wanted to clean up PR is they were abandoned, but we can keep this open if needed

Right. Here are some brainfarts I wrote down related to a possible no-alias feature, needed to make type-state safe: #3766

One use-case for this feature is a type-safe query builder, as discussed here: #3740

@orklah
Copy link
Collaborator

orklah commented Aug 4, 2021

I think it could even be used in Psalm's tests itself instead of using an external array of assertions

@olleharstedt
Copy link
Contributor Author

I think it could even be used in Psalm's tests itself instead of using an external array of assertions

Not sure I get it. What could be used? What's an external array of assertions?

@orklah
Copy link
Collaborator

orklah commented Aug 5, 2021

This is what a unit test looks like in Psalm: https://github.com/vimeo/psalm/blob/master/tests/ArgTest.php#L23 it's a little code snippet with variable and the goal is to make Psalm succeed (by not emitting any issue). The other goal is to make variables in the snippet match an assertion array provided just below where each variable must match with a type.

This works great but it's limited to variable in the global scope (I think). With @psalm-if-this-is, I think we should be able to insert those assertions anywhere in the snippet

@danog
Copy link
Collaborator

danog commented Sep 21, 2021

Resumed work on this PR in https://github.com/zoonru/psalm/tree/if-this-is, IDK if I should make a separate PR :)

@olleharstedt
Copy link
Contributor Author

Resumed work on this PR in https://github.com/zoonru/psalm/tree/if-this-is, IDK if I should make a separate PR :)

Awesome! Yes, open a new PR and then I'll close this one.

@danog danog mentioned this pull request Sep 21, 2021
@orklah orklah merged commit 7e0b489 into vimeo:master Oct 4, 2021
@weirdan weirdan added the release:feature The PR will be included in 'Features' section of the release notes label Oct 23, 2021
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

5 participants