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

Dropping @psalm-self-out to avoid incorrect type inference in inheritance #78

Merged

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Dec 3, 2022

Before this change, in following example, @psalm-self-out would delete inheritance type information at runtime.
For example:

class Foo implements ParentType {
    // ...
}

$foo = new Foo();

$foo->methodWithSelfOut();

/** @psalm-trace $foo */ // produces `ParentType`

This change corrects that, preserving static in the type information.

Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Before this change, in following example, `@psalm-self-out` would delete inheritance
type information at runtime.
For example:

```php
class Foo implements ParentType {
    // ...
}

$foo = new Foo();

$foo->methodWithSelfOut();

/** @psalm-trace $foo */ // produces `ParentType`
```

This change corrects that, preserving `static` in the type information.
@Ocramius Ocramius added the Bug Something isn't working label Dec 3, 2022
@Ocramius Ocramius added this to the 3.16.1 milestone Dec 3, 2022
@Ocramius
Copy link
Member Author

Ocramius commented Dec 3, 2022

Note: I noticed the problem while working on laminas-session, which (ab-)uses a lot of the ArrayObject stuff that keeps haunting us everywhere.

… type inference

Cases like following break completely with `@psalm-self-out`:

```php
$o = new ArrayObject(['foo' => 'bar']);

$o->baz = 'taz'; // `never` due to `'foo'&'bar'` not being possible
```

Rolling back the entire `psalm-self-out` improvement: too aggressive, and
not practically useful on these mutable data containers.
@Ocramius Ocramius changed the title Corrected @psalm-self-out references to preserve child class types too Dropping @psalm-self-out to avoid incorrect type inference in inheritance Dec 3, 2022
Ocramius added a commit to laminas/laminas-session that referenced this pull request Dec 3, 2022
…block overrides causing type clashes

Note: we assume laminas/laminas-stdlib#78 will be merged first.
@internalsystemerror
Copy link
Member

Is it not possible to do @psalm-self-out static<...> to get around this issue?

Copy link
Member

@internalsystemerror internalsystemerror left a comment

Choose a reason for hiding this comment

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

LGTM if we have to. Strange how somehow removing some of the type data which reduced the baseline last time also reduces the baseline here 😆

@Ocramius
Copy link
Member Author

Ocramius commented Dec 3, 2022

The &static approach is something I had already tried, but caused even more issues.

I will keep using psalm-self-out outside the context of inheritance, in builders and similar abstractions, but it is not a.good fit here 😁

@Ocramius Ocramius self-assigned this Dec 3, 2022
@Ocramius Ocramius merged commit f4f7736 into laminas:3.16.x Dec 3, 2022
@Ocramius Ocramius deleted the fix/correct-psalm-self-out-references branch December 3, 2022 18:48
Ocramius added a commit to laminas/laminas-navigation that referenced this pull request Dec 3, 2022
Ocramius added a commit to laminas/laminas-session that referenced this pull request Dec 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants