Add remove imports method#59
Conversation
|
It inherits from this commit: #51 |
Coverage Report for CI Build 27782496828Coverage increased (+0.1%) to 98.801%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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6475ddf93
ℹ️ 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".
| { | ||
| return !empty($this->nodeFinder->findFirst( | ||
| $nodes, | ||
| fn (Node $node) => $node instanceof Name && get_class($node) === Name::class && $node->getFirst() === $name, |
There was a problem hiding this comment.
The usage detector matches any plain Name node with the same first segment, regardless of context. This includes function and constant references, so a class import can be treated as "used" just because code calls a same-named function/constant, preventing cleanup of actually unused imports. That violates the method contract to remove unused imports safely and makes results depend on unrelated identifiers.
Useful? React with 👍 / 👎.
|
@artengin please resolve conflicts |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e76faa2338
ℹ️ 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".
| $nodesWithoutImports = array_filter($targetNodes, fn ($node) => !($node instanceof Use_) && !($node instanceof GroupUse)); | ||
|
|
||
| if ($this->hasUsageOf($importName, $nodesWithoutImports)) { |
There was a problem hiding this comment.
When a requested import is referenced only from PHPDoc, such as @return UserService or @var Collection<UserService>, the default removeImports(..., false) still deletes it because usage detection only scans AST nodes and never inspects doc comments. That breaks static-analysis and IDE type resolution even though the non-forced mode is documented as safe unless the import is unused; please include doc comments in the usage check before removing.
Useful? React with 👍 / 👎.
3 participants
refs: #60