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

Add SplObjectStorage stub #301

Merged
merged 3 commits into from Aug 22, 2020

Conversation

VincentLanglet
Copy link
Contributor

I'm trying to close phpstan/phpstan#3111

This will be a BC-break I think, because before we could write
SplObjectStorage<object> and now it will be SplObjectStorage<object, mixed>.

Maybe I'm wrong and I can avoid this BC-break @ondrejmirtes ?

@ondrejmirtes
Copy link
Member

Hi, you don't "need" this stub for phpstan/phpstan-symfony#92, you can go on and use just SplObjectStorage<MutableAclInterface> in there. You're right, this would be a BC break, better to target 1.0 with this.

@ondrejmirtes
Copy link
Member

And fix the stub by looking at the build log :)

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Aug 17, 2020

Hi, you don't "need" this stub for phpstan/phpstan-symfony#92, you can go on and use just SplObjectStorage<MutableAclInterface> in there.

Currently it's only possible to set the keys, not the values.
So I could write SplObjectStorage<ObjectIdentityInterface> indeed.

You're right, this would be a BC break, better to target 1.0 with this.

When using array, we can either write array<Value> or array<Key, Value>.

A BC way would be to allow SplObjectStorage<Key> and SplObjectStorage<Key, Value>.
But I don't know how to write this. And I'm not sure it's a good idea, since it's not consistent with array.

Why target 1.0 ? Breaking changes are allowed in a 0.x version. It could be 0.13.

And fix the stub by looking at the build log :)

Sure, but the errors are not really clear to me.
If I understand correctly I should keep the functionMap definition.

@ondrejmirtes
Copy link
Member

The next major is going to be 1.0, not 0.13 :)

It's a BC break because on level 6 there's going to be a new error message because SplObjectStorage becomes generic.

@VincentLanglet
Copy link
Contributor Author

The next major is going to be 1.0, not 0.13 :)

It's a BC break because on level 6 there's going to be a new error message because SplObjectStorage becomes generic.

Ok !

Any idea about the build failure ?

@ondrejmirtes
Copy link
Member

interface does not implement other interfaces, it extends them. You can also run vendor/bin/phing phpstan lcoally for faster feedback.

@@ -105,3 +105,66 @@ class SplFixedArray implements Iterator, ArrayAccess, Countable
*/
public function toArray(): array { }
}

/**
* @template TKey of object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You understand how SplObjectStorage works wrong. The user cannot influence the iterable key, it's always int: https://3v4l.org/J4M0N

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but the ArrayAccess Key is an objet: https://3v4l.org/DsUbK

So it would be

@template-implements Iterator<int, TKey>
@template-implements ArrayAccess<TKey, TValue>

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but it'd make more sense to rename them to TObject and TData.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ! I have only 3 build failures. It's for php 8 so I assume it's normal.

@ondrejmirtes
Copy link
Member

Sure, thanks. I'll make a note to merge this once 1.0-dev is opened.

@VincentLanglet
Copy link
Contributor Author

Sure, thanks. I'll make a note to merge this once 1.0-dev is opened.

I'm curious, any release-goal for the 1.0 version ?

@ondrejmirtes
Copy link
Member

Do you mean a date? Winter 2020-21. So in around 6 months.

@VincentLanglet
Copy link
Contributor Author

It's a BC break because on level 6 there's going to be a new error message because SplObjectStorage becomes generic.

I thought about this.
I find kinda sad that every new generic class will be a BC break.

The following class could be generics: SplPriorityQueue, SplMinHeap, SplMaxHeap, SplHeap, SplQueue, SplStack, SplDoublyLinkedList. I'm sure there is (lot of) others.
Either you'll need to often release major, either people will need to wait for this kind of feature.

What is the name of the rule which will fail ?
Maybe a exclude param could be useful.

When you introduce a generic, you'll had to the exclude list

genericRule: true
genericRuleExcluded:
    - SplObjectStorage

And then, in the next major, you remove it from the exclude list.

WDYT ?

@ondrejmirtes
Copy link
Member

I've thought about this - we could add these new stubs in bleedingEdge.neon and that would be fine by me.

@@ -0,0 +1,62 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't work without the opening <?php tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. But some tests are still failing for memory reason 😕

@VincentLanglet VincentLanglet force-pushed the SplObjectStorage branch 2 times, most recently from 9d8e041 to 5ab4973 Compare August 22, 2020 13:44
@ondrejmirtes ondrejmirtes merged commit 0632865 into phpstan:master Aug 22, 2020
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants