update property map

[discordier]

This is an attempt at fixing #7983 by:

1. adding a (at least more) correct doc parsing approach than in phan/phan.
2. updating the property map with said parser.

Currently tests fail on my machine but I submit anyway to have a discussion base if this is the general correct direction.

[discordier]

For the code style issue, shall I suppress the phpcs issue or change the function to a variable?

[discordier]

We should also decide how we want to handle "manual" classes which are currently dropped by this PR as there is no documentation.

[orklah]

I think the offset should be a lowercase string

[discordier]

Shall I also lowercase the property names?
They were so in the original file but I currently keep them as documented in the XML files.

[orklah]

Interesting:

psalm/src/Psalm/Internal/Codebase/PropertyMap.php

Line 35 in 97e6511

$cased_key = strtolower($key);

Psalm does a strtolower by itself on every class name. It would be best to do it on generation instead of in execution (we could drop the whole loop then). There is no such requirement on property names (I'm also not sure they're case insensitive)

[discordier]

Did both in latest commit.
The removal of the loop I left for a later commit/PR, as we have some more strtolower() calls over the code base where the property map is used.

[orklah]

using strtolower when using propertymap is expected, we'll retrieve all sort of case from user code that we need to normalize

[orklah]

We should also decide how we want to handle "manual" classes

Yeah, this will cause issues. I'm fine with keeping an automatically generated file, but if we do, we should create a second one with manual additions.

shall I suppress the phpcs issue or change the function to a variable

I think you can suppress it. Bin files are pretty special in that regard

[AndrolGenhald]

we should create a second one with manual additions

And removals I think. For stuff overridden by stubs it would probably be less confusing if they were omitted from the PropertyMap instead of being unused.

[discordier]

we should create a second one with manual additions

And removals I think. For stuff overridden by stubs it would probably be less confusing if they were omitted from the PropertyMap instead of being unused.

I am unsure how I should approach the removal file.
In the end, it should be a direct map between class with properties and stub, can you elaborate on your idea?

[orklah]

removal could be as simple as having a key in the second file that match the first file but without any property. If we merge the two arrays, the second key will simply overwrite the first. Maybe not very self-explanatory...

[AndrolGenhald]

In the end, it should be a direct map between class with properties and stub, can you elaborate on your idea?

Stubs can support much more than the property map, and if a stub exists it seems like it might be confusing to future contributors for the property map to exist as well. See eg this stub for DOMNode that marks most properties as @readonly.

Also, I would like to get Psalm to not load extension classes unless the extension is required by the project. I'm not entirely sure how the property map works with that (I think maybe property map doesn't matter unless it's in callmap as well?), but I think it would be best if fully stubbed classes from PHP extensions didn't exist in the property map.

removal could be as simple as having a key in the second file that match the first file but without any property.

Haven't looked at the implementation, but maybe something like ["domnode" => null, "otherclasstoremove" => null, ...] with array_diff_key? I'm not sure if pointing a class to an empty array in the property map has any significance, but it's probably better if it's removed entirely. (Note that the DOMNode stub example is currently an unmerged PR, so don't actually remove it from the property map just yet...)

[discordier]

Implemented the extraction of stub class names and skipping them when generating the propertymap.

Checking the tests, I found that the stubs do not define the properties at all.
Shall I add them in this PR?

I also found a little discrepancy for DOMElement::attributes
Old code is:

psalm/dictionaries/PropertyMap.php

Line 321 in 4ce8a06

'attributes' => 'DOMNamedNodeMap<DOMAttr>',

While PHP Docs state (for parent class DOMNode): DOMNamedNodeMap|null.
Shall I add the attributes to both (DOMNode and DOMElement) in the stub? The first one nullable and the second not?

What do you think?

[AndrolGenhald]

Checking the tests, I found that the stubs do not define the properties at all.
Shall I add them in this PR?

I wouldn't bother, that should mostly be handled by #7641 whenever I get the time and motivation to finish it (hopefully soon).

I also found a little discrepancy for DOMElement::attributes

I went through most of these by hand pretty extensively for #7641, and my comment here suggests that it's intentional. You can look into it further to confirm if you'd like, iirc I actually checked php-src for those to make sure they couldn't be null.

[discordier]

I wouldn't bother, that should mostly be handled by #7641 whenever I get the time and motivation to finish it (hopefully soon).

So how shall I'll fix the tests regarding the property maps?
Most of them get resolved when using the stubs from #7641.

However, one is remaining and seems fishy to me.

psalm/tests/PropertyTypeTest.php

Line 510 in 3aadec6

echo $a->name . " - " . $a->class;',

Where does the property name come from? The tested class does not declare it and does not extend any parent or use traits.
So the failure seems correct to me and therefore the test seems wrong.

I went through most of these by hand pretty extensively for #7641, and my comment here suggests that it's intentional. You can look into it further to confirm if you'd like, iirc I actually checked php-src for those to make sure they couldn't be null.

Looks good to me.

Can we make a roadmap on this?
IMO this now depends on #7641.
Can I help to speed that one up?
What is missing?

[orklah]

The tested class does not declare it and does not extend any parent or use traits.

Documentation do mention it though: https://www.php.net/manual/en/class.reflectionmethod.php
It seems to come from https://www.php.net/manual/en/class.reflectionfunctionabstract.php

[discordier]

The tested class does not declare it and does not extend any parent or use traits.

Documentation do mention it though: https://www.php.net/manual/en/class.reflectionmethod.php It seems to come from https://www.php.net/manual/en/class.reflectionfunctionabstract.php

You are right, it is missing in

psalm/stubs/Reflection.phpstub

Line 93 in 3aadec6

class ReflectionMethod implements Reflector

In fact, the whole inheritance is also misisng in there, most only implement Reflector.

After updating the stub files locally, all went smooth.

So it seems we definitely need #7641 in here to make the tests pass.

[AndrolGenhald]

I'll find some time this week to go through that, I might end up splitting it up into separate PRs for each stub so more important ones can be merged sooner.

[discordier]

I'll find some time this week to go through that, I might end up splitting it up into separate PRs for each stub so more important ones can be merged sooner.

No need to rush this, I'll be on vacation until June 13th from tomorrow on.
Feel free to adjust/merge/postpone this PR though.

[discordier]

So, I am back from vacation.
How may I further assist on this one?
Shall we incorporate #7641 into this one or rather split that one up?

[AndrolGenhald]

I've been a bit busy lately, give me a day or two and I'll pull that out into separate PRs that we can review and merge quickly (I hope).

[AndrolGenhald]

I haven't forgotten about this! I wasn't super clean about keeping the commits on that PR fully separate for the different stubs, so pulling it apart isn't as easy as I expected, I should be able to get the DOM one in a separate PR today or tomorrow though.

[AndrolGenhald]

Opened #8140.

[discordier]

Shall I rebase on #8140 or do we want to merge that one?

[AndrolGenhald]

@orklah I didn't notice this was actually targeting 4.x, do you think I should backport #8140 to 4.x or should this be changed to target master?

[orklah]

It would probably be easier to target this one on master I think

[discordier]

I have updated the target branch and rebased onto current master.

[discordier]

Do I have to rebase against current master again?
What else should I cram into this PR?

@orklah is this ready from your POV?

[orklah]

There is still an error in CI that should be adressed before I can merge this

[discordier]

Ok, I fixed the shepherd run, there was an issue in code I did not touch.
I suppose this means the updated property map is used and works correctly. :)

I have no idea how to fix the "label" run - I suppose this needs to re run to spit out any information as you just added the label.

[orklah]

Thanks!