Skip to content

Generate timestamp parsing that static analysis accepts without an annotation - #2132

Open
Amoifr wants to merge 1 commit into
async-aws:masterfrom
Amoifr:fix-2128-timestamp-static-analysis
Open

Generate timestamp parsing that static analysis accepts without an annotation#2132
Amoifr wants to merge 1 commit into
async-aws:masterfrom
Amoifr:fix-2128-timestamp-static-analysis

Conversation

@Amoifr

@Amoifr Amoifr commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #2128

The generated code relied on /** @var \DateTimeImmutable $d */ $d = ... to say that createFromFormat() cannot return false, which neither Psalm nor PHPStan honours. The generator now emits code that carries the same information in the type system, so no annotation and no baseline entry are needed:

-'LastUpdateTime' => /** @var \DateTimeImmutable $d */ $d = \DateTimeImmutable::createFromFormat('U.u', \sprintf('%.6F', $json['LastUpdateTime'])),
+'LastUpdateTime' => \DateTimeImmutable::createFromFormat('U.u', \sprintf('%.6F', $json['LastUpdateTime'])) ?: throw new UnparsableResponse('Invalid timestamp received.'),

It has to be an expression, since the generated code sits inside array literals, which rules out assert() and any statement form. A throw expression is the only shape that narrows the type there, and it happens to be the right behaviour: today a malformed timestamp reaches a \DateTimeImmutable typed property and dies on a TypeError with nothing pointing at the response. UnparsableResponse already exists in the core for exactly this and was simply never used by the generator.

Only the required branch changes. The optional one already narrows correctly through ($d = ...) ? $d : null, which is why it never needed a baseline entry.

Result. The two PossiblyFalsePropertyAssignmentValue entries are gone from psalm.baseline.xml, and psalm.phar reports No errors found!. On the 16 regenerated files, PHPStan at level 7 goes from 16 errors to none, all of the shape RestoreDateTime: DateTimeImmutable|false; the project's own level 6 stays green. ./generate --all is stable, a second run changes nothing.

One trap worth flagging for future generator work. RestJsonParser::$imports is private, so the $this->imports[] = ... I first wrote in JsonRpcParser created a dynamic property, deprecated since 8.2, and the import silently never reached the generated file. The five json-rpc clients came out referencing an unimported class, which PHPStan caught. Hence the shared rejectUnparsableTimestamp() on the parent rather than the same code in both parsers.

Touched clients: DynamoDb, Kinesis, S3Vectors, Ses and StepFunctions, with a changelog entry each.

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.

Generated code for required timestamp fields does not work for static analysis for json-rpc

1 participant