Problem/Motivation

Psalm and phpstan support complex strings that provide some additional static analysis like "numeric-string" or "class-string" that can help find type miss-matches and logic errors.

https://phpstan.org/writing-php-code/phpdoc-types
https://psalm.dev/docs/annotating_code/type_syntax/scalar_types/

However using these breaks code's FunctionCommentSniff.

----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 244 | ERROR | [x] Expected "numericstring|null" but found
     |       |     "numeric-string|null" for parameter type
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Steps to reproduce

Here's a method that will trigger the sniff failure.

  /**
   * Apply numeric formatting rules from style guide.
   *
   * @param numeric-string|null $value
   *   Unformated raw numeric data.
   * @param string $format
   *   The expected format.
   * @param int $precision
   *   The numeric "precision" to apply to the output.
   *
   * @return string
   *   A formatted number ready for display.
   */
  private function formatData($value, string $format, $precision) {
    //
  }

Proposed resolution

At least one of the problems is that FunctionCommentSniff::suggestType treats '-' as an invalid character in types.

I poked at this briefly and it seems to be a bit more complex with fixing that just revealing some additional checks in FunctionCommentSniff that fail so probably more fixes would be required.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Oh hey me from the past. Why didn't you fix this? This is also true for template types like this:

/**
 * return iterable<\My\Classes\Biz>
 */
function foo() {
  foreach ($this->bar as $value) {
    yield new Biz($value);
  }
}

Both of these seem to boil down to FunctionCommentSniff::suggestType not allowing the characters so the suggestion doesn't match the existing value as noted in the proposed solution.

It looks like this is going to break again if you try to use they're array key format like array<int, int> since I don't see space as an allowed character. I have to wonder if the regex is going to even really work.

mfb’s picture

klausi’s picture

Started to work on this in https://github.com/pfrenssen/coder/pull/194 , not finished yet.

I think this issue will be the final straw where I will remove quite a bit of data type validation from Coder to avoid contradictions with PHPStan.

I want to remove some more code that seems of low value in FunctionCommentSniff to make it simpler as well. Also need to add tests for @return data types.

  • klausi authored d5911f81 on 8.3.x
    feat(FunctionComment): Allow PHPStan advanced string types (#3205017)
    
    
klausi’s picture

Version: 8.3.12 » 8.3.x-dev
Status: Active » Fixed

Finished the cleanup, added more tests and merged it. Please test!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.