feat: Add ast/config, ast/stack, stackutils support packages for stack/values features - #128
feat: Add ast/config, ast/stack, stackutils support packages for stack/values features#128diofeher wants to merge 4 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughCore AST no longer tracks include blocks; include/dependency helpers were removed. New packages Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Def as Definition code
participant C as ConfigAST (wrapper)
participant I as IndexedAST
participant S as Includes Scope
Def->>C: NewConfigAST(store.AST)
C->>I: wrap IndexedAST
C->>I: FindNodeAt(hcl.Pos)
I-->>C: *IndexedNode / nil
C->>I: (when building) iterate nodes
I-->>C: nodes...
C->>S: add include blocks to Includes scope
C-->>Def: GetIncludeLabel / GetDependencyLabel result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
internal/ast/ast_test.go (1)
287-287: Consider a more meaningful "no match" predicate.Using
func(*ast.IndexedNode) bool { return false }works, but it's a bit of a tautology — it'll always return nil regardless of the AST, so the test no longer really exercisesFindFirstParentMatch's traversal. Now thatisIncludeBlock/isDependencyBlocklive ininternal/ast/config, a nice replacement would be a predicate that's genuinely unmatched by this fixture, e.g. looking for aninputsblock or a non-existent block type, so the traversal is actually walked end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/ast_test.go` at line 287, The test currently uses a tautological predicate func(*ast.IndexedNode) bool { return false } which prevents exercising FindFirstParentMatch traversal; change the predicate to a realistic but unmatched condition (for example, test for a block type not present in the fixture such as "inputs" or any non-existent block name) so the traversal actually walks the tree; reference the FindFirstParentMatch call and replace the predicate with one that inspects the node (e.g., node.Node.Type or a helper like isIncludeBlock/isDependencyBlock style check) and returns true only for that absent block type to ensure full traversal.internal/ast/config/config_test.go (1)
41-45: Method-value non-nil checks are always true.Same note as in
stack_test.go:assert.NotNil(t, configAST.FindNodeAt)and friends will never fail because method values on a non-nil interface are always non-nil.require.NotNil(t, configAST)is sufficient; for real behavior coverage, call the methods. Not blocking, just noise to prune.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/config/config_test.go` around lines 41 - 45, The test currently asserts method-value non-nil on configAST (assert.NotNil(t, configAST.FindNodeAt), GetIncludeLabel, GetDependencyLabel, GetLocals, GetIncludes) which is meaningless; replace these with a single require.NotNil(t, configAST) and then call the methods (e.g., configAST.FindNodeAt(...), configAST.GetIncludeLabel(), configAST.GetDependencyLabel(), configAST.GetLocals(), configAST.GetIncludes()) with minimal inputs to verify real behavior or return values, asserting expected outputs or non-nil results instead of checking the method values themselves.internal/ast/config/config.go (2)
45-53: Scope construction walks the entire index — fine today, worth noting.
buildIncludesScopeiterates every indexed node across every line. For typicalterragrunt.hclfiles this is trivially cheap, but since the index already preserves structure, you could alternatively walk top-level blocks fromHCLFile.Body.(*hclsyntax.Body).Blocks— O(top-level blocks) instead of O(all nodes). Not a bug, just a heads-up if stack files ever grow large.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/config/config.go` around lines 45 - 53, buildIncludesScope currently walks every node in c.Index which is O(all nodes); change it to iterate only top-level blocks instead. In function buildIncludesScope, obtain the body via the file AST (e.g. assert c.HCLFile.Body to *hclsyntax.Body and iterate its Blocks), call isIncludeBlock(block) for each top-level block and c.includes.Add(block) when true; if the type assertion to *hclsyntax.Body fails, fall back to the existing c.Index iteration to preserve behavior.
72-114: Same dead branch pattern as instack.go.
isIncludeBlockandisDependencyBlockboth guaranteelen(Labels) > 0, so thename := ""; if labels := ...; len(labels) > 0guard here (and inGetDependencyLabel) is dead code —labels[0]is always safe after the matcher succeeds. Feel free to simplify or, better, share a helper with the equivalent methods ininternal/ast/stack/stack.go(they're all the same walk-attr-then-block-and-grab-first-label shape).Nothing blocking; just a readability win.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/config/config.go` around lines 72 - 114, Both GetIncludeLabel and GetDependencyLabel contain a dead len(labels) > 0 guard because their matchers (isIncludeBlock / isDependencyBlock) guarantee a block with at least one label; remove the redundant check and directly read labels[0], or better yet factor the common walk-attr-then-block-and-grab-first-label pattern into a small helper (e.g., GetFirstLabelFromParentAttribute) and use it from GetIncludeLabel, GetDependencyLabel and the similar methods in internal/ast/stack/stack.go to eliminate duplication and clarify intent.internal/ast/stack/stack.go (2)
40-78: Dead branch + duplication inGetUnitLabel/GetStackLabel.Two small things worth cleaning up:
isUnitBlock/isStackBlockalready guaranteelen(Labels) > 0(lines 127-136), so thename := ""+if labels := ...; len(labels) > 0dance is unreachable-guard code.labels[0]is always safe here.GetUnitLabelandGetStackLabelare structurally identical to each other (and toGetIncludeLabelininternal/ast/config/config.go). A tiny shared helper would DRY this up nicely.♻️ Suggested tidy-up
-// GetUnitLabel returns the label of the given node, if it is a unit block -func (s *stackAST) GetUnitLabel(node *ast.IndexedNode) (string, bool) { - attr := ast.FindFirstParentMatch(node, ast.IsAttribute) - if attr == nil { - return "", false - } - - unitBlock := ast.FindFirstParentMatch(attr, isUnitBlock) - if unitBlock == nil { - return "", false - } - - name := "" - if labels := unitBlock.Node.(*hclsyntax.Block).Labels; len(labels) > 0 { - name = labels[0] - } - - return name, true -} - -// GetStackLabel returns the label of the given node, if it is a stack block -func (s *stackAST) GetStackLabel(node *ast.IndexedNode) (string, bool) { - attr := ast.FindFirstParentMatch(node, ast.IsAttribute) - if attr == nil { - return "", false - } - - stackBlock := ast.FindFirstParentMatch(attr, isStackBlock) - if stackBlock == nil { - return "", false - } - - name := "" - if labels := stackBlock.Node.(*hclsyntax.Block).Labels; len(labels) > 0 { - name = labels[0] - } - - return name, true -} +// GetUnitLabel returns the label of the given node, if it is a unit block +func (s *stackAST) GetUnitLabel(node *ast.IndexedNode) (string, bool) { + return firstLabelFromContainingBlock(node, isUnitBlock) +} + +// GetStackLabel returns the label of the given node, if it is a stack block +func (s *stackAST) GetStackLabel(node *ast.IndexedNode) (string, bool) { + return firstLabelFromContainingBlock(node, isStackBlock) +} + +// firstLabelFromContainingBlock walks up to the containing attribute and then +// the nearest matching block, returning the block's first label. +func firstLabelFromContainingBlock(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool) (string, bool) { + attr := ast.FindFirstParentMatch(node, ast.IsAttribute) + if attr == nil { + return "", false + } + block := ast.FindFirstParentMatch(attr, blockMatcher) + if block == nil { + return "", false + } + return block.Node.(*hclsyntax.Block).Labels[0], true +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/stack/stack.go` around lines 40 - 78, Both GetUnitLabel and GetStackLabel duplicate logic and contain an unreachable safety check: because isUnitBlock/isStackBlock already ensure the block has at least one label, remove the unnecessary name := "" and the len(labels) > 0 guard and read labels[0] directly; then extract the shared logic into a small helper (e.g., getLabelFromParent(node *ast.IndexedNode, parentMatch func(*ast.IndexedNode) bool) (string, bool) or a method on stackAST) that finds the closest attribute parent (ast.IsAttribute), finds the desired parent via the provided matcher (use isUnitBlock or isStackBlock), and returns the first label, and rewrite GetUnitLabel and GetStackLabel to call that helper (also consider reusing the same helper for GetIncludeLabel).
162-180: Usecty.Stringtype check instead ofFriendlyName()comparison.Comparing
e.Val.Type().FriendlyName() == "string"works, but it relies on a human-facing string that could theoretically change. The better approach ise.Val.Type() == cty.String, which is a direct type comparison and won't break if the friendly-name formatting ever shifts. This is the idiomatic pattern used across HashiCorp codebases.Diff
- case *hclsyntax.LiteralValueExpr: - if e.Val.Type().FriendlyName() == "string" { - return e.Val.AsString(), true - } + case *hclsyntax.LiteralValueExpr: + if e.Val.Type() == cty.String { + return e.Val.AsString(), true + } case *hclsyntax.TemplateExpr: // Handle quoted strings which are parsed as TemplateExpr if len(e.Parts) == 1 { if literal, ok := e.Parts[0].(*hclsyntax.LiteralValueExpr); ok { - if literal.Val.Type().FriendlyName() == "string" { + if literal.Val.Type() == cty.String { return literal.Val.AsString(), true } } }You'll need to add
"github.com/zclconf/go-cty/cty"to the imports—it's already a dependency and already imported elsewhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/stack/stack.go` around lines 162 - 180, Replace FriendlyName() string comparisons with direct cty type checks in stackAST.extractStringValue: change checks like e.Val.Type().FriendlyName() == "string" to e.Val.Type() == cty.String in both the LiteralValueExpr and TemplateExpr branches, and add the import "github.com/zclconf/go-cty/cty" if not already present so the cty.String identifier is available.internal/ast/stack/stack_test.go (1)
44-50: These assertions don't actually assert much.A couple of test-hygiene nits:
assert.NotNil(t, stackAST.FindNodeAt)(and siblings): method values on a non-nil interface are always non-nil in Go, so these checks can never fail. If the intent is "the interface is wired up", thenrequire.NotNil(t, stackAST)already covers it. If the intent is "the method works", call it with a realhcl.Posand assert on the result.var _ = stackASTin the "interface compliance" case is a no-op — it doesn't verify interface compliance (Go does that at compile time through the return type ofNewStackAST). A true compile-time interface check would look likevar _ stack.StackAST = (*someConcreteType)(nil), but sinceNewStackASTalready returnsStackAST, it's redundant.Also,
TestStackAST_InterfacecoversGetUnitLabel/GetStackLabel/GetUnitSource/GetUnitPathbut skipsGetStackSource/GetStackPath— worth adding for symmetry.Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ast/stack/stack_test.go` around lines 44 - 50, The test currently asserts method values (stackAST.FindNodeAt, GetUnitLabel, etc.) with assert.NotNil which is meaningless in Go; replace those with a single require.NotNil(t, stackAST) for interface wiring, remove the no-op var _ = stackAST, and instead add real behavioral assertions: call FindNodeAt/FindUnitAt/FindStackAt with a concrete hcl.Pos and assert expected results (or nil) to verify functionality; also add symmetry tests for GetStackSource and GetStackPath similar to GetUnitSource/GetUnitPath; if you want an explicit compile-time interface check add a line like var _ stack.StackAST = (*<concreteType>)(nil) referencing the concrete implementation type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/ast/ast_test.go`:
- Line 287: The test currently uses a tautological predicate
func(*ast.IndexedNode) bool { return false } which prevents exercising
FindFirstParentMatch traversal; change the predicate to a realistic but
unmatched condition (for example, test for a block type not present in the
fixture such as "inputs" or any non-existent block name) so the traversal
actually walks the tree; reference the FindFirstParentMatch call and replace the
predicate with one that inspects the node (e.g., node.Node.Type or a helper like
isIncludeBlock/isDependencyBlock style check) and returns true only for that
absent block type to ensure full traversal.
In `@internal/ast/config/config_test.go`:
- Around line 41-45: The test currently asserts method-value non-nil on
configAST (assert.NotNil(t, configAST.FindNodeAt), GetIncludeLabel,
GetDependencyLabel, GetLocals, GetIncludes) which is meaningless; replace these
with a single require.NotNil(t, configAST) and then call the methods (e.g.,
configAST.FindNodeAt(...), configAST.GetIncludeLabel(),
configAST.GetDependencyLabel(), configAST.GetLocals(), configAST.GetIncludes())
with minimal inputs to verify real behavior or return values, asserting expected
outputs or non-nil results instead of checking the method values themselves.
In `@internal/ast/config/config.go`:
- Around line 45-53: buildIncludesScope currently walks every node in c.Index
which is O(all nodes); change it to iterate only top-level blocks instead. In
function buildIncludesScope, obtain the body via the file AST (e.g. assert
c.HCLFile.Body to *hclsyntax.Body and iterate its Blocks), call
isIncludeBlock(block) for each top-level block and c.includes.Add(block) when
true; if the type assertion to *hclsyntax.Body fails, fall back to the existing
c.Index iteration to preserve behavior.
- Around line 72-114: Both GetIncludeLabel and GetDependencyLabel contain a dead
len(labels) > 0 guard because their matchers (isIncludeBlock /
isDependencyBlock) guarantee a block with at least one label; remove the
redundant check and directly read labels[0], or better yet factor the common
walk-attr-then-block-and-grab-first-label pattern into a small helper (e.g.,
GetFirstLabelFromParentAttribute) and use it from GetIncludeLabel,
GetDependencyLabel and the similar methods in internal/ast/stack/stack.go to
eliminate duplication and clarify intent.
In `@internal/ast/stack/stack_test.go`:
- Around line 44-50: The test currently asserts method values
(stackAST.FindNodeAt, GetUnitLabel, etc.) with assert.NotNil which is
meaningless in Go; replace those with a single require.NotNil(t, stackAST) for
interface wiring, remove the no-op var _ = stackAST, and instead add real
behavioral assertions: call FindNodeAt/FindUnitAt/FindStackAt with a concrete
hcl.Pos and assert expected results (or nil) to verify functionality; also add
symmetry tests for GetStackSource and GetStackPath similar to
GetUnitSource/GetUnitPath; if you want an explicit compile-time interface check
add a line like var _ stack.StackAST = (*<concreteType>)(nil) referencing the
concrete implementation type.
In `@internal/ast/stack/stack.go`:
- Around line 40-78: Both GetUnitLabel and GetStackLabel duplicate logic and
contain an unreachable safety check: because isUnitBlock/isStackBlock already
ensure the block has at least one label, remove the unnecessary name := "" and
the len(labels) > 0 guard and read labels[0] directly; then extract the shared
logic into a small helper (e.g., getLabelFromParent(node *ast.IndexedNode,
parentMatch func(*ast.IndexedNode) bool) (string, bool) or a method on stackAST)
that finds the closest attribute parent (ast.IsAttribute), finds the desired
parent via the provided matcher (use isUnitBlock or isStackBlock), and returns
the first label, and rewrite GetUnitLabel and GetStackLabel to call that helper
(also consider reusing the same helper for GetIncludeLabel).
- Around line 162-180: Replace FriendlyName() string comparisons with direct cty
type checks in stackAST.extractStringValue: change checks like
e.Val.Type().FriendlyName() == "string" to e.Val.Type() == cty.String in both
the LiteralValueExpr and TemplateExpr branches, and add the import
"github.com/zclconf/go-cty/cty" if not already present so the cty.String
identifier is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9314d93d-bf70-4325-9285-afa5b66b2588
📒 Files selected for processing (9)
internal/ast/ast.gointernal/ast/ast_test.gointernal/ast/config/config.gointernal/ast/config/config_test.gointernal/ast/stack/stack.gointernal/ast/stack/stack_test.gointernal/stackutils/stackutils.gointernal/stackutils/stackutils_test.gointernal/tg/definition/definition.go
- Remove dead len(labels) > 0 guards in include/dependency/unit/stack label getters (matchers already guarantee a label) - Extract firstLabelFromContainingBlock helper in ast/stack to DRY GetUnitLabel and GetStackLabel - Use cty.String comparison instead of FriendlyName() == "string" in extractStringValue for idiomatic HCL type checking - Replace tautological predicate in FindFirstParentMatch test with one that actually walks the tree but doesn't match - Remove meaningless assert.NotNil checks on method values in config and stack tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ast/stack/stack.go`:
- Around line 41-65: The helpers require an attribute parent before locating a
block, so passing a block node itself to GetUnitLabel/GetStackLabel fails;
change firstLabelFromContainingBlock to search for the nearest matching block
starting at the node (not via ast.IsAttribute). Replace the attr lookup
(ast.FindFirstParentMatch(node, ast.IsAttribute) ->
ast.FindFirstParentMatch(attr, blockMatcher)) with a direct call that finds the
nearest block matching blockMatcher from node (e.g., block :=
ast.FindFirstParentMatch(node, blockMatcher)), keep the existing nil checks and
return block.Node.(*hclsyntax.Block).Labels[0], true when found so these
functions work when given a block node or any descendant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d0d1279-ca85-4f45-b942-29c0221efe74
📒 Files selected for processing (5)
internal/ast/ast_test.gointernal/ast/config/config.gointernal/ast/config/config_test.gointernal/ast/stack/stack.gointernal/ast/stack/stack_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/ast/stack/stack_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/ast/config/config_test.go
- internal/ast/config/config.go
- internal/ast/ast_test.go
| // GetUnitLabel returns the label of the given node, if it is a unit block | ||
| func (s *stackAST) GetUnitLabel(node *ast.IndexedNode) (string, bool) { | ||
| return firstLabelFromContainingBlock(node, isUnitBlock) | ||
| } | ||
|
|
||
| // GetStackLabel returns the label of the given node, if it is a stack block | ||
| func (s *stackAST) GetStackLabel(node *ast.IndexedNode) (string, bool) { | ||
| return firstLabelFromContainingBlock(node, isStackBlock) | ||
| } | ||
|
|
||
| // firstLabelFromContainingBlock walks up to the containing attribute and then the | ||
| // nearest block matching blockMatcher, returning that block's first label. | ||
| func firstLabelFromContainingBlock(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool) (string, bool) { | ||
| attr := ast.FindFirstParentMatch(node, ast.IsAttribute) | ||
| if attr == nil { | ||
| return "", false | ||
| } | ||
|
|
||
| block := ast.FindFirstParentMatch(attr, blockMatcher) | ||
| if block == nil { | ||
| return "", false | ||
| } | ||
|
|
||
| return block.Node.(*hclsyntax.Block).Labels[0], true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect current StackAST getter usage and confirm whether callers/tests pass block nodes or attribute nodes.
rg -nP -C3 '\b(GetUnitLabel|GetStackLabel|GetUnitSource|GetUnitPath|GetStackSource|GetStackPath|FindUnitAt|FindStackAt)\s*\('Repository: gruntwork-io/terragrunt-ls
Length of output: 5919
🏁 Script executed:
cat -n internal/ast/stack/stack.go | head -150 | tail -100Repository: gruntwork-io/terragrunt-ls
Length of output: 4155
🏁 Script executed:
cat -n internal/ast/stack/stack_test.go | sed -n '140,160p'Repository: gruntwork-io/terragrunt-ls
Length of output: 759
🏁 Script executed:
cat -n internal/ast/stack/stack_test.go | sed -n '270,290p'Repository: gruntwork-io/terragrunt-ls
Length of output: 579
Make the block getters work from the block itself, not just from attributes inside it.
Here's the thing: FindUnitAt and FindStackAt return block nodes, but if you pass those block nodes to GetUnitLabel or GetUnitSource, they return false. Why? Because both helpers currently require an attribute parent first—but blocks don't have attributes as parents; they're the parent of attributes.
The fix is straightforward: walk directly to the matching block instead of requiring an attribute step. This makes these getters work from anywhere—block labels, headers, or any node inside the block—not just from within attributes.
🐛 Proposed fix
func firstLabelFromContainingBlock(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool) (string, bool) {
- attr := ast.FindFirstParentMatch(node, ast.IsAttribute)
- if attr == nil {
- return "", false
- }
-
- block := ast.FindFirstParentMatch(attr, blockMatcher)
+ block := ast.FindFirstParentMatch(node, blockMatcher)
if block == nil {
return "", false
}
return block.Node.(*hclsyntax.Block).Labels[0], true
}
@@
func (s *stackAST) getBlockAttribute(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool, attrName string) (string, bool) {
- // First, try to find the attribute that contains the current node
- attr := ast.FindFirstParentMatch(node, ast.IsAttribute)
- if attr == nil {
- return "", false
- }
-
- // Check if the found attribute has the name we're looking for
- if attrNode, ok := attr.Node.(*hclsyntax.Attribute); ok {
- if attrNode.Name == attrName {
- // Verify we're within the correct block type
- block := ast.FindFirstParentMatch(attr, blockMatcher)
- if block != nil {
- // Extract the string value from the attribute expression
- return s.extractStringValue(attrNode.Expr)
- }
- }
+ block := ast.FindFirstParentMatch(node, blockMatcher)
+ if block == nil {
+ return "", false
}
- return "", false
+ blockNode := block.Node.(*hclsyntax.Block)
+ attrNode, ok := blockNode.Body.Attributes[attrName]
+ if !ok || attrNode == nil {
+ return "", false
+ }
+
+ return s.extractStringValue(attrNode.Expr)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // GetUnitLabel returns the label of the given node, if it is a unit block | |
| func (s *stackAST) GetUnitLabel(node *ast.IndexedNode) (string, bool) { | |
| return firstLabelFromContainingBlock(node, isUnitBlock) | |
| } | |
| // GetStackLabel returns the label of the given node, if it is a stack block | |
| func (s *stackAST) GetStackLabel(node *ast.IndexedNode) (string, bool) { | |
| return firstLabelFromContainingBlock(node, isStackBlock) | |
| } | |
| // firstLabelFromContainingBlock walks up to the containing attribute and then the | |
| // nearest block matching blockMatcher, returning that block's first label. | |
| func firstLabelFromContainingBlock(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool) (string, bool) { | |
| attr := ast.FindFirstParentMatch(node, ast.IsAttribute) | |
| if attr == nil { | |
| return "", false | |
| } | |
| block := ast.FindFirstParentMatch(attr, blockMatcher) | |
| if block == nil { | |
| return "", false | |
| } | |
| return block.Node.(*hclsyntax.Block).Labels[0], true | |
| } | |
| // GetUnitLabel returns the label of the given node, if it is a unit block | |
| func (s *stackAST) GetUnitLabel(node *ast.IndexedNode) (string, bool) { | |
| return firstLabelFromContainingBlock(node, isUnitBlock) | |
| } | |
| // GetStackLabel returns the label of the given node, if it is a stack block | |
| func (s *stackAST) GetStackLabel(node *ast.IndexedNode) (string, bool) { | |
| return firstLabelFromContainingBlock(node, isStackBlock) | |
| } | |
| // firstLabelFromContainingBlock walks up to the containing attribute and then the | |
| // nearest block matching blockMatcher, returning that block's first label. | |
| func firstLabelFromContainingBlock(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool) (string, bool) { | |
| block := ast.FindFirstParentMatch(node, blockMatcher) | |
| if block == nil { | |
| return "", false | |
| } | |
| return block.Node.(*hclsyntax.Block).Labels[0], true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/ast/stack/stack.go` around lines 41 - 65, The helpers require an
attribute parent before locating a block, so passing a block node itself to
GetUnitLabel/GetStackLabel fails; change firstLabelFromContainingBlock to search
for the nearest matching block starting at the node (not via ast.IsAttribute).
Replace the attr lookup (ast.FindFirstParentMatch(node, ast.IsAttribute) ->
ast.FindFirstParentMatch(attr, blockMatcher)) with a direct call that finds the
nearest block matching blockMatcher from node (e.g., block :=
ast.FindFirstParentMatch(node, blockMatcher)), keep the existing nil checks and
return block.Node.(*hclsyntax.Block).Labels[0], true when found so these
functions work when given a block node or any descendant.
Introduce three focused packages ported from PR #55 to support stack/values-file features in subsequent PRs: - internal/ast/config: ConfigAST interface with include/dependency label extraction and include-scope building. - internal/ast/stack: StackAST interface with unit/stack block helpers (FindUnitAt, FindStackAt, source/path getters). - internal/stackutils: LookupUnitPath and LookupStackPath against a parsed *config.StackConfig. Co-authored-by: Diogenes Fernandes <diofeher@gmail.com>
Relocate IsIncludeBlock, IsDependencyBlock, GetNodeIncludeLabel, GetNodeDependencyLabel and the Includes scope out of internal/ast and into internal/ast/config. Include/dependency are config-file concerns and do not belong on the generic IndexedAST. Stack and values files get their own AST packages in follow-ups. - definition.GetDefinitionTargetWithContext now wraps store.AST in astconfig.NewConfigAST and calls GetIncludeLabel/GetDependencyLabel. - Drop corresponding tests from internal/ast/ast_test.go; equivalent coverage lives in internal/ast/config/config_test.go. Co-authored-by: Diogenes Fernandes <diofeher@gmail.com>
- Remove dead len(labels) > 0 guards in include/dependency/unit/stack label getters (matchers already guarantee a label) - Extract firstLabelFromContainingBlock helper in ast/stack to DRY GetUnitLabel and GetStackLabel - Use cty.String comparison instead of FriendlyName() == "string" in extractStringValue for idiomatic HCL type checking - Replace tautological predicate in FindFirstParentMatch test with one that actually walks the tree but doesn't match - Remove meaningless assert.NotNil checks on method values in config and stack tests Co-authored-by: Diogenes Fernandes <diofeher@gmail.com>
b79cda0 to
893b027
Compare
firstLabelFromContainingBlock required an attribute ancestor before
walking up to the matching block, so passing a block node (e.g. one
returned by FindUnitAt/FindStackAt) returned ("", false). Drop the
attribute step and walk directly to the nearest matching block so
the helpers work from the block itself, the label, or any descendant.
Addresses CodeRabbit review on PR #128.
| ) | ||
|
|
||
| // ConfigAST provides methods for working with standard terragrunt.hcl files. | ||
| type ConfigAST interface { |
There was a problem hiding this comment.
Why are we abstracting this away to an interface? Can't we just use the concrete type?
|
|
||
| // ConfigAST provides methods for working with standard terragrunt.hcl files. | ||
| type ConfigAST interface { | ||
| // Core AST methods |
There was a problem hiding this comment.
Avoid leaving comments like this right above the method signatures they document. Hovering over FindNodeAt is going to return Core AST methods. The comment directly above the method signature should document it.
| @@ -0,0 +1,114 @@ | |||
| // Package config provides AST functionality specific to standard terragrunt.hcl files. | |||
| package config | |||
There was a problem hiding this comment.
These are called "units", so I think calling this package "unit" instead of config, which applies to units and stacks is better.
| // isIncludeBlock returns TRUE if the node is an HCL block of type "include". | ||
| func isIncludeBlock(inode *ast.IndexedNode) bool { | ||
| block, ok := inode.Node.(*hclsyntax.Block) | ||
| return ok && block.Type == "include" && len(block.Labels) > 0 |
There was a problem hiding this comment.
Includes can actually have zero labels (for now):
https://docs.terragrunt.com/migrate/bare-include/
| return "", false | ||
| } | ||
|
|
||
| return includeBlock.Node.(*hclsyntax.Block).Labels[0], true |
There was a problem hiding this comment.
As mentioned above, includes can have zero labels, so make sure to handle that here.
| // Test HCL content with include and dependency blocks | ||
| content := ` | ||
| include "root" { | ||
| path = find_in_parent_folders() |
There was a problem hiding this comment.
This is using deprecated functionality. Make sure that's on purpose:
https://docs.terragrunt.com/migrate/migrating-from-root-terragrunt-hcl/
|
|
||
| // Test that locals and includes are captured | ||
| locals := configAST.GetLocals() | ||
| assert.NotNil(t, locals) |
There was a problem hiding this comment.
NIT: These assertions could be stronger, right? We can also assert on the length and values.
| ) | ||
|
|
||
| // StackAST provides methods for working with terragrunt.stack.hcl files. | ||
| type StackAST interface { |
There was a problem hiding this comment.
Same question here about why this is an interface.
| } | ||
|
|
||
| // buildIncludesScope scans the AST to build the includes scope | ||
| func (c *configAST) buildIncludesScope() { |
There was a problem hiding this comment.
NewConfigAST is called on every GetDefinitionTargetWithContext call, so we're going to re-run this every time. Can we not build this during the initial parse and reuse it?
| } | ||
|
|
||
| // FindNodeAt returns the node at the given position in the file | ||
| func (c *configAST) FindNodeAt(pos hcl.Pos) *ast.IndexedNode { |
There was a problem hiding this comment.
This doesn't do anything. *ast.IndexedAST is already embedded in *configAST, so the method is already accessible in the parent type.
| } | ||
|
|
||
| // extractStringValue extracts a string value from various HCL expression types | ||
| func (s *stackAST) extractStringValue(expr hclsyntax.Expression) (string, bool) { |
There was a problem hiding this comment.
Why is this a method? It doesn't use the state from s *stackAST.
| } | ||
|
|
||
| // getBlockAttribute is a helper to get attribute values from blocks | ||
| func (s *stackAST) getBlockAttribute(node *ast.IndexedNode, blockMatcher func(*ast.IndexedNode) bool, attrName string) (string, bool) { |
There was a problem hiding this comment.
Why is this a method? It doesn't use the state from s *stackAST (assuming extractStringValue is no longer a method).
|
|
||
| indexedAST, err := ast.ParseHCLFile("test.hcl", []byte(tt.content)) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, indexedAST) |
There was a problem hiding this comment.
We do this here, then again in the testFuncs, which doesn't actually increase coverage. If this shouldn't ever be nil in any test case, it doesn't make sense to have testFuncs also assert nil-ness.
There was a problem hiding this comment.
This also happens in TestStackAST_Interface, TestConfigAST_Methods and TestStackAST_Methods.
| return "", false | ||
| } | ||
|
|
||
| for _, unit := range stackCfg.Units { |
There was a problem hiding this comment.
NIT: Prefer slices.IndexFunc.
yhakbar
left a comment
yhakbar
left a comment
There was a problem hiding this comment.
I would just like the question answered about the seemingly unnecessary interface addressed before we proceed.
2 participants
Summary
Ports foundation packages from #55 onto current
main(post #89 merge), adapting tomain's unifiedStore+FileTypearchitecture.internal/ast/config:ConfigASTinterface withGetIncludeLabel,GetDependencyLabel,GetLocals,GetIncludes.internal/ast/stack:StackASTinterface withFindUnitAt,FindStackAt, and unit/stack source/path getters.internal/stackutils:LookupUnitPathandLookupStackPathagainst a parsed*config.StackConfig.internal/ast/ast.go: include/dependency helpers and theIncludesscope move out — they're config-file concerns and don't belong on the genericIndexedAST. Stack files get their own AST package.internal/tg/definition/definition.go: now wrapsstore.ASTinastconfig.NewConfigASTand calls through the new interface.No user-facing behavior changes; definition/hover on
terragrunt.hclfiles is identical. Follow-up PRs build stack/values hover + definition + completion on top of these packages.Depends on: none (based on
mainatdf11fce).Blocks: follow-up PRs for stack hover/definition, values hover/definition, and completion refactor.
Test plan
go build ./...go test ./internal/...— all greengolangci-lint run— 0 issuesterragrunt.hcl. Stack/values handlers are added in follow-ups.Summary by CodeRabbit
Refactor
New Features
Tests