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

report invalidCasing when using a class that is not user defined too #8465

Merged

Conversation

kkmuffme
Copy link
Contributor

@kkmuffme kkmuffme commented Sep 7, 2022

e.g. new DateTime

Fix #8456

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Sep 7, 2022

Please do NOT merge this yet

You can use drafts for that 🙂

Looks like it's already catching a couple of casing issues in the tests!

@AndrolGenhald AndrolGenhald marked this pull request as draft September 7, 2022 15:42
@AndrolGenhald AndrolGenhald added the release:feature The PR will be included in 'Features' section of the release notes label Sep 7, 2022
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

You can use drafts for that 🙂

Thanks :-)

@kkmuffme kkmuffme force-pushed the invalid-class-casing-for-native-and-stubbed-classes branch from f0c2595 to 6849a2a Compare September 8, 2022 11:26
@kkmuffme kkmuffme force-pushed the invalid-class-casing-for-native-and-stubbed-classes branch from 6849a2a to 1a10654 Compare September 8, 2022 11:28
@kkmuffme kkmuffme marked this pull request as ready for review September 8, 2022 11:28
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

Ready to be merged.

@AndrolGenhald
Copy link
Collaborator

I don't see this part, is it not necessary for some reason?

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

This part is not strictly necessary for class casing check, but only for aliases. Also this requires a lot of other changes and it's a bitch to get working/correct with how cache handles this, so I will PR this separately, bc I don't know when I will have time to check it.

@AndrolGenhald
Copy link
Collaborator

@orklah it looks fine to me, but I don't know why internal classes were excluded in the first place, so that makes me a bit nervous just merging this without investigating that first.

@orklah
Copy link
Collaborator

orklah commented Sep 8, 2022

@orklah it looks fine to me, but I don't know why internal classes were excluded in the first place, so that makes me a bit nervous just merging this without investigating that first.

I'm not surprised given how many errors it raises in Psalm itself. I'm not even sure the documentation was correct on older versions of PHP for things like DOMElement.

@kkuffme can you fix the remaining casing errors and it should be good to go

@AndrolGenhald
Copy link
Collaborator

I'm not surprised given how many errors it raises in Psalm itself.

I didn't even notice those 😆 I pay less attention to that now because I know it's been failing. We should really get that fixed soon...

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

@orklah thanks, I didn't even notice them, as Shepherd awlays reports a false positive, so I tend not to check it anymore at all.

All fixed now.

vendor/nikic/php-parser/lib/PhpParser/Node/UnionType.php:18:24: InvalidPropertyAssignmentValue: $this->types with declared type 'array<array-key, PhpParser\Node\Identifier|PhpParser\Node\Name>' cannot be assigned type 'array<array-key, PhpParser\Node\Identifier|PhpParser\Node\IntersectionType|PhpParser\Node\Name>' (see https://psalm.dev/145)

This is not from this branch, this is an issue created somewhere else/exists on 4.x branch already.

@AndrolGenhald
Copy link
Collaborator

What about:

src/Psalm/Config.php:755:18: UnnecessaryVarAnnotation: The @var DOMElement|null annotation for $psalm_node is unnecessary

Why is that showing up now? Did the incorrect casing cause the redundancy to be missed? If so maybe we should also open an issue to fix UnnecessaryVarAnnotation to ignore casing.

@kkmuffme kkmuffme force-pushed the invalid-class-casing-for-native-and-stubbed-classes branch from c372f09 to d0984f4 Compare September 8, 2022 17:35
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

Yes, that's new now, fixed it.

There might be an issue there, I will check https://psalm.dev/r/84fbd22c02 once this PR is merged and open a separate issue for it if necessary.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/84fbd22c02
<?php

use dateTime;

/**
 * @param dAteTime $arg
 * @return void
 */
function a( $arg ) {
	/** @var daTeTime $arg */
    echo $arg->format('Y-m-d H:i:s');
}
Psalm output (using commit afe85fa):

ERROR: UnnecessaryVarAnnotation - 10:11 - The @var dateTime annotation for $arg is unnecessary

@AndrolGenhald
Copy link
Collaborator

Hmm, that one already shows an error though. It's weird that fixing the casing on DOMElement would cause that UnnecessaryVarAnnotation to show up if it already shows up for other stuff with incorrect casing.

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Sep 8, 2022

I'm not sure what's happening there, but it's unrelated to this PR but a separate issue. It would be much easier to tackle this on a separate PR though, if there is an issue.

@orklah
Copy link
Collaborator

orklah commented Sep 8, 2022

Thanks! Let's fix the unnecessary var in another PR indeed

@orklah orklah merged commit 6374a96 into vimeo:4.x Sep 8, 2022
@kkmuffme kkmuffme deleted the invalid-class-casing-for-native-and-stubbed-classes branch September 8, 2022 19:05
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.

InvalidClass does not work PHP native classes and stubs
3 participants