fix: resolve ${VAR} bash sources against the variable's real base#2193
Closed
Souptik96 wants to merge 1 commit into
Closed
fix: resolve ${VAR} bash sources against the variable's real base#2193Souptik96 wants to merge 1 commit into
Souptik96 wants to merge 1 commit into
Conversation
…aphify-Labs#2172) Graphify-Labs#2079 resolved `source "${VAR}/lib/x.sh"` by stripping the leading expansion and resolving the literal suffix against the sourcing script's own directory. That is correct for the canonical `DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"` idiom, but wrong whenever the variable points somewhere else. With ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" source "${ROOT}/lib/utils.sh" the real target is <root>/lib/utils.sh, yet a same-named decoy under the script dir (<root>/scripts/lib/utils.sh) won: a wrong imports_from edge to a real node. Track top-level variable assignments and use the assigned base for the leading variable: - the script-dir idiom, including any number of trailing `/..` hops (and the `$0` spelling), resolves to the script dir walked up that many times - a literal value resolves as-is when absolute, or against the script dir when relative - anything else -- a value built from other variables, or command substitution we do not model -- stays untracked, so the previous script-dir guess is kept Only the base changes. The suffix guards are untouched: an expansion left in the suffix, an empty suffix, and a `..` component in the suffix are all still rejected, the edge is still gated on is_file() and still emitted as INFERRED.
Collaborator
|
Thanks @Souptik96. Shipped in v0.9.27 (cherry-picked to v8). Closed-unmerged here, but it's in the release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.27 |
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
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.
Fixes #2172
Reproduced
Confirmed the false positive against
v8(0.9.26) with your exact shape —scripts/deploy.shwhereROOTis the script dir's parent, plus a same-named decoyunder the script dir:
Before — bound to the decoy:
After — bound to the real file:
Fix
Track top-level variable assignments and use the assigned base for the leading variable,
exactly as the issue suggests:
"$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"— resolvesto the script dir walked up once per trailing
/..hop (any number, and the$0spellingis accepted too)
model) stays untracked, so the previous script-dir guess is kept rather than binding
somewhere invented
Only the base changes. Every existing guard is untouched: an expansion left in the suffix,
an empty suffix, and a
..component in the suffix are all still rejected, the edge is stillgated on
is_file(), and it is still emitted asINFERRED.Assignments are read from the parse tree's top-level
variable_assignmentnodes, so areassignment inside a function body doesn't feed the base.
Testing
Three new tests in
tests/test_extract.py— the false positive, plus two guards that thebehaviour #2079 added still holds:
Confirmed the false-positive test actually catches the bug — same tests with
graphify/extractors/bash.pyreverted tov8(note the two guard tests still pass there,which is the point: they assert preserved behaviour):
Affected suites and both pre-commit gates:
Those 4 failures are pre-existing on Windows, not from this PR — exactly the 4
test_extract.pyfailures in my baseline on unmodifiedv8(66d8110), all symlink-related(
test_collect_files_*, e.g.OSError: [WinError 1314] A required privilege is not held by the client). Full-suite baseline there is 45 failed / 3578 passed / 15 skipped. NoLinux/macOS box here to confirm they're green.
Sent separately from #2171 (the extensionless/bare-name source gaps) so the two are
reviewable independently, though both touch
graphify/extractors/bash.py.