Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bad8b5e51b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR improves Phan’s multi-pass single-process analysis to better converge inferred types across inter-file call chains. It enhances --analyze-twice by reordering files for the second pass based on observed call dependencies, and adds an optional worklist-driven --analyze-until-convergence mode for targeted re-analysis until a fixpoint (or an iteration cap) is reached.
Changes:
- Rework
--analyze-twiceto use a no-op issue collector for pass 1 and reorder pass 2 via a new call-dependency topological sort. - Add
--analyze-until-convergenceto run additional targeted convergence passes after the two full passes. - Introduce new infrastructure classes:
ConvergenceWorklistandNullCollector, plus config keys and regenerated CLI help.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Phan/Phan.php | Switch pass 1 to a type-gathering pass (no issue collection), reorder pass 2 using dependency info, and optionally run convergence passes. |
| src/Phan/Analysis/ConvergenceWorklist.php | New helper for indexing tracked elements, topologically reordering pass 2, and running the post-pass worklist convergence loop. |
| src/Phan/Output/Collector/NullCollector.php | New no-op issue collector used to avoid collecting/emitting issues during pass 1. |
| src/Phan/CLI.php | Add --analyze-until-convergence flag; make --analyze-twice force reference tracking. |
| src/Phan/Config.php | Add internal config keys for convergence mode and max iterations. |
| internal/CLI-HELP.md | Update CLI help text to document the new flag. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR improves Phan’s single-process multi-pass analysis to help inferred return types converge across call chains, enhancing the effectiveness of --analyze-twice and adding a new --analyze-until-convergence mode for targeted fixpoint re-analysis.
Changes:
- Reorders the second pass of
--analyze-twiceusing a call dependency graph derived from reference tracking. - Makes pass 1 a type-gathering pass by suppressing issue collection via a new
NullCollector. - Adds
--analyze-until-convergenceto run additional targeted re-analysis passes until types stabilize or a max-iteration cap is hit.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Phan/Phan.php | Implements suppressed-issue pass 1, reordered pass 2, and optional convergence loop. |
| src/Phan/Analysis/ConvergenceWorklist.php | New worklist engine for pass-2 reordering and convergence iterations. |
| src/Phan/Output/Collector/NullCollector.php | New no-op collector used to skip pass-1 issue collection overhead. |
| src/Phan/CLI.php | Adds --analyze-until-convergence and ensures reference tracking is enabled for convergence features. |
| src/Phan/Config.php | Adds internal config keys for convergence enablement and max iterations. |
| internal/CLI-HELP.md | Updates CLI help text to document the new flag. |
| tests/run_test | Adds a new test suite entry for convergence testing. |
| tests/run_all_tests | Includes the convergence test suite in the full test run list. |
| tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected | Updates expected output (removes an issue line). |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR improves Phan’s multi-pass analysis so inferred return types converge more reliably across call chains, and adds an optional worklist-based fixpoint mode for further convergence without re-analyzing the entire codebase repeatedly.
Changes:
- Reworks
--analyze-twiceto suppress issue collection in pass 1 (type-gathering) and reorder files for pass 2 based on call dependencies. - Adds
--analyze-until-convergence, which performs additional targeted re-analysis passes (with an iteration cap) until return types stop changing. - Adds a new no-op
NullCollectorand a newConvergenceWorklisthelper, plus a new test suite.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Phan/Phan.php |
Implements type-gathering pass 1 (NullCollector), reordered pass 2, and optional convergence worklist loop. |
src/Phan/Analysis/ConvergenceWorklist.php |
New helper to snapshot return types, compute changed-caller files, topologically reorder pass 2, and run the convergence loop. |
src/Phan/Output/Collector/NullCollector.php |
New issue collector that discards issues during type-gathering passes. |
src/Phan/CLI.php |
Adds --analyze-until-convergence, makes --analyze-twice force reference tracking, updates help text. |
src/Phan/Config.php |
Adds internal config keys for convergence mode and iteration cap. |
internal/CLI-HELP.md |
Regenerates CLI help to include the new flag. |
tests/convergence_test/* |
Adds a new test suite intended to validate convergence behavior. |
tests/run_test / tests/run_all_tests |
Wires the new convergence test suite into the test runner. |
tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected |
Updates expected output due to changed multi-pass issue collection semantics. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR improves Phan’s single-process multi-pass analysis to help type inference converge across multi-hop call chains by reordering pass 2 based on reference tracking, suppressing pass-1 issue collection, and optionally running targeted re-analysis until a fixpoint is reached.
Changes:
- Enhance
--analyze-twiceby (a) forcing reference tracking, (b) discarding pass-1 issues via aNullCollector, and (c) reordering pass-2 files using a dependency graph built from reference lists. - Add
--analyze-until-convergenceto run additional targeted “worklist” re-analysis iterations after pass 2, capped by__convergence_max_iterations. - Add a new convergence-focused test suite hook (but currently missing its committed fixtures).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Phan/Phan.php |
Implements two-pass flow with pass-1 NullCollector, pass-2 reordering, and optional convergence loop. |
src/Phan/Analysis/ConvergenceWorklist.php |
New worklist + topo reordering logic using tracked reference lists and type snapshots. |
src/Phan/Output/Collector/NullCollector.php |
New no-op issue collector to suppress pass-1 issue storage. |
src/Phan/CLI.php |
Adds --analyze-until-convergence; forces reference tracking for analyze-twice modes; help text updates. |
src/Phan/Config.php |
Adds internal config keys __analyze_until_convergence and __convergence_max_iterations. |
internal/CLI-HELP.md |
Documents the new CLI flag. |
tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected |
Updates expected output to reflect pass-1 issue suppression / stale issue removal. |
tests/run_test |
Adds a dispatch case for the new convergence test suite. |
tests/run_all_tests |
Adds the convergence test suite to the default run list. |
tests/convergence_test/test.sh |
New test runner for --analyze-until-convergence (currently expects fixtures that aren’t present). |
tests/convergence_test/.phan/config.php |
New config for the convergence test suite (expects a local src/ tree). |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Improves Phan’s multi-pass analysis so type inference converges more reliably by reordering pass 2 based on call dependencies, and adds an optional worklist-based re-analysis mode to iterate until a fixpoint.
Changes:
- Update
--analyze-twiceto suppress pass-1 issues (NullCollector) and reorder files for pass 2 using reference-tracked call dependencies. - Add
--analyze-until-convergenceto run additional targeted re-analysis iterations after pass 2. - Add a new convergence integration test suite and update expected outputs/help text.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Phan/Phan.php | Orchestrates pass-1 type gathering, pass-2 reordered analysis, and optional convergence loop. |
| src/Phan/Analysis/ConvergenceWorklist.php | New worklist implementation for file reordering + convergence re-analysis. |
| src/Phan/Output/Collector/NullCollector.php | New no-op collector to suppress pass-1 issue collection. |
| src/Phan/CLI.php | Adds --analyze-until-convergence, wires config flags, updates help text. |
| src/Phan/Config.php | Adds internal config keys for convergence mode + max iterations. |
| internal/CLI-HELP.md | Regenerated CLI help text to include the new flag. |
| tests/run_test | Adds a runner entry for the new convergence integration test suite. |
| tests/run_all_tests | Includes convergence suite in the default test run list. |
| tests/convergence_test/test.sh | New integration test runner invoking --analyze-until-convergence. |
| tests/convergence_test/.phan/config.php | Config for convergence integration tests. |
| tests/convergence_test/src/001_a_app.php | Test fixture (consumer) validating multi-hop type inference. |
| tests/convergence_test/src/001_b_middle.php | Test fixture (middle layer) for call-chain inference. |
| tests/convergence_test/src/001_c_provider.php | Test fixture (provider/producer) returning the concrete type. |
| tests/convergence_test/expected/001_a_app.php.expected | Expected issue verifying the inferred type reaches the consumer. |
| tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected | Expected output adjusted due to pass-1 issue suppression behavior. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Enhances Phan’s single-process multi-pass analysis to improve type inference convergence by reordering pass-2 analysis based on discovered call dependencies, and adds an optional worklist-based incremental re-analysis mode.
Changes:
- Make
--analyze-twicesuppress pass-1 issue collection (type-gathering only) and reorder files for pass 2 using reference-based dependencies. - Add
--analyze-until-convergenceto run additional targeted re-analysis iterations until return types stabilize (or a capped limit is reached). - Add/adjust test suites and expected outputs to cover the new analysis behavior and CLI flags.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected | Updates expected output due to pass-1 issue suppression / stale-issue elimination. |
| tests/run_test | Adds a new fake test suite entry to run tests/convergence_test. |
| tests/run_all_tests | Registers the new convergence test suite in the full test run. |
| tests/convergence_test/test.sh | New integration test runner invoking --analyze-until-convergence. |
| tests/convergence_test/src/001_c_provider.php | Adds a “provider” file to form a multi-hop inference chain. |
| tests/convergence_test/src/001_b_middle.php | Adds a “middle layer” file to form a multi-hop inference chain. |
| tests/convergence_test/src/001_a_app.php | Adds an “app” file that depends on inferred return types from downstream files. |
| tests/convergence_test/expected/001_a_app.php.expected | Expected issue demonstrating that the inferred type reaches the consumer. |
| tests/convergence_test/.phan/config.php | Test configuration for the new convergence test suite. |
| src/Phan/Phan.php | Restructures analysis flow for --analyze-twice/--analyze-until-convergence (NullCollector pass 1, reordered pass 2, optional worklist loop). |
| src/Phan/Output/Collector/NullCollector.php | Introduces a no-op collector to avoid pass-1 collection/formatting overhead. |
| src/Phan/Config.php | Adds internal config keys for convergence mode and iteration cap. |
| src/Phan/CLI.php | Adds --analyze-until-convergence flag wiring and help text; ensures daemon mode disables it. |
| src/Phan/Analysis/ConvergenceWorklist.php | New implementation for dependency-based reordering and worklist re-analysis loop. |
| internal/CLI-HELP.md | Regenerated CLI help including the new flag. |
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR improves Phan’s single-process multi-pass analysis so type inference can converge across longer call chains. It enhances --analyze-twice by reordering files for the second pass based on reference/call dependencies, and introduces --analyze-until-convergence for additional targeted re-analysis until types stabilize (or a cap is reached).
Changes:
- Reworks
--analyze-twiceinto a “type-gathering” pass 1 (issues suppressed) followed by a reordered full-analysis pass 2. - Adds
--analyze-until-convergenceto run targeted convergence passes using a worklist algorithm after pass 2. - Adds a new convergence-focused test suite and updates expected outputs affected by suppressing pass-1 issues.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Phan/Phan.php |
Implements pass-1 NullCollector, pass-2 file reordering, and optional convergence loop in single-process mode. |
src/Phan/Analysis/ConvergenceWorklist.php |
New: builds dependency ordering for pass 2 and runs worklist-based targeted re-analysis until convergence. |
src/Phan/Output/Collector/NullCollector.php |
New: no-op collector used to suppress pass-1 issue collection. |
src/Phan/CLI.php |
Adds --analyze-until-convergence flag, daemon-mode disabling notice, and help text. |
src/Phan/Config.php |
Adds internal config keys for convergence mode and its iteration cap. |
internal/CLI-HELP.md |
Regenerated CLI help to include the new flag. |
tests/run_test |
Adds a test-suite entry for convergence_test. |
tests/run_all_tests |
Ensures convergence_test runs in the full test suite list. |
tests/convergence_test/test.sh |
New test runner invoking --analyze-until-convergence and diffing expected output. |
tests/convergence_test/.phan/config.php |
New: config for the convergence test fixture. |
tests/convergence_test/src/001_a_app.php |
New fixture: top-level consumer file (alphabetically first) that should see converged types. |
tests/convergence_test/src/001_b_middle.php |
New fixture: middle layer delegating to provider (no declared return type). |
tests/convergence_test/src/001_c_provider.php |
New fixture: provider returning a concrete service instance (inference source). |
tests/convergence_test/expected/001_a_app.php.expected |
New expected output asserting inferred type flows back to the consumer. |
tests/track_all_inferred_types_test/expected/001_override_return_types.php.expected |
Updates expectations due to pass-1 issue suppression removing stale collector entries. |
You can also share your feedback on Copilot code review. Take the survey.
Improve
--analyze-twiceand add--analyze-until-convergenceAddresses #5488
Problem
--analyze-twicere-analyzes all files a second time to let type inference converge. However, it analyzes files in the same order both times. When type information flows through a chain of method calls with undeclared return types (A calls B calls C), each hop in the chain requires an additional pass to resolve. Since files are analyzed in alphabetical order and the dependency often goes the other way, two passes only resolve chains 1 hop deep.After
--analyze-twice,$objis still unknown because:SomeService, but B was analyzed before C so B is still unknown, and A was analyzed before B.Changes
1. Smarter
--analyze-twice: topological file reordering for pass 2After pass 1, Phan now builds a call dependency graph using the reference lists collected during analysis. It topologically sorts the file list so that files defining called methods ("producers") are analyzed before files that call them ("consumers"). Pass 2 uses this reordered list.
With reordering, the example above is analyzed in pass 2 as C, B, A — resolving the full chain regardless of depth.
To support this,
--analyze-twicenow enablesforce_tracking_referencesso thatgetReferenceList()is populated during pass 1.2. Pass 1 is now a type-gathering pass
Since pass 2 will re-analyze everything with better type info, pass 1 issues are wasted work — they get overwritten or become stale. Pass 1 now uses a
NullCollectorthat discards all issues, skipping formatting and collection overhead. The real collector is restored for pass 2.This also eliminates a subtle false-positive source: in the old behavior, pass 1 could emit warnings based on incomplete type info that weren't re-emitted in pass 2, but persisted in the collector because the
BufferingCollectoronly overwrites entries with matching keys — it doesn't remove stale ones.3. New
--analyze-until-convergenceflagFor edge cases where even the reordered pass 2 isn't sufficient (e.g., circular dependencies), the new
--analyze-until-convergenceflag adds worklist-based targeted re-analysis after pass 2:This is equivalent to running unlimited full passes but reaches the fixpoint in far fewer file analyses since only downstream files are re-analyzed.
--analyze-until-convergenceimplies--analyze-twice.Files
src/Phan/Analysis/ConvergenceWorklist.phpsrc/Phan/Output/Collector/NullCollector.phpsrc/Phan/Phan.phpsrc/Phan/CLI.php--analyze-until-convergenceflag;force_tracking_referencesfor--analyze-twice; help textsrc/Phan/Config.php__analyze_until_convergence,__convergence_max_iterationsinternal/CLI-HELP.mdTesting
3-hop chain (A calls B calls C):
--analyze-twice:$objhas empty union type--analyze-twice:$objinferred as\SomeService4-hop chain (A calls B calls C calls D):
--analyze-twice: resolves in 2 passes (reordering handles any depth)--analyze-until-convergence: also resolves, with 0 additional convergence passes neededSingle-pass mode (no flags) is unchanged.
All 2342 existing tests pass. Phan self-analysis is clean.
Constraints
--analyze-twice)--analyze-twicenow enablesforce_tracking_references, adding some memory overhead for reference tracking--analyze-until-convergencehas a safety cap of 10 iterations (configurable via__convergence_max_iterationsin config)