Skip to content

Upgrade Psalm to 7 and declare delay() as io - #470

Open
danog wants to merge 10 commits into
amphp:3.xfrom
danog:psalm-delay-capabilities
Open

danog wants to merge 10 commits into
amphp:3.xfrom
danog:psalm-delay-capabilities

Conversation

@danog

@danog danog commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Upgrades psalm/phar from 6.16.1 to 7.0.0-rc10. The Psalm job runs on PHP 8.2+ (the phar requires PHP ^8.2); PHP 8.1 still skips it. Analysis stays at phpVersion 8.1.

delay() is @psalm-capabilities io: it suspends the fiber on the event loop, the same model as usleep in Psalm 7 (vimeo/psalm#12482). Cancellation::subscribe() and unsubscribe() take their capabilities from the implementation through a purity template, so delay($timeout) is only io, and a call that passes a cancellation also pays for that cancellation. trapSignal() and Future::await() use the same template.

The event-loop io contract comes from the annotations in revolt/event-loop's own source (revoltphp/event-loop#134), so this config loads no stub. Psalm is clean against that branch. Packagist revolt/event-loop 1.0.9 doesn't have those annotations yet, so the Psalm job fails until a release includes them.

The other Psalm 7 findings are fixed in the code. There is no issue handler and no baseline. weakClosure() keeps its @psalm-suppress InvalidScope: the replacement closure is bound before it is invoked.

  • Callback ids come from str_increment() (a, b, …, aa). symfony/polyfill-php83 provides that function on PHP 8.1 and 8.2. Psalm is analyzing PHP 8.1, and it accepts the call because the polyfill declares the function.
  • awaitAnyN() documents $count as positive-int and still throws ValueError for a non-positive count. The docblock is not enforced at runtime.
  • Debug traces are captured only when assertions are enabled.
  • Public API is @api. Destructors that the engine calls on internal classes are @api as well, so properties read only from those destructors stay.

@kelunik

kelunik commented Oct 25, 2026

Copy link
Copy Markdown
Member

Note: @psalm-capabilities is a Psalm 7 annotation. The psalm job of this repository runs psalm/phar 6.16.1, whose docblock parser rejects unknown @psalm- tags ("Unrecognised annotation @psalm-capabilities", InvalidDocblock), so that job fails on this PR until the repository moves to Psalm 7.

Does this mean it also breaks for every user using Psalm 6 or earlier once we merge this?

@danog

danog commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

No issue there

@kelunik

kelunik commented Oct 25, 2026

Copy link
Copy Markdown
Member

Do you want to work on the Psalm 7 upgrade?

@danog

danog commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Sure why not

Psalm 7 rejects the previous docblocks and reports purity, reference, and dead-code issues. Fix those in the implementation, and declare capabilities so delay() stays io while callers pay for the cancellation they pass.
@danog danog changed the title Declare delay()'s Psalm capabilities: io Upgrade Psalm to 7 and declare delay() as io Oct 25, 2026
Comment thread src/Future/functions.php Resolved
}

/**
* @psalm-capabilities read-props|write-this-props|write-refs|io

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declaring this everywhere seems kind of noisy. What do we gain by declaring that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better type inference and security analysis in projects using amphp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that in general, but I'm interested in more specifics, so what are examples where this benefits users?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Fewer security false positives: Psalm specializes the taints of calls to pure and read-only functions and methods per call site. Tainted input passed at one call then no longer taints the result of every other call. Without annotations, Psalm can only infer this for functions and methods that can't be overridden.
  • Better type inference: Psalm remembers the results of methods of immutable classes between calls, so $a->get() !== null ? $a->get() : '' is a string. Without @psalm-immutable, the second call is still ?string.
  • Checked contracts: pure code can call the annotated functions, and every override and implementation is checked not to do more than the annotation allows.
  • Purity annotations: @psalm-mutation-free and @psalm-external-mutation-free are still accepted, but Psalm now suggests @psalm-capabilities. The self-accessing-*/self-mutating-* callable types are removed, @psalm-purity-from $param is replaced by purity templates, and purity arguments go in square brackets (Traversable[pure]<int, string>).

danog added 8 commits October 25, 2026 21:25
positive-int is only a docblock, so awaitAnyN still throws ValueError for a non-positive count. Callback ids follow str_increment (a, b, …, aa). PHP 8.3+ calls str_increment(); 8.1 and 8.2 advance the same sequence without ++, which Psalm 7 reports even inside the version guard.
symfony/polyfill-php83 provides str_increment() on PHP 8.1 and 8.2, so callback ids stay on that sequence. Debug traces are captured only when zend.assertions is 1.
The child callback shares those properties by reference, so it does not retain the cancellation and the destructor can still unsubscribe.
The nullsafe subscribe call is equivalent to the explicit null check. The unsubscribe site stays explicit so a null id is not passed without a suppress.
A first-class callable is rebound with an anonymous closure. The InvalidScope suppress stays because that closure is bound before it is invoked.
The yielded value is widened to object before the instanceof check, so a non-Future still throws TypeError at runtime.
Psalm now reads vendor/revolt/event-loop/stubs/EventLoop.phpstub.
That file is added by revoltphp/event-loop#134 and is no longer
copied into this repository.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants