Add remove imports method#59
Conversation
|
It inherits from this commit: #51 |
Coverage Report for CI Build 25502782139Coverage increased (+0.1%) to 98.688%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3253334ff4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
artengin
force-pushed
the
add-remove-imports-method
branch
from
3253334 to
adf1204
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adf1204014
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| protected function hasUsageOf(string $name, array $nodes): bool | ||
| { | ||
| return !empty($this->nodeFinder->find($nodes, fn (Node $node) => $node instanceof Name && $node->toString() === $name)); |
There was a problem hiding this comment.
PHP class-name resolution through use aliases is case-insensitive, but this check only treats an import as used when the Name::toString() value matches exactly. In files that reference the alias with different casing (for example use App\Models\User; and later user in a type hint), removeImports() will incorrectly drop a still-used import, which can change resolution to the current namespace and lead to runtime class-not-found failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a valid edge case per PHP spec, but in practice it's a non-issue - writing USER or user instead of User violates every coding standard and no real codebase does this.
…remove-imports-method
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efab0d1f94
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| $targetNodes = array_filter($targetNodes, fn ($node) => !($node instanceof Use_) || !empty($node->uses)); |
There was a problem hiding this comment.
array_filter() preserves numeric keys, so this assignment can leave $targetNodes sparse after imports are removed. In workflows that chain removeImports() before another visitor that inserts top-level statements (for example addImports()), getInsertIndex()/array_splice() treat those stale keys as positional offsets and can place new use statements after the class, producing invalid statement ordering. Reindex the filtered list (e.g., with array_values) before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
array_values is not needed here, testRemoveImportsThenAddImports confirms this
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 996ae46e48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $targetNamespace = array_find($nodes, fn ($node) => $node instanceof Namespace_); | ||
|
|
||
| if (!is_null($targetNamespace)) { | ||
| return $targetNamespace->stmts; |
There was a problem hiding this comment.
getNamespaceStatements returns only the first Namespace_ block, so removeImports() (and any visitor using this helper) skips imports declared in later namespace blocks of the same file. In files that use multiple namespaces (e.g., bracketed namespace syntax), requested removals in non-first namespaces are silently ignored, which makes the new API behave inconsistently and leaves stale imports behind.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
PSR-1 requires one namespace per file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de82a8221
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19e86cb28e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| $targetNodes = array_filter($targetNodes, fn ($node) => !$this->isEmptyImportNode($node)); |
There was a problem hiding this comment.
afterTraverse() assigns array_filter(...) directly to $targetNodes, but array_filter keeps original numeric keys. When this visitor is chained with addImports(), BaseNodeVisitorAbstract::getInsertIndex() uses those sparse keys as if they were positional indexes, so array_splice can insert a new use after the class (or below the import separator) if an earlier import was removed. This can produce invalid PHP in workflows like removeImports(...)->addImports(...); reindexing the filtered array (e.g., array_values) avoids the offset mismatch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
array_values is not needed here, testRemoveImportsThenAddImports confirms this
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 054bddcf1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| $targetNodes = array_filter($targetNodes, fn ($node) => !$this->isEmptyImportNode($node)); |
There was a problem hiding this comment.
afterTraverse() assigns array_filter(...) directly to $targetNodes, which preserves numeric keys; when an early import is removed (e.g., key 0 or a middle key), chaining removeImports(...)->addImports(...) can pass a sparse top-level statement array into NodeInserter::getInsertIndex(), and array_splice then inserts based on those stale key values, potentially placing new use statements after the class declaration. Fresh evidence: the new testRemoveImportsThenAddImports path only removes trailing imports from tests/Support/OriginStructures/class.php, so it does not exercise the sparse-key case that triggers this ordering bug.
Useful? React with 👍 / 👎.
3 participants
refs: #60