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

General type inference improvements #98

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

gsteel
Copy link
Member

@gsteel gsteel commented Sep 5, 2023

Q A
Documentation yes
QA yes

Description

  • Adds template parameters to ParameterObjectInterface and AbstractOptions
  • Fixes a range of psalm issues, mostly in tests
  • Updates the baseline
  • Bumps dev dependencies and refreshes the lock file

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.

- Adds template parameters to ParameterObjectInterface and AbstractOptions
- Fixes a range of psalm issues, mostly in tests
- Updates the baseline
- Bumps dev dependencies and refreshes the lock file

Signed-off-by: George Steel <[email protected]>
@gsteel gsteel added this to the 3.18.0 milestone Sep 5, 2023
@gsteel gsteel mentioned this pull request Sep 5, 2023
src/AbstractOptions.php Show resolved Hide resolved
@Ocramius Ocramius merged commit ec071d3 into laminas:3.18.x Sep 5, 2023
10 checks passed
@Ocramius Ocramius self-assigned this Sep 5, 2023
@Ocramius
Copy link
Member

Ocramius commented Sep 5, 2023

Thanks @gsteel!

@gsteel gsteel deleted the type-inference-improvements branch September 5, 2023 12:12
@boesing
Copy link
Member

boesing commented Nov 8, 2023

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
Copy link
Member Author

gsteel commented Nov 8, 2023

@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
Copy link
Member

boesing commented Nov 9, 2023

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
Copy link
Member Author

gsteel commented Nov 10, 2023

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
Copy link
Member

boesing commented Nov 10, 2023

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.

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

Successfully merging this pull request may close these issues.

3 participants