laminas / laminas-stdlib

SPL extensions, array utilities, error handlers, and more
https://docs.laminas.dev/laminas-stdlib/
BSD 3-Clause "New" or "Revised" License
190 stars 40 forks source link

General type inference improvements #98

Closed gsteel closed 1 year ago

gsteel commented 1 year ago
Q A
Documentation yes
QA yes

Description

Closes #97

Primarily, this patch is just to get renovate green again because psalm has made everything go red with various improvements.

Stdlib is also good to go for PHP 8.3, so I'll send in another patch for this.

Ocramius commented 1 year ago

Thanks @gsteel!

boesing commented 11 months ago

I like that the generic is just TValue so that I can provide an array shape rather than an (imho) useless int|bool|string. So what I usually do is:

/** 
  * @psalm-type WhateverOptionsArrayShape = array{key1: bool, key2: int} 
  * @template-extends AbstractOptions<WhateverOptionsArrayShape>
  */
final class WhateverOptions extends AbstractOptions
{
     public function setKey1(bool $value): void
     {}
     public function getKey1(): bool
     {}
     // etc
}

Sadly, that does not work, since it is not possible. IMHO, it would be decent to be able to actually annotate possible keys. Maybe we can consider this for v4, generic would look like:

/**
 * @template TValue of iterable
 * @implements ParameterObjectInterface<TValue>
 */
abstract class AbstractOptions
{
   /**
     * @param TValue|AbstractOptions<TValue>|null $options
     */
    public function __construct($options = null)
    {}
}

Just an idea, I've added these types via stubs for now in our projects since that suits for us better but will likely diverge with other component implementations which would be sad.

gsteel commented 11 months ago

@boesing

Using a shape as a template would only work if the implementation used an array internally for storage right? i.e. AbstractOptions itself looked like

/**
 * @template TShape = array{foo: string}
 */
abstract class AbstractOptions {
  /** @var TShape */
  protected array $storage;

  /** @param TShape|self<TShape> $options */
  public function __construct(iterable $options) {
    $options = ArrayUtils::iterableToArray($options);
    $this->storage = $options;
  }

  public function setFoo(string $foo): void
  {
    $this->storage['foo'] = $foo;
  }

  public function getFoo(): string
  {
    return $this->storage['foo'];
  }
}

Or am I missing something? It was a while ago I did this, but IIRC, using a shape wasn't possible because psalm can't translate the keys to properties declared in the concrete class (??)

boesing commented 11 months ago

Actually not. In this case, array keys to be passed have to reflect the property structure. Especially when __strictMode__ is enabled, the __construct will throw an exception if properties are not existent as a property.

What you could do with psalm is something like:

/**
 * @psalm-type MyFancyOptionsType = array<property-of<MyFancyOptions>,mixed>
 */

But obviously, that won't be really helpful. But it would be helpful if we have something like:

/**
 * @psalm-type MyFancyOptionsType = array{property1:int,property2:bool}
 */

As this will tell psalm exactly what the array must contain. Thats why I would not accept iterable in the next major. Either accept an array shape with the structure of the generic or allow an instance of the same object (even tho I still do not understand why we even need this kind of feature - i.e. passing the same object as a constructor... thats why I would drop that as well).

I'd only allow an array shape which allows upstream to explicitly declare what stuff is required and what is optional (i.e. by adding property2?:bool since property2 has a default value of false).

IMHO that would be a huge benefit for this class.

gsteel commented 11 months ago

Thanks for explaining @boesing - yes, accepting only an array shape to __construct would be a big improvement.

I'm still not quite sure I'm getting everything completely, because having had a look at various patches in psalm, we could actually use @param properties-of<MyFancyOptions>|other-stuff in __construct right now, so theoretically, we could annotate AbstractOptions::__construct with @param properties-of<self>|other-stuff to improve inference for consumers that call new FancyOptions(array), assuming self|static works with properties-of.

At some point, I'll look into this in more detail to improve my understanding of it.

Cheers

boesing commented 11 months ago

But it is not just property-of since it has to be a map of values which are then passed to the properties declared in the implementing class.

Therefore, that example you state wont fit the requirement.