Allow comments on classes

[nickvergessen]

Hi there,

thanks for the tool it's really helpful 👍🏼 (although I'm not sure we are using it correctly 🙈 )
The latest update however broke our usage due to a bug in #424 :(
and my understanding/observation makes me think it is not intended.

We hope this patch is okay as we'd like to keep our local comments on the classes:
https://github.com/nextcloud/spreed/blob/f60017eb778730874615a955b99510624c074534/lib/Signaling/Responses/DialOut.php#L29-L47

The problem is that the new list of strings you check is a bit flawed. This can be observed by var_dump($cases); after:

Valinor/src/Utility/Reflection/DocParser.php

Line 74 in f4884cf

$cases = self::splitStringBy($doc, '@phpstan-type', '@psalm-type');

Current master with my adjusted test file:

array(14) {
  [0]=>
  string(311) "Comment:
Some comment before types
* @psalm-type AliasWithEqualsSign = int
@psalm-type AliasWithoutEqualsSign int
@psalm-type AliasShapedArray = array{foo: string, bar: int}
@psalm-type AliasShapedArrayMultiline = array{
foo: string,
bar: int
}
@psalm-type AliasGeneric = GenericObjectWithPsalmLocalAlias<int>
 "
  [1]=>
  string(311) "Comment:
Some comment before types
* @psalm-type AliasWithEqualsSign = int
@psalm-type AliasWithoutEqualsSign int
@psalm-type AliasShapedArray = array{foo: string, bar: int}
@psalm-type AliasShapedArrayMultiline = array{
foo: string,
bar: int
}
@psalm-type AliasGeneric = GenericObjectWithPsalmLocalAlias<int>
 "
  [2]=>
  string(37) "Comment:
Some comment before types
* "
  [3]=>
  string(27) " AliasWithEqualsSign = int
"
  [4]=>
  string(28) " AliasWithoutEqualsSign int
"
  [5]=>
  string(49) " AliasShapedArray = array{foo: string, bar: int}
"
  [6]=>
  string(60) " AliasShapedArrayMultiline = array{
foo: string,
bar: int
}
"
  [7]=>
  string(55) " AliasGeneric = GenericObjectWithPsalmLocalAlias<int>
 "
  [8]=>
  string(37) "Comment:
Some comment before types
* "
  [9]=>
  string(27) " AliasWithEqualsSign = int
"
  [10]=>
  string(28) " AliasWithoutEqualsSign int
"
  [11]=>
  string(49) " AliasShapedArray = array{foo: string, bar: int}
"
  [12]=>
  string(60) " AliasShapedArrayMultiline = array{
foo: string,
bar: int
}
"
  [13]=>
  string(55) " AliasGeneric = GenericObjectWithPsalmLocalAlias<int>
 "
}

And I think that is more than intended, because the segment before the first matching $case string is being duplicated everytime (and the initial comment staying there too). The only reason this is not visible without my broken comment is that $types[$matches['name']] = self::findType($matches['type']); is basically deduplicating your results and saving the last split string as final result, which is the one that is correct. But if there was an invalid type in between it bails out with an exception.

With my PR the resulting cases are:

array(5) {
  [0]=>
  string(27) " AliasWithEqualsSign = int
"
  [1]=>
  string(28) " AliasWithoutEqualsSign int
"
  [2]=>
  string(49) " AliasShapedArray = array{foo: string, bar: int}
"
  [3]=>
  string(60) " AliasShapedArrayMultiline = array{
foo: string,
bar: int
}
"
  [4]=>
  string(57) " AliasGeneric = GenericObjectWithPhpStanLocalAlias<int>
 "
}

Result before this PR with the adjusted test file

1) CuyZ\Valinor\Tests\Integration\Mapping\Object\LocalTypeAliasMappingTest::test_values_are_mapped_properly
Error: Call to undefined method CuyZ\Valinor\Type\Parser\Lexer\Token\ColonToken::traverse()

…/src/Type/Parser/Lexer/TokenStream.php:36
…/src/Type/Parser/LexingParser.php:23
…/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:174
…/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:129
…/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:81
…/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:79
…/src/Definition/Repository/Reflection/ReflectionClassDefinitionRepository.php:67
…/src/Definition/Repository/Cache/CacheClassDefinitionRepository.php:31
…/src/Mapper/Tree/Builder/InterfaceNodeBuilder.php:47
…/src/Mapper/Tree/Builder/CasterProxyNodeBuilder.php:24
…/src/Mapper/Tree/Builder/IterableNodeBuilder.php:26
…/src/Mapper/Tree/Builder/StrictNodeBuilder.php:36
…/src/Mapper/Tree/Builder/ErrorCatcherNodeBuilder.php:33
…/src/Mapper/Tree/Builder/RootNodeBuilder.php:16
…/src/Mapper/TypeTreeMapper.php:44
…/src/Mapper/TypeTreeMapper.php:25
…/tests/Integration/Mapping/Object/LocalTypeAliasMappingTest.php:33

Result with this PR

$ php vendor/bin/phpunit --testsuite=integration tests/Integration/Mapping/Object/LocalTypeAliasMappingTest.php
PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

..                                                                  2 / 2 (100%)

Time: 00:00.019, Memory: 10.00 MB

OK (2 tests, 14 assertions)

[romm]

Thanks for the very detailed report and fix, very much appreciated. 👍