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

Support for generics in interface inference #270

Open
arnaud-lb opened this issue Nov 28, 2022 · 2 comments
Open

Support for generics in interface inference #270

arnaud-lb opened this issue Nov 28, 2022 · 2 comments

Comments

@arnaud-lb
Copy link

arnaud-lb commented Nov 28, 2022

Thank you for the awesome work on this library. Valinor is super useful and makes it much easier to deal with external data in a strongly typed project.

I am facing an issue since 0.11 added the following restriction:

It is now mandatory to list all possible class-types that can be inferred by the mapper. This change is a step towards the library being able to deliver powerful new features such as compiling a mapper for better performance.

Currently, this change makes interface inference incompatible with generic classes, as Valinor will not accept a return type like class-string<Foo<A>|Foo<B>> (rightfully, as this is not a valid type) or 'Foo<A>'|'Foo<B>'.

To give some context, I'm trying to map a discriminated union, but the discriminator is on the parent node.

The source looks like this:

{
    "type": "string",
    "node": "..."
}

And the model looks like this:

/** @template T */
interface RootInterface {}

class Root implements RootInterface {
    public function __construct(
        /** @var T */
        NodeInterface $node,
    ){}
}

interface NodeInterface {}
class NodeA implements NodeInterface {}
class NodeB implements NodeInterface {}

Due to the position of the discriminator, I can not directly infer the type of the nested node: I must infer the type of the root node.

It seems that supporting a return type that consists of a union of strings in this case would fix the issue, as I could type the mapper like this:

/** @return 'Root<NodeA>'|'Root<NodeB>' */
fn (string $type) => ...

Here is a full example of my use-case: https://gist.github.com/arnaud-lb/535d89874ed719265f66bc19a88b5fb0

@romm
Copy link
Member

romm commented Nov 30, 2022

Hi @arnaud-lb, thank you for reporting this issue; I did not have this usecase in mind when pushing 1b0ff39 indeed.

From my POV, the following should make sense, although it is not supported by the library I certainly could make this work:

/** @return class-string<Root<NodeA>|Root<NodeB>> */
function inferRootType(string $type): string {
    return match ($type) {
        'NodeA' => 'Root<NodeA>',
        'NodeB' => 'Root<NodeB>',
        default => throw new \Exception(),
    };
}

But static analysis tools wont be happy about that, see PHPStan example and Psalm example; this probably makes sense as Root<NodeA> is not considered a class-string, but Root is.

Do you see any other way than using string values like @return 'Root<NodeA>'|'Root<NodeB>' — even if that would mean contributing to PHPStan/Psalm?

BTW big fan of your work on PHPStan generics. 🙂

@arnaud-lb
Copy link
Author

arnaud-lb commented Jan 2, 2023

Thank you :D

I wish you and this project a happy new year :)

But static analysis tools wont be happy about that, see PHPStan example and Psalm example; this probably makes sense as Root<NodeA> is not considered a class-string, but Root is.

Agreed. I think that the class-string type would lose usefulness if class-string<Root<NodeA>> was valid, as this does not represent a class name string that PHP constructs can understand.

Do you see any other way than using string values like @return 'Root<NodeA>'|'Root<NodeB>' — even if that would mean contributing to PHPStan/Psalm?

Introducing a new generic-class-string<T> type would solve this issue, but it would have limited usefulness.

Adding support for class-string<Root<NodeA>> would also work, if PHPStan/Psalm treated generic class-strings and non-generic ones differently (e.g. generic class-string is not valid in new), and if class-string<Root<NodeA>> was not considered a sub type of class-string. This would make class-string more confusing.

Using string values like @return 'Root<NodeA>'|'Root<NodeB>' would work for me, though.

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

No branches or pull requests

2 participants