staabm / phpstan-dba

PHPStan based SQL static analysis and type inference for the database access layer
https://staabm.github.io/archive.html#phpstan-dba
MIT License
250 stars 17 forks source link

AST: generic operator support #589

Open hemberger opened 1 year ago

hemberger commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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