Fix condition literal-string is non-empty-string #10927
Conversation
VincentLanglet
force-pushed
the
nonEmptyString
branch
from
7ea7ce8
to
d135a6f
Compare
VincentLanglet
force-pushed
the
nonEmptyString
branch
from
d135a6f
to
f1ac418
Compare
VincentLanglet
force-pushed
the
nonEmptyString
branch
from
7d78bc8
to
78e526c
Compare
| @@ -35,8 +35,10 @@ public function __construct(Union $key, Union $value, bool $is_list, ?int $count | |||
|
|
|||
| /** | |||
| * @return ( | |||
| * $type is TArrayKey ? self : ( | |||
There was a problem hiding this comment.
TArrayKey doesn't exist.
As soon as I fixed TemplateInferredTypeReplacer.php, the error was detected.
| @@ -451,6 +451,12 @@ private static function replaceConditional( | |||
| null, | |||
| false, | |||
| false, | |||
| ) && null === Type::intersectUnionTypes( | |||
There was a problem hiding this comment.
The logic was
When doing A is B ? C : D:
- If A is contained by B => It's C
- If B is not contained by A => It's D
- Else, it's C|D
I think this logic is wrong and does not work well with string type like literal-string and non-empty-string.
literal-string is non-empty-string ? int : float.
Since non-empty-string is not contained by literal-string it was considering the result as a float.
But since literal-string&non-empty-string is non empty, the result should have been int|float.
| @@ -787,8 +789,26 @@ public static function intersectUnionTypes( | |||
|
|
|||
| //if a type is contained by the other, the intersection is the narrowest type | |||
| if (!$intersection_performed) { | |||
| $type_1_in_2 = UnionTypeComparator::isContainedBy($codebase, $type_1, $type_2); | |||
| $type_2_in_1 = UnionTypeComparator::isContainedBy($codebase, $type_2, $type_1); | |||
| $type_1_in_2 = UnionTypeComparator::isContainedBy( | |||
There was a problem hiding this comment.
When I tried to use intersectUnionTypes with $allow_float_int_equality = false I got a bug.
Seems like those parameters were not passed to every calls.
VincentLanglet
force-pushed
the
nonEmptyString
branch
from
eabe01e
to
91ea61e
Compare
| @@ -891,7 +891,7 @@ function getSomethingElse() | |||
| interface ContainerInterface | |||
| { | |||
| /** | |||
| * @template TRequestedInstance extends InstanceType | |||
| * @template TRequestedInstance of InstanceType | |||
There was a problem hiding this comment.
@template TRequestedInstance extends InstanceType
is not a correct syntax, of should be used.
As soon as I fixed the TemplateInferredTypeReplacer.php the error occured.
| @@ -400,7 +400,8 @@ public function initialize( | |||
| $this->path_mapper->configureClientRoot($this->getPathPart($rootUri)); | |||
| } | |||
|
|
|||
| return call( | |||
| /** @var Promise<InitializeResult> $promise */ | |||
| $promise = call( | |||
There was a problem hiding this comment.
This avoid
The type 'Amp\Promise<LanguageServerProtocol\InitializeResult>|Amp\Promise<mixed>' is more general than the declared return type 'Amp\Promise<LanguageServerProtocol\InitializeResult>' for Psalm\Internal\LanguageServer\LanguageServer::initialize
This is technically true because the conditional return type is
(T is Promise ? false : (T is \Generator ? (TGenerator is Promise ? Promise<TGeneratorPromise> : Promise<TGeneratorReturn>) : int))
and the check InitializeResult is Promise cannot be resolved since InitializeResult&Promise would be a possible type.
|
The only error I get now is coming from the test https://github.com/vimeo/psalm/pull/10927/files/91ea61e4170656cb7a54347271d054e9037106d6#diff-19d1ea03f9fa43528c7190e1c91a6a4f216c48b742e3e2dcdd4e75e750c6a7a5 But this failure also happen in 5.x as soon as I fix the phpdoc of the tests Should I comment the test in a separate PR @orklah since the test is wrong as shown by #10929 ? |
Any recommendation about such situation @weirdan ? |
Hi @orklah, this PR is similar to #10912
Closes #10926