fix: keep import edges when file_list holds relative paths - #696
Open
wkeuppens wants to merge 1 commit into
Open
fix: keep import edges when file_list holds relative paths#696wkeuppens wants to merge 1 commit into
wkeuppens wants to merge 1 commit into
Conversation
ts_build_dep_graph discarded every import edge when file_list used relative paths, which made every file in the project look orphaned. resolve_import returns a path in the same space as the source_file it is handed, so a relative file_list produces relative results. The builder then unconditionally joined those onto the absolute scan_path and tested membership against the relative file_set, so the lookup never matched. Reproduced on a real React Router project: same file set, relative file_list gives 0 edges, absolute gives 142, and a db.server.js with 15 importers was reported as having zero. Running the scanner over that project, orphaned findings drop from 49 to 24 and code quality goes from 42.8% to 72.0%. Resolved paths are now matched against the file set in whichever space it uses, with the previous scan_path-relative interpretation kept as a fallback. The existing test only covered an absolute file_list with a scan_path-relative resolver, which is the one combination that worked. Added a regression test for the relative case.
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
1 participant
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.
Problem
ts_build_dep_graphdiscards every import edge whenfile_listholds relative paths, so every file in the project is reported as orphaned.resolve_importreturns a path in the same space as thesource_fileit is handed — a relativefile_listtherefore produces relative results.graph.pythen unconditionally joins those onto the absolutescan_pathand tests membership against the relativefile_set, so the lookup never matches.Reproduced in isolation on a real React Router project, same file set both ways:
A
db.server.jswith 15 importers was reported as having zero.End to end, scanning that project:
The remaining 24 are React Router route files and framework entry points, which genuinely have no importers — a separate concern, see below.
This was easy to miss because the existing test happens to cover the one combination that works: an absolute
file_listwith ascan_path-relative resolver.Fix
Match the resolved path against the file set in whichever space that set uses, indexing by absolute path. The previous
scan_path-relative interpretation is kept as a fallback, so existing callers are unaffected.Added a regression test for the relative-
file_listcase. It fails before the change (assert set() == {'src/support.js'}) and passes after.Two things I did not fix here, to keep this focused
import_querymatches(import_statement ...)only, soawait import('./thing')produces no edge. One file in the project above is orphaned for exactly this reason.root.jsx/entry.server.jsxare loaded by convention and correctly have zero importers, but are still reported as orphaned. Several languages already have an "entry patterns" concept for this.Happy to follow up on either if useful.