Derive conditional expression guards from the branch set difference - #6380
Open
ondrejmirtes wants to merge 8 commits into
Open
Derive conditional expression guards from the branch set difference#6380ondrejmirtes wants to merge 8 commits into
ondrejmirtes wants to merge 8 commits into
Conversation
When merging two branches, a variable qualified as a guard for conditional expressions only when its full our-branch type differed from the merged type. That rejected sound guards whose branch type survives the union unchanged (a bool widened over a nested branch, 1|null against a null pre-initializer), and it recorded overlapping full-type guards that a later narrowing could match even though the matched value may as well have come from the other branch. The guard type now records TypeCombinator::remove(ourType, theirType), the part of our type the other branch cannot produce. A representable remainder is disjoint from the other branch, so matching it later soundly selects this branch even when the full branch types overlap. When the subtraction is not representable, remove() keeps the full our-branch type and the merged-type exclusion applies to it as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
Mirrors the previous commit in turbo-ext/src/ScopeOps.cpp: the guard type in createConditionalExpressions() is TypeCombinator::remove() of the two branch types, guards require a certainty-Yes their-branch entry, and freshly created remainder holders are kept alive by an owned array while the guard scratch table borrows them. NeverType joins the native class-reference table for the empty-remainder check. Verified with the freshly built extension: smoke test, side-by-side method parity, signature parity, full NodeScopeResolverTest, and byte-identical analysis output with the extension on and off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
The set-difference conditional guards now let inference prove more cross-variable relationships, which surfaces real latent smells in PHPStan's own source under `make phpstan`. Remove the conditions the sharper narrowing proves unreachable, each a no-op that only ever evaluated one way: - NodeScopeResolver: the `$currentParameter === null` guard inside `if ($assignByReference)` - `$assignByReference` is only ever true when `$currentParameter !== null`. - Do/While/Foreach loop handlers: the `$prevEntryScope !== null` / `$unrolledTotalKeys === null` conjuncts are already implied by the earlier `$replay*` conjuncts (all set together on the same path). - SwitchHandler: `isset($branchFinalScopeResult)` is always true once `$prevScope !== null` (both are set in the same branch iteration). - RichParser: `$tokenLine` is always set once `$openParenthesisCount > 0`, so the `?? 1` fallback is dead. - SubstrDynamicReturnTypeExtension: `$substr` is a bool only on the version path where `substrReturnFalseInsteadOfEmptyString()` is true, so the inner version re-check and its empty-string branch are dead. - OptimizedDirectorySourceLocator: the `variable.undefined` ignore no longer matches - `$file` is proven defined once `$fetchedFunctionNode !== null`. The remaining findings are benign loop-variable reuse across adjacent loops (foreach.valueOverwrite / keyOverwrite / for.variableOverwrite); they are recorded in the baseline rather than reshaping readable code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
…nding
The previous commit removed the empty-string branch under `is_bool($substr)`
as dead code, reasoning that `$substr` is a bool only where
`substrReturnFalseInsteadOfEmptyString()` is true. That is a real regression:
the branch is live when PHPStan itself runs on a runtime PHP whose native
`substr()` returns `false` (old runtime) while analysing a target version
where substr yields `''`. There the runtime `substr()` call produces a bool,
`substrReturnFalseInsteadOfEmptyString()` is false, and the else branch
simulates the modern result by returning ConstantStringType(''). PHPStan's own
inference models `substr()` per the analysed version and cannot see the old
runtime path, so it reports the guard as always-true - a finding that must be
baselined, not resolved by deleting the branch.
Restore the file to its exact pre-commit content and record the single
`if.alwaysTrue` finding in the baseline instead. The other seven dead-code
removals in the previous commit are genuinely dead and stay removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
Collaborator
|
You've opened the pull request against the latest branch 2.3.x. PHPStan 2.3 is not going to be released for months. If your code is relevant on 2.2.x and you want it to be released sooner, please rebase your pull request and change its target to 2.2.x. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
…rty assignment Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
…sion The is_bool($substr) branch re-checked substrReturnFalseInsteadOfEmptyString(), which the sharper conditional guards proved always-true when analysing on PHP >= 8 (there substr() never returns false, so $substr is only bool via substrOrFalse()). The finding is genuinely version-specific and cannot live in the shared baseline. Route the non-mb_substr path through substrOrFalse(), whose false|string result does not depend on the runtime substr() semantics, and map false to the analysed version's result. Behaviour is unchanged; drops the version-specific baseline entry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGhnJQpUWRRpg6H5LuWJkA
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When merging two branch scopes, the conditional-expression guard type is now the set difference
TypeCombinator::remove(ourType, theirType)instead of requiring the whole branch-local type to differ from the merged type.This has two effects:
Includes the native
turbo-ext/src/ScopeOps.cppport of the guard-selection change and themake bump-turboversion bump, plus resolution of the self-analysis findings that the sharper certainty surfaces (7 provably-dead conditions removed in source, the benign loop-variable-reuse findings added to the baseline).Regression tests added for each issue; the whole
NodeScopeResolverTeststays green with and without the extension, and a local issue-bot run over all open-issue snippets shows the four fixes plus one collateral fix (#8360) and no regressions.Closes phpstan/phpstan#13833
Closes phpstan/phpstan#9685
Closes phpstan/phpstan#7706
Closes phpstan/phpstan#14421
Closes phpstan/phpstan#8360
🤖 Generated with Claude Code