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

AST: generic operator support #589

Open
hemberger opened this issue Mar 25, 2023 · 2 comments
Open

AST: generic operator support #589

hemberger opened this issue Mar 25, 2023 · 2 comments
Milestone

Comments

@hemberger
Copy link
Contributor

I've been looking into handling the case where QueryScope::getType receives an instance of SqlFtw\Sql\Expression\Operator as input. Currently, this falls back to simply returning MixedType, which renders the AST-derived types mostly useless. It would be much more powerful if we could get the true type by applying the operator.

Most of the operators will have a direct analog to PHP operators, and so the logic in PHPStan\Analyser\MutatingScope and PHPStan\Reflection\InitializerExprTypeResolver for handling operators has much of the logic we need already. However, this logic handles types that are irrelevant to SQL as well (such as arrays etc.), and it is buried so deep in the PHPStan architecture that it is likely unreasonable (or inappropriate) to use within QueryScope.

If we want to support SQL operators, would we need to duplicate all this logic from PHPStan? Even if we omit the parts that are irrelevant to SQL, this seems like it would be quite a lot of code duplication. Would it be reasonable to consider refactoring PHPStan to create a generic set of "type math" classes?

@staabm I'd be very grateful to hear your opinions about how to approach this, and whether or not it's something you'd like me to work on. Thanks!

@staabm
Copy link
Owner

staabm commented Mar 25, 2023

If we want to support SQL operators, would we need to duplicate all this logic from PHPStan? Even if we omit the parts that are irrelevant to SQL, this seems like it would be quite a lot of code duplication. Would it be reasonable to consider refactoring PHPStan to create a generic set of "type math" classes?

I think we could take different approaches. I am not sure the logic required is really 100% identical, therefore I guess it would make sense to build it in phpstan-dba itself.

I think we can/should directly base it on the SQL AST Nodes. the phpstan logic is based on nikic/php-parser AST nodes. we could transform the SQL AST Nodes into equivalent nikic/php-parser nodes and pass it into phpstan, but then I think the logic in phpstan would be to php-specific and in the end will not reflect the sql context/semantics we need.

summary: I think we should implement it our own for now and do it in a separate math class based on sql ast nodes

whether there is room for re-use can be decided after we implemented a rough prototype and we see similarities or not

@staabm
Copy link
Owner

staabm commented Mar 25, 2023

if you plan to work on this please do it for one operator at first and post a rough prototype, so we can decide how to approach/go on from it

@staabm staabm added this to the SQL AST milestone Apr 9, 2023
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