update property map #8000

discordier · 2022-05-23T16:50:18Z

This is an attempt at fixing #7983 by:

adding a (at least more) correct doc parsing approach than in phan/phan.
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 · 2022-05-23T16:56:27Z

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

discordier · 2022-05-23T17:05:51Z

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

orklah · 2022-05-23T17:49:10Z

dictionaries/PropertyMap.php

-        'actualEncoding' => 'string',
-        'encoding' => 'string',
-        'version' => 'string',
+    'CommonMark\\Node' => [


I think the offset should be a lowercase string

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.

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)

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.

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

orklah · 2022-05-23T17:53:11Z

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 · 2022-05-24T13:36:29Z

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 · 2022-05-24T16:34:46Z

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 · 2022-05-24T16:45:19Z

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 · 2022-05-24T16:48:07Z

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 · 2022-05-25T09:51:21Z

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 · 2022-05-26T03:42:50Z

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 · 2022-05-30T08:32:13Z

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 · 2022-05-30T20:20:19Z

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 · 2022-05-31T11:34:54Z

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 · 2022-05-31T15:54:02Z

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 · 2022-05-31T16:32:53Z

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 · 2022-06-15T08:29:48Z

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 · 2022-06-15T21:19:26Z

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 · 2022-06-20T16:58:53Z

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 · 2022-06-22T06:59:36Z

Opened #8140.

discordier · 2022-06-22T15:57:02Z

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

AndrolGenhald · 2022-06-23T00:40:34Z

@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 · 2022-06-23T17:19:27Z

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

discordier · 2022-06-29T07:16:01Z

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

We need a defined, reproducible starting point.

See vimeo#7983

The class names in property map are now always lowercase

This should fix the now failing tests as the properties got removed from the autogenerated property map.

We now have the extensions in a sub directory and therefore have to skip that one.

discordier · 2022-07-04T10:26:05Z

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 · 2022-07-04T19:29:12Z

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

discordier · 2022-07-06T07:14:53Z

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 · 2022-07-06T18:44:35Z

Thanks!

discordier mentioned this pull request May 23, 2022

Extending ArrayObject with private properties produces false positives #7983

Closed

discordier force-pushed the hotfix/update-property-map branch from 90c7c63 to 877c9a2 Compare May 23, 2022 16:53

orklah reviewed May 23, 2022

View reviewed changes

discordier force-pushed the hotfix/update-property-map branch from 877c9a2 to ef90170 Compare May 24, 2022 16:29

AndrolGenhald mentioned this pull request Jun 14, 2022

tests: add test that validates the callmap in the current runtime #8104

Merged

discordier changed the base branch from 4.x to master June 29, 2022 07:13

discordier force-pushed the hotfix/update-property-map branch from 46d690b to aebe3c4 Compare June 29, 2022 07:14

Sort PropertyMap by keys

c65e6e8

We need a defined, reproducible starting point.

discordier added 4 commits June 29, 2022 09:23

Add update script and update property map

5fd2090

See vimeo#7983

Remove now obsolete loop

6305c21

The class names in property map are now always lowercase

Do not convert the file list to array

9cb9b1e

Skip classes in stub files when generating property map

631936c

discordier force-pushed the hotfix/update-property-map branch from aebe3c4 to 631936c Compare June 29, 2022 07:23

discordier added 3 commits July 4, 2022 11:14

Add missing properties to stubs/Reflection.phpstub

ce4ba69

This should fix the now failing tests as the properties got removed from the autogenerated property map.

Skip sub directories when parsing stubs

a8f2b2b

We now have the extensions in a sub directory and therefore have to skip that one.

Regenerate property map file

8e05adb

orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jul 4, 2022

discordier added 2 commits July 6, 2022 08:59

Fix variable annotation

ea9903a

Remove now obsolete casts

8ffab5e

orklah merged commit 6d45003 into vimeo:master Jul 6, 2022

discordier deleted the hotfix/update-property-map branch July 7, 2022 09:59

orklah mentioned this pull request Feb 6, 2023

tidyNode properties’ types are inaccurate #2855

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update property map #8000

update property map #8000

discordier commented May 23, 2022

discordier commented May 23, 2022

discordier commented May 23, 2022 •

edited

orklah May 23, 2022

discordier May 24, 2022

orklah May 24, 2022 •

edited

discordier May 24, 2022

orklah May 24, 2022

orklah commented May 23, 2022

AndrolGenhald commented May 24, 2022

discordier commented May 24, 2022

orklah commented May 24, 2022

AndrolGenhald commented May 24, 2022 •

edited

discordier commented May 25, 2022

AndrolGenhald commented May 26, 2022

discordier commented May 30, 2022

orklah commented May 30, 2022

discordier commented May 31, 2022

AndrolGenhald commented May 31, 2022

discordier commented May 31, 2022

discordier commented Jun 15, 2022

AndrolGenhald commented Jun 15, 2022

AndrolGenhald commented Jun 20, 2022

AndrolGenhald commented Jun 22, 2022

discordier commented Jun 22, 2022

AndrolGenhald commented Jun 23, 2022

orklah commented Jun 23, 2022

discordier commented Jun 29, 2022

discordier commented Jul 4, 2022

orklah commented Jul 4, 2022

discordier commented Jul 6, 2022

orklah commented Jul 6, 2022

update property map #8000

update property map #8000

Conversation

discordier commented May 23, 2022

discordier commented May 23, 2022

discordier commented May 23, 2022 • edited

orklah May 23, 2022

Choose a reason for hiding this comment

discordier May 24, 2022

Choose a reason for hiding this comment

orklah May 24, 2022 • edited

Choose a reason for hiding this comment

discordier May 24, 2022

Choose a reason for hiding this comment

orklah May 24, 2022

Choose a reason for hiding this comment

orklah commented May 23, 2022

AndrolGenhald commented May 24, 2022

discordier commented May 24, 2022

orklah commented May 24, 2022

AndrolGenhald commented May 24, 2022 • edited

discordier commented May 25, 2022

AndrolGenhald commented May 26, 2022

discordier commented May 30, 2022

orklah commented May 30, 2022

discordier commented May 31, 2022

AndrolGenhald commented May 31, 2022

discordier commented May 31, 2022

discordier commented Jun 15, 2022

AndrolGenhald commented Jun 15, 2022

AndrolGenhald commented Jun 20, 2022

AndrolGenhald commented Jun 22, 2022

discordier commented Jun 22, 2022

AndrolGenhald commented Jun 23, 2022

orklah commented Jun 23, 2022

discordier commented Jun 29, 2022

discordier commented Jul 4, 2022

orklah commented Jul 4, 2022

discordier commented Jul 6, 2022

orklah commented Jul 6, 2022

discordier commented May 23, 2022 •

edited

orklah May 24, 2022 •

edited

AndrolGenhald commented May 24, 2022 •

edited