test(static-analysis): Add dot-sourcing dependency check #6540
Conversation
WalkthroughAdds a PowerShell static-analysis Pester test and a supporting library that parse script ASTs, extract functions and call sites, resolve imported and dot-sourced dependencies, and perform BFS traversal to detect missing dot-sourced file dependencies; also adds multiple dot-sourced imports across several bin/libexec scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Pester Test Suite
participant Lib as StaticAnalysisLib
participant PS as PowerShell Parser
Test->>Lib: Get-ScriptAst(path)
Lib->>PS: Parse file (cache)
PS-->>Lib: AST / error
Lib-->>Test: AST
Test->>Lib: Get-ScriptFunction(path)
Lib->>Lib: Scan AST -> Function list
Lib-->>Test: Functions
Test->>Lib: Get-ScriptFunctionCallMap(path)
Lib->>Lib: Find invocations per function
Lib-->>Test: Call map
Test->>Lib: Get-ImportedDependency(path)
Lib->>Lib: Resolve dot-sources / using: / relative paths
Lib-->>Test: Resolved imports (or warnings)
Test->>Lib: Get-DotsourcingDependency(libs, entries)
rect rgb(230,250,230)
note right of Lib: BFS over functions and files\nuse call maps + lib functions\naccumulate per-path deps
Lib-->>Test: AllDeps
end
Test->>Test: Validate AllDeps exist and are imported/pre-imported
alt no missing deps
Test-->>Test: Pass
else
Test-->>Test: Fail (messages)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30–40 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
z-Fng
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
test/Scoop-00StaticAnalysis.Tests.ps1 (1)
34-38: Clarify when dependency parsing would fail.The error message "Error parsing dot-sourced dependency" suggests a parsing failure, but based on the implementation of
Get-DotSourcedDependency, it should always return an entry for each script (possibly an empty hashtable). This condition would only trigger if$AllDepsdoesn't contain a key for$Path.Consider making the error message more specific:
if (-not $Deps) { $FlagMissingDep = $true - Write-Host "Error parsing dot-sourced dependency for $Path." + Write-Host "Error: No dependency entry found for $Path. This may indicate a parsing failure." }test/Scoop-StaticAnalysisLib.ps1 (4)
17-53: LGTM: Function extraction logic is sound.The pipeline processing correctly aggregates functions across multiple scripts, and the duplicate detection prevents conflicts. The commented-out debug statements suggest the developer considered observability during development.
If duplicate functions across different files should be flagged as a potential issue, consider uncommenting the debug message or elevating it to a warning for better visibility during CI runs.
72-73: Document timeline for removing the workaround.The comment states this workaround "will be removed in the future," but provides no context about when or under what conditions it should be removed.
Consider adding an issue reference or version target:
- # Workaround for using: prefix in module import. Will be removed in the future. + # TODO: Remove this workaround for 'using:' prefix once all scripts are migrated (see issue #XXXX) $Dependency = $Dependency -replace 'using:', ''
119-120: Consolidate temporary workarounds.The workaround for the
script:prefix appears in multiple functions (here and inGet-ScriptFunctionCallat lines 160-161). Consider extracting this into a helper function to centralize the logic and make future removal easier.Add a helper function at the top of the file:
function Normalize-FunctionName { param ([string]$Name) # Workarounds for prefixes; remove once codebase is migrated $Name = $Name -replace 'script:', '' $Name = $Name -replace 'using:', '' return $Name }Then replace the workaround lines:
- # Workaround for script:url prefix in function calls. Will be removed in the future. - $Func = $Func -replace 'script:', '' + $Func = Normalize-FunctionName -Name $Func
179-262: LGTM: BFS-based transitive dependency analysis is well-designed.The breadth-first search correctly:
- Seeds the queue with direct script-level calls
- Resolves function definitions from either the current script or libraries
- Tracks the call stack for each dependency
- Handles transitive dependencies through recursive enqueuing
- Avoids infinite loops via the
$SearchedhashtableConsider if the constraint at line 233 (
-not $Dependency.ContainsKey($FuncDefination.File)) is too restrictive. It only records the first call stack to each dependency file, which may hide alternate call paths that could be valuable for debugging. If multiple call paths are useful, consider storing an array of call stacks per dependency:if ($FuncDefination.File -ne $Path) { if (-not $Dependency.ContainsKey($FuncDefination.File)) { $Dependency[$FuncDefination.File] = @() } $Dependency[$FuncDefination.File] += ,@($Invocation.CallStack) }However, this change should be weighed against increased memory usage and output complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/Scoop-00StaticAnalysis.Tests.ps1(1 hunks)test/Scoop-StaticAnalysisLib.ps1(1 hunks)
🔇 Additional comments (6)
test/Scoop-00StaticAnalysis.Tests.ps1 (3)
1-4: LGTM: Test suite setup is correct.The test suite structure and library import are properly configured.
6-20: LGTM: Test case configuration is well-structured.The two test scenarios properly cover different contexts:
- libexec scripts with an entry point that may pre-import dependencies
- bin scripts without an entry point
42-49: LGTM: Dependency validation logic is sound.The logic correctly identifies missing dependencies by checking if each required dependency file is either:
- Explicitly imported by the script itself, or
- Pre-imported by the entry point
The error reporting includes helpful call stack information.
test/Scoop-StaticAnalysisLib.ps1 (3)
111-134: LGTM: Call deduplication is appropriate.The
$Visitedhashtable correctly prevents recording duplicate invocations of the same function within a caller, which avoids redundant entries in the call map while preserving unique dependencies.
145-177: LGTM: Script-level call extraction is correct.The function properly extracts top-level function invocations (outside of function definitions), which is essential for identifying the entry points for dependency analysis. The deduplication logic is consistent with
Get-ScriptFunctionCallMap.
228-231: LGTM: Graceful handling of unresolved functions.The code correctly continues when a function cannot be resolved, which appropriately handles built-in cmdlets, .NET methods, and functions from dynamically loaded modules that aren't part of the static analysis scope.
z-Fng
force-pushed
the
add-dot-sourced-dep-check
branch
from
23e034d to
5c40340
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/Scoop-00StaticAnalysis.Tests.ps1 (1)
31-50: Consider clarifying error handling for parse failures.The test correctly identifies and reports missing dependencies. However, the error message at line 37 ("Error parsing dot-sourcing dependency") could be misleading if
$AllDeps[$Path]is simply absent (hashtable miss) versus a genuine parse error. The underlyingGet-DotsourcingDependencyreturns a hashtable, so an absent key is more likely than a parse error at this stage.Consider this refinement to distinguish between the two cases:
$Deps = $AllDeps[$Path] if (-not $Deps) { $FlagMissingDep = $true - Write-Host "Error parsing dot-sourcing dependency for $Path." + Write-Host "No dependency information found for $Path. This may indicate a parsing error or the script was not analyzed." }test/Scoop-StaticAnalysisLib.ps1 (1)
243-250: Dependency tracking stores only one call stack per file.The code at line 244 stores a single call stack per dependency file:
$Dependency[$FuncDefination.File] = @($Invocation.CallStack)This means if a script calls multiple functions from the same lib file via different code paths, only the last discovered call stack is preserved. While this is sufficient to prove the dependency exists, it may hide other usage patterns that could be valuable for debugging or comprehensive analysis.
If this behavior is intentional (showing one example per dependency), consider documenting it. Otherwise, consider accumulating all unique call stacks per dependency file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/Scoop-00StaticAnalysis.Tests.ps1(1 hunks)test/Scoop-StaticAnalysisLib.ps1(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: WindowsPowerShell
🔇 Additional comments (5)
test/Scoop-00StaticAnalysis.Tests.ps1 (2)
2-4: LGTM!The BeforeAll setup correctly dot-sources the static analysis library, making all helper functions available for the test suite.
7-20: LGTM!The test cases appropriately cover both scenarios: libexec scripts (which inherit dependencies from the scoop.ps1 entry point) and bin scripts (which have no pre-imported dependencies). The relative path usage with
$PSScriptRootensures portability.test/Scoop-StaticAnalysisLib.ps1 (3)
1-25: LGTM! Error handling properly implemented.The function now correctly handles parse failures with comprehensive error handling:
- File existence validation before parsing
- try-catch wrapper around
ParseFile- Clear warning messages
- Safe null return on errors
- Script-level caching to avoid redundant parses
This addresses the previous review feedback completely.
198-202: Verify error handling for lib collection.The
beginblock pipes$LibstoGet-ScriptFunctionandGet-ScriptFunctionCallMap, which aggregate results across all lib files. However, given the null-handling issues identified in those functions, if any lib file fails to parse:
- The pipeline will throw an exception and stop processing
- Subsequent libs won't be analyzed
- The dependency analysis will be incomplete or fail entirely
After applying the null-handling fixes to
Get-ScriptFunctionandGet-ScriptFunctionCallMap, verify that this aggregation gracefully handles parse failures in individual lib files. Run the test suite and check that a syntax error in one lib file doesn't break the entire analysis.
252-263: Defensive null check is appropriate.The
Where-Object { $_ -and (-not $Searched[$_.Name]) }filter includes a null check ($_) which might seem redundant, but is actually defensive programming:$FuncCallMapcould be null if the function definition has no body or no commands, so the filter correctly guards against iterating over null.
z-Fng
force-pushed
the
add-dot-sourced-dep-check
branch
from
5c40340 to
9bd4141
Compare
1 participant
Description
This PR makes the following changes:
Static Analysis -- Dot-sourcing dependency check
A Function-Level Dot-Sourced Dependency Check is a static analysis mechanism that scans PowerShell scripts to determine which external scripts (modules or libraries) are dot-sourced (i.e., loaded via the . operator) and which functions depend on them.
How It Works
A typical implementation proceeds as follows:
Parse the script into an AST (Abstract Syntax Tree)
Get-ScriptAst: The script is parsed into an AST structure, enabling static inspection of function definitions and calls.Extract function definitions and invocations
Get-ScriptFunction: Collect all functions defined in the current script.Get-ScriptFunctionCall: Collect all functions called in the current script.Get-ScriptFunctionCallMap: Collect all function calls inside each function body.Skip system-level cmdlets and built-in commands (retrieved via $cmdlet, $function, $alias, or Get-Command -CommandType Cmdlet).This step was removed because executingGet-Commandin a GitHub workflow takes too long.Resolve dot-sourced dependencies
Get-ImportedDependency: Detect all . expressions (dot-sourcing operators).Resolve these file paths (handling $PSScriptRoot, using: prefixes, etc.).
Record each dot-sourced file as a potential provider of external functions.
Cross-link function calls to definitions, Traverse dependencies (BFS)
Get-DotsourcingDependency: For each function call, determine whether it refers to a local function or one defined in a dot-sourced file. Use breadth-first traversal to resolve transitive dependencies between functions and scripts. Recursively check the function calls within each external function call. Build a dependency map that associates each script with the files and functions it depends on.Advantage
Helps to detect missing dot-sourced dependencies automatically.
Limitations
Motivation and Context
nightly_versionnot defined error when running scoop download #6385How Has This Been Tested?
Tested in GitHub workflow CI.
Checklist
developbranch.Summary by CodeRabbit
Tests
Chores