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

Incompatible types found for T #7549

Open
come-nc opened this issue Feb 1, 2022 · 8 comments
Open

Incompatible types found for T #7549

come-nc opened this issue Feb 1, 2022 · 8 comments

Comments

@come-nc
Copy link
Contributor

come-nc commented Feb 1, 2022

Hello,

I am trying to fix an error we have in the Nextcloud codebase.

I reproduced the situation on psalm.dev:
https://psalm.dev/r/ab70e17540

I want the line 80 to be an error, because LoginFailed is not a child class of ARemoteWipeEvent.
But I want the line 79 to not be an error, because RemoteWipeStarted is a child class of ARemoteWipeEvent.

Psalm says: Incompatible types found for T (RemoteWipeStarted is not in ARemoteWipeEvent)
But RemoteWipeStarted extends ARemoteWipeEvent.

I tried to fix this by using template-covariant instead of template on IEventListener, but if I do so:

  • I have to remove the @param T $event annotation from the method
  • line 80 is no longer an error!

Any help or suggestion appreciated!

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ab70e17540
<?php

class Event {
}

class LoginFailed extends Event {
	/** @var string */
	private $loginName;

	public function __construct(string $loginName) {
		$this->loginName = $loginName;
	}

	public function getLoginName(): string {
		return $this->loginName;
	}
}

class RemoteWipeStarted extends ARemoteWipeEvent {
}

abstract class ARemoteWipeEvent extends Event {
}

/**
 * @template-implements IEventListener<ARemoteWipeEvent>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof ARemoteWipeEvent)) {
			return;
		}
	}
}

/**
 * @template-implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @since 17.0.0
 *
 * @template T of Event
 */
interface IEventListener {
	/**
	 * @param T $event
	 *
	 * @since 17.0.0
	 */
	public function handle(Event $event): void;
}

/**
	 * @template T of Event
	 * @param string $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @psalm-param string|class-string<T> $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @param string $className fully qualified class name (or ::class notation) of a IEventListener that can be built by the DI container
	 * @psalm-param class-string<IEventListener<T>> $className fully qualified class name that can be built by the DI container
	 * @param int $priority The higher this value, the earlier an event
	 *                      listener will be triggered in the chain (defaults to 0)
	 *
	 * @since 17.0.0
	 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class);
addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class);
Psalm output (using commit 2e01e9b):

ERROR: InvalidArgument - 79:1 - Incompatible types found for T (RemoteWipeStarted is not in ARemoteWipeEvent)

ERROR: InvalidArgument - 80:1 - Incompatible types found for T (LoginFailed is not in ARemoteWipeEvent)

@orklah
Copy link
Collaborator

orklah commented Feb 1, 2022

Hi, Psalm prevents you from pushing childs to a template that is only expecting parents. This is indeed because of the issue caused by covariance: https://psalm.dev/docs/annotating_code/templated_annotations/#template-covariance

When you add the covariant, psalm find a new error, because you receive a T as a param of handle. This could cause new issues if you use T to alter the collection/template.

However, you can tell Psalm that you won't change the collection by adding immutable annotations like this: https://psalm.dev/r/47078f5aab

With those, Psalm doesn't complain anymore

@orklah orklah closed this as completed Feb 1, 2022
@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/47078f5aab
<?php

class Event {
}

class LoginFailed extends Event {
	/** @var string */
	private $loginName;

	public function __construct(string $loginName) {
		$this->loginName = $loginName;
	}

	public function getLoginName(): string {
		return $this->loginName;
	}
}

class RemoteWipeStarted extends ARemoteWipeEvent {
}

abstract class ARemoteWipeEvent extends Event {
}

/**
 * @template-implements IEventListener<ARemoteWipeEvent>
 * @psalm-immutable
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof ARemoteWipeEvent)) {
			return;
		}
	}
}

/**
 * @template-implements IEventListener<LoginFailed>
 * @psalm-immutable
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @since 17.0.0
 *
 * @template-covariant T of Event
 * @psalm-immutable
 */
interface IEventListener {
	/**
	 * @param T $event
	 *
	 * @since 17.0.0
	 */
	public function handle(Event $event): void;
}

/**
	 * @template T of Event
	 * @param string $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @psalm-param string|class-string<T> $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @param string $className fully qualified class name (or ::class notation) of a IEventListener that can be built by the DI container
	 * @psalm-param class-string<IEventListener<T>> $className fully qualified class name that can be built by the DI container
	 * @param int $priority The higher this value, the earlier an event
	 *                      listener will be triggered in the chain (defaults to 0)
	 *
	 * @since 17.0.0
	 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class);
addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class);
Psalm output (using commit 2e01e9b):

No issues!

@come-nc
Copy link
Contributor Author

come-nc commented Feb 1, 2022

Hi, Psalm prevents you from pushing childs to a template that is only expecting parents. This is indeed because of the issue caused by covariance: https://psalm.dev/docs/annotating_code/templated_annotations/#template-covariance

When you add the covariant, psalm find a new error, because you receive a T as a param of handle. This could cause new issues if you use T to alter the collection/template.

However, you can tell Psalm that you won't change the collection by adding immutable annotations like this: https://psalm.dev/r/47078f5aab

With those, Psalm doesn't complain anymore

But in this case psalm does not complain about the call on line 80 which is incorrect (LoginFailed is not a subclass of ARemoteWipeEvent).
Is there any way I can tell psalm that I want $className to be a class that implements IEventListener<T> with $eventName being T or a subclass of T?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/47078f5aab
<?php

class Event {
}

class LoginFailed extends Event {
	/** @var string */
	private $loginName;

	public function __construct(string $loginName) {
		$this->loginName = $loginName;
	}

	public function getLoginName(): string {
		return $this->loginName;
	}
}

class RemoteWipeStarted extends ARemoteWipeEvent {
}

abstract class ARemoteWipeEvent extends Event {
}

/**
 * @template-implements IEventListener<ARemoteWipeEvent>
 * @psalm-immutable
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof ARemoteWipeEvent)) {
			return;
		}
	}
}

/**
 * @template-implements IEventListener<LoginFailed>
 * @psalm-immutable
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @since 17.0.0
 *
 * @template-covariant T of Event
 * @psalm-immutable
 */
interface IEventListener {
	/**
	 * @param T $event
	 *
	 * @since 17.0.0
	 */
	public function handle(Event $event): void;
}

/**
	 * @template T of Event
	 * @param string $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @psalm-param string|class-string<T> $eventName preferably the fully-qualified class name of the Event sub class to listen for
	 * @param string $className fully qualified class name (or ::class notation) of a IEventListener that can be built by the DI container
	 * @psalm-param class-string<IEventListener<T>> $className fully qualified class name that can be built by the DI container
	 * @param int $priority The higher this value, the earlier an event
	 *                      listener will be triggered in the chain (defaults to 0)
	 *
	 * @since 17.0.0
	 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class);
addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class);
Psalm output (using commit 2e01e9b):

No issues!

@AndrolGenhald
Copy link
Collaborator

I thought this might work, but not quite. Second try was also a failure, but might work if #7553 is fixed.

This works if you're willing to add the extra function call. Or you could force it, but that's dangerous.

@orklah I think this should probably be reopened as an enhancement or bugfix to fix handling of templates as template constraints. I would expect something like this to work. For addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class):

  • TEvent should resolve to ARemoteWipeEvent
  • TListener should resolve to RemoteWipeActivityListener
  • TCanBeChildEvent should resolve to RemoteWipeStarted.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/e9779b57e9
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @template T of ARemoteWipeEvent
 * @implements IEventListener<T>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Should be redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TListener of IEventListener<TEvent>
 * @param class-string<TEvent> $eventName
 * @param class-string<TListener> $className
 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(ARemoteWipeEvent::class, RemoteWipeActivityListener::class); // Incorrect
addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class); // Incorrect
addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class); // Correctly invalid
Psalm output (using commit 2e01e9b):

ERROR: InvalidArgument - 53:1 - Incompatible types found for TEvent (ARemoteWipeEvent is not in T:RemoteWipeActivityListener as ARemoteWipeEvent)

ERROR: InvalidArgument - 54:1 - Incompatible types found for TEvent (RemoteWipeStarted is not in T:RemoteWipeActivityListener as ARemoteWipeEvent)

ERROR: InvalidArgument - 55:1 - Incompatible types found for TEvent (LoginFailed is not in T:RemoteWipeActivityListener as ARemoteWipeEvent)
https://psalm.dev/r/c88688bf94
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @template T of ARemoteWipeEvent
 * @implements IEventListener<T>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Correctly redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
	/**
	 * @param T $event
	 */
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TListener of IEventListener<TEvent>
 */
class ServiceListenerManager
{
    /** @param class-string<TListener> $listenerClass */
    public function __construct(
        private string $listenerClass,
    ) {}
    
    /**
     * @template TChildEvent of TEvent
 	 * @param class-string<TChildEvent> $eventName
	 */
	public function addServiceListener(string $eventName, string $className, int $priority = 0): void
	{
    	$a = [$eventName,$className,$priority];
    	echo count($a);
	}
}

$lfslm = new ServiceListenerManager(LoginFailedListener::class);
$lfslm->addServiceListener(LoginFailed::class, LoginFailedListener::class);
$rwslm = new ServiceListenerManager(RemoteWipeActivityListener::class);
$rwslm->addServiceListener(ARemoteWipeEvent::class, RemoteWipeActivityListener::class);
$rwslm->addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class);
/** @psalm-trace $rwslm */;
// $rwslm is ServiceListenerManager<T:RemoteWipeActivityListener as ARemoteWipeEvent, RemoteWipeActivityListener>
// Why is TChildEvent allowed to be LoginFailed?
$rwslm->addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class); // Should be invalid
Psalm output (using commit 2e01e9b):

INFO: Trace - 70:27 - $rwslm: ServiceListenerManager<T:RemoteWipeActivityListener as ARemoteWipeEvent, RemoteWipeActivityListener>

ERROR: RedundantConditionGivenDocblockType - 18:9 - Docblock-defined type T:RemoteWipeActivityListener as ARemoteWipeEvent for $event is always ARemoteWipeEvent
https://psalm.dev/r/b982ff8efd
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @implements IEventListener<ARemoteWipeEvent>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Should be redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
    /** @param T $event */
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TListener of IEventListener<TEvent>
 * @param class-string<TEvent> $eventName
 * @param class-string<TListener> $className
 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(widenRemoteWipeEvent(ARemoteWipeEvent::class), RemoteWipeActivityListener::class); // Incorrect
addServiceListener(widenRemoteWipeEvent(RemoteWipeStarted::class), RemoteWipeActivityListener::class); // Incorrect
addServiceListener(widenRemoteWipeEvent(LoginFailed::class), RemoteWipeActivityListener::class); // Correctly invalid

/**
 * @param class-string<ARemoteWipeEvent> $arg
 * @return class-string<ARemoteWipeEvent>
 */
function widenRemoteWipeEvent(string $arg): string
{
    return $arg;
}
Psalm output (using commit 2e01e9b):

ERROR: InvalidArgument - 55:41 - Argument 1 of widenRemoteWipeEvent expects class-string<ARemoteWipeEvent>, LoginFailed::class provided

ERROR: RedundantConditionGivenDocblockType - 17:9 - Docblock-defined type ARemoteWipeEvent for $event is always ARemoteWipeEvent
https://psalm.dev/r/ce8cf4e36d
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @implements IEventListener<ARemoteWipeEvent>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Correctly redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
    /**
     * @template TChildEvent of T
     * @param T $event
     */
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TListener of IEventListener<TEvent>
 * @param class-string<TEvent> $eventName
 * @param class-string<TListener> $className
 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

/** @var class-string<ARemoteWipeEvent> */
$eventName = RemoteWipeStarted::class;
addServiceListener($eventName, RemoteWipeActivityListener::class);
Psalm output (using commit 2e01e9b):

ERROR: RedundantConditionGivenDocblockType - 17:9 - Docblock-defined type ARemoteWipeEvent for $event is always ARemoteWipeEvent
https://psalm.dev/r/25a36f3820
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @implements IEventListener<ARemoteWipeEvent>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Correctly redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
    /**
     * @template TChildEvent of T
     * @param T $event
     */
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TListener of IEventListener<TEvent>
 * @param class-string<TEvent> $eventName
 * @param class-string<TListener> $className
 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

/** @var class-string<ARemoteWipeEvent> */
$eventName = LoginFailed::class;
addServiceListener($eventName, RemoteWipeActivityListener::class);
Psalm output (using commit 2e01e9b):

ERROR: RedundantConditionGivenDocblockType - 17:9 - Docblock-defined type ARemoteWipeEvent for $event is always ARemoteWipeEvent
https://psalm.dev/r/6f8179ebe7
<?php

class Event {
}

class LoginFailed extends Event {}

class RemoteWipeStarted extends ARemoteWipeEvent {}

abstract class ARemoteWipeEvent extends Event {}

/**
 * @implements IEventListener<ARemoteWipeEvent>
 */
class RemoteWipeActivityListener implements IEventListener {
	public function handle(Event $event): void {
        assert($event instanceof ARemoteWipeEvent); // Should be redundant
	}
}

/**
 * @implements IEventListener<LoginFailed>
 */
class LoginFailedListener implements IEventListener {
	public function handle(Event $event): void {
		if (!($event instanceof LoginFailed)) {
			return;
		}
	}
}

/**
 * @template T of Event
 */
interface IEventListener {
    /**
     * @template TCanBeChildEvent of T
     * @param TCanBeChildEvent $event
     */
	public function handle(Event $event): void;
}

/**
 * @template TEvent of Event
 * @template TCanBeChildEvent of TEvent
 * @template TListener of IEventListener<TEvent>
 * @param class-string<TCanBeChildEvent> $eventName
 * @param class-string<TListener> $className
 */
function addServiceListener(string $eventName, string $className, int $priority = 0): void
{
    $a = [$eventName,$className,$priority];
    echo count($a);
}

addServiceListener(LoginFailed::class, LoginFailedListener::class);
addServiceListener(ARemoteWipeEvent::class, RemoteWipeActivityListener::class);
addServiceListener(RemoteWipeStarted::class, RemoteWipeActivityListener::class);
addServiceListener(LoginFailed::class, RemoteWipeActivityListener::class); // Invalid
Psalm output (using commit 2e01e9b):

ERROR: InvalidArgument - 58:1 - Incompatible types found for TEvent (RemoteWipeStarted is not in ARemoteWipeEvent)

ERROR: InvalidArgument - 59:1 - Incompatible types found for TEvent (LoginFailed is not in ARemoteWipeEvent)

@come-nc
Copy link
Contributor Author

come-nc commented Sep 12, 2023

I think this can be closed, the snippet I posted in the first post now behaves as expected on psalm.dev (you can compare with the result from when I opened this)

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

No branches or pull requests

3 participants