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

Use a @template for countDistinct #352

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

craigfrancis
Copy link
Contributor

Not complete.

The tests that use a non-literal-string for parameters 2 and 3, incorrectly return a literal-string.


These tests should work (results).

I suspect the original function definition, which implies only 1 parameter is used (source):

public function countDistinct($x)
{
    return 'COUNT(DISTINCT ' . implode(', ', func_get_args()) . ')';
}

Maybe the Stub file should be overriding this? Because I suspect the @template only considers the first parameter in this case, even though the Stub tries to correct it (source):

/**
 * @template T of string
 * @param T ...$x
 * @return (T is literal-string ? literal-string : string)
 */
public function countDistinct(...$x) {
}

@craigfrancis
Copy link
Contributor Author

craigfrancis commented Jul 18, 2022

I have opened PR #9911 on doctrine/orm, as it should be fixed there as well.

@ondrejmirtes
Copy link
Member

Stub files are only for overriding PHPDocs, not variadic-ness of parameters :)

@craigfrancis
Copy link
Contributor Author

Yep, thanks Ondřej, hopefully Doctrine will accept my PR :-)

@craigfrancis
Copy link
Contributor Author

My PR to fix the Expr::countDistinct() arguments has been accepted & merged for Doctrine ORM 3.0.

But 3.0 doesen't seem to have an expected release date. There are "what to expect" type articles from 2019, and it looks like they are working on 2.12 (current) and 2.13 (dev)... so I doubt this will be resolved any time soon.

Should I:

  1. Leave this PR here until phpstan-doctrine starts using doctrine-orm 3.0 (could be a long wait).
  2. Temporarily comment out the 2 tests that check the 2nd and 3rd arguments, countDistinctNonLiteralString3 and countDistinctNonLiteralString4 (this PR kinda works, but will incorrectly say something is a literal-string even if any of the 2nd or later arguments are provided with an unsafe string; so it won't identify mistakes until 3.0 is in use, but will allow Expr::countDistinct() to return a literal-string).
  3. Close this PR, and try again when 3.0 is in use (could be a while)?

@ondrejmirtes
Copy link
Member

Feel free to do option number 2) so that we have benefits faster :)

@craigfrancis
Copy link
Contributor Author

Thanks, have commented out those tests.

The failing tests are for PHP 8.2, which seems to be affecting main as well... and I did re-create the patch, as I made a mess when trying to rebase (one day I'll get that right).

@craigfrancis craigfrancis marked this pull request as ready for review September 21, 2022 15:31
@ondrejmirtes ondrejmirtes merged commit ed600bf into phpstan:1.3.x Oct 26, 2022
@ondrejmirtes
Copy link
Member

Thank you :)

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

Successfully merging this pull request may close these issues.

2 participants