Skip to content

Combine parsing for all addresses (8/8) - #39355

Merged
jbardin merged 6 commits into
mainfrom
jbardin/target-patterns-H
Oct 9, 2026
Merged

Combine parsing for all addresses (8/8)#39355
jbardin merged 6 commits into
mainfrom
jbardin/target-patterns-H

Conversation

@jbardin

@jbardin jbardin commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Here we get to do the final refactors based on the changes made so far. It is a bit large, so is best review commit-by-commit (we could break it up into multiple PRs, but the review would probably be the same commits anyway)

  • Represent partial-expanded modules as a single module path. This largely mechanical refactor simplifies PartialExpandedModule to a single path, using wildcard keys to handle unexpanded portions. In the process we uncovered and corrected few bugs related to partial expansion handling.
  • Parse every address with one module and resource parser. The addrs package had three parsers for module paths (parseModulePrefix, parseModuleInstancePrefix, and ParsePartialExpandedModule), three for resource addresses (parseConfigResourceUnderModule, parseResourceInstanceUnderModule, and ParsePartialExpandedResource), and two for action instances (parseActionInstanceUnderModule and ParseAbsActionInstance). They had drifted apart: only some of them handled the ephemeral and list modes, the "resource." prefix, or the reserved keyword check, and each handled instance keys differently.
  • Share one parser for address expressions with dynamic keys. This features a new type, AddrExpr which captures a static address expression with dynamic indexes. This is used in a few places, like imports, replace_triggered_by and action triggers, which can all now share the same parser and key access.
  • add addrs.MostSpecific for choosing between matching addresses
  • match move endpoints using the shared address shapes

and finally some cleanup from earlier

  • Make targetShape a sealed interface. A targetShape struct could be simplified as a ModuleInstance, AbsResourceInstance, or AbsActionInstance in which any instance key may be WildcardKey. Those types already treat WildcardKey as matching every instance, so targetShape is now an interface implemented only by them, sealed by an unexported shapeModule method which returns the module path of the address.

@jbardin jbardin added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Oct 7, 2026
@jbardin
jbardin force-pushed the jbardin/target-patterns-G branch from d8d17d8 to dfa5ffa Compare October 7, 2026 17:35
@jbardin
jbardin force-pushed the jbardin/target-patterns-H branch from 769f7f4 to 1a839b7 Compare October 7, 2026 17:38
@jbardin jbardin changed the title Combine parsing for all addresses Combine parsing for all addresses (8/8) Oct 7, 2026
@jbardin
jbardin force-pushed the jbardin/target-patterns-H branch from 75a1703 to 072250f Compare October 8, 2026 14:56
@jbardin
jbardin force-pushed the jbardin/target-patterns-G branch from dfa5ffa to e77b454 Compare October 8, 2026 15:58
jbardin added 6 commits October 8, 2026 12:03
@jbardin
represent partial-expanded modules as a single module path
4223a48 
PartialExpandedModule stored its address in two parts: the expanded
prefix as a ModuleInstance, and the unexpanded suffix as a relative
Module of call names. Each method reassembled or compared those parts
in its own way. It is now stored as a single ModuleInstance, where each
step which is not expanded yet has an instance key of WildcardKey. That
is the same form returned by UnknownModuleInstance and used for the
module path of a targetShape, so converting between them no longer
rebuilds the address.

The matching methods of PartialExpandedModule, PartialExpandedResource,
and PartialExpandedAction are now implemented with targetShape, using
the same containment rules as targeting, limited to addresses at the
same depth. Their behavior is unchanged, which the new tests verify
against the previous implementation

Two bugs fixed in this process:

- ModuleInstance.PartialModule moved every step with a wildcard key to
  the end of the address, so module.a[*].module.b[0] became
  module.b[0].module.a[*]. LoadPlannedDeferrals converts the addresses
  of planned deferrals this way, and ParsePartialResourceInstanceStr
  now accepts [*] in any step, so a plan containing such an address
  would have been loaded incorrectly. The order of the steps is now
  preserved, and the steps after the first wildcard are not expanded.

- PartialExpandedAction.Equal discarded the unexpanded steps of the
  other address before comparing, so a partial-expanded action within
  an unexpanded module was never equal to itself.

newPartialExpandedModule also copies the given address, so a
PartialExpandedModule no longer shares a backing array with the address
it was created from. UnexpandedChild previously retained the caller's
slice, which is what allowed the shared buffer in
partialExpandedModuleInstances to corrupt its results

The unused AbsAction.UnexpandedAction is removed, because it ignored
the receiver's action and would have needed rewriting for the new
representation. The parser tests now compare the String form of the
result, since the fields they compared no longer exist.
@jbardin
parse every address with one module and resource parser
b65d803 
The addrs package had three parsers for module paths (parseModulePrefix,
parseModuleInstancePrefix, and ParsePartialExpandedModule), three for
resource addresses (parseConfigResourceUnderModule,
parseResourceInstanceUnderModule, and ParsePartialExpandedResource),
and two for action instances (parseActionInstanceUnderModule and
ParseAbsActionInstance). They had drifted apart: only some of them
handled the ephemeral and list modes, the "resource." prefix, or the
reserved keyword check, and each handled instance keys differently.

parse_address.go now has a single parser for module paths and one for
resource addresses, which share one parser for instance keys. Each
caller says which instance keys are valid:

- noInstanceKeys for configuration addresses, such as removed blocks
- knownInstanceKeys for concrete addresses
- wildcardInstanceKeys for patterns and partial-expanded addresses,
  where [*] and unknown index values are both parsed as WildcardKey

The other parse functions are now thin wrappers around these, and
ParseAbsActionInstance uses the same parser as action targets.

Wildcards and unknown keys are reported clearly where they aren't
allowed, rather than leaving a [*] in the remainder of the traversal to
fail with an unrelated error, or panicking on an unknown index value.

Since removed blocks now use the shared resource parser, they accept
the optional "resource." prefix, report reserved keywords, and reject
ephemeral and list resources with an explicit error, where these were
previously misparsed as a managed resource type named "ephemeral" or
"list". ParsePartialExpandedResource also gains the ephemeral and list
modes, though configuration only allows managed resources as import
targets.
@jbardin
share one parser for address expressions with dynamic keys
4552f1b 
The "to" argument of an import block and the "from" argument of a
stacks removed block are expressions referring to an address, where
instance keys may be expressions such as each.key rather than
constants. Three functions walked these expressions separately:
importToExprToTraversal evaluated the keys while planning imports,
exprToResourceTraversal discarded them to find the configuration
address of an import, and the stacks exprToComponentTraversal kept them
in a linked list of partial traversals for evaluating later.

addrs.ParseAddressExpr now flattens these expressions into an
AddressExpr, which is the steps of the traversal with each non-constant
key kept as its expression. AddressExpr.Traversal resolves the keys
with a function provided by the caller, or as wildcards if there isn't
one, which the parsers from the previous commit already accept:

- Planning an import evaluates the keys with each.key and each.value.
  When for_each is unknown, the unknown keys are parsed as wildcards by
  ParsePartialResourceInstance, so ParsePartialExpandedResource and
  ParsePartialExpandedModule are no longer used, and are removed.

- Decoding an import block parses the keys as wildcards to find the
  configuration address, rather than discarding them.

- The stacks ParseRemovedFrom walks the steps of the AddressExpr,
  keeping the key expressions to evaluate later, which replaces the
  linked list and the special cases for the length of each traversal.
@jbardin
add addrs.MostSpecific for choosing between matching addresses
8be94b0 
Placeholder values for partial-expanded modules and test overrides both
choose the most specific of several matching addresses, but did so in
different ways. namedvals compared the number of known module instance
keys of each placeholder, while the mock overrides compared targets by
containment.

addrs.MostSpecific chooses the candidate contained by all of the others,
using a containment function given by the caller, with earlier
candidates taking precedence where they aren't nested.The placeholders
that include an address are always nested within each other, so the most
specific is the one with the most known keys, as before.
ModuleLevelsKnown existed only for the previous comparison and is
removed.
@jbardin
match move endpoints using the shared address shapes
b207851 
MoveEndpointInModule had its own wildcard instance key, anyKey, to
build a synthetic module path in which the module where the endpoint
was declared, and a module call, select every instance.
moduleInstanceCanMatch then compared these paths with wildcards on
either side, which is the same comparison as targetShape.couldContain,
and SelectsModule repeated it with wildcards on one side, which is
targetShape.contains.

MoveEndpointInModule.targetShape now builds the shape of the objects
which an endpoint selects, with WildcardKey for the declaring module
and module calls, and SelectsModule, SelectsResource, CanChainFrom, and
NestedWithin are implemented with it. The rules specific to moves are
kept explicit: a whole resource only chains with a whole resource and
only contains its instances, and a module call contains its instances,
while any other nested module must have a longer path. The endpoints
themselves remain concrete addresses, since a move needs exactly one
destination for each object it moves.

anyKey, synthModuleInstance, and moduleInstanceCanMatch are removed, so
WildcardKey is the only wildcard instance key.
@jbardin
make targetShape a sealed interface
cda58d3 
A targetShape with a module path and a leaf was the same thing as a
ModuleInstance, AbsResourceInstance, or AbsActionInstance in which any
instance key may be WildcardKey. Those types already treat WildcardKey
as matching every instance, so targetShape is now an interface
implemented only by them, sealed by an unexported shapeModule method
which returns the module path of the address.
@jbardin
jbardin force-pushed the jbardin/target-patterns-H branch from 072250f to cda58d3 Compare October 8, 2026 16:04
@jbardin
jbardin marked this pull request as ready for review October 8, 2026 16:55
@jbardin
jbardin requested a review from a team as a code owner October 8, 2026 16:55
Base automatically changed from jbardin/target-patterns-G to main October 9, 2026 14:35
mildwonkey
mildwonkey reviewed Oct 9, 2026
View reviewed changes
Comment thread internal/addrs/parse_address.go
// The remainder of the traversal is returned for the caller to interpret as
// an object within the module. It always starts with a TraverseRoot if it's
// not empty.
func parseModuleInstancePrefix(traversal hcl.Traversal, keys instanceKeys) (ModuleInstance, hcl.Traversal, tfdiags.Diagnostics) {

@mildwonkey mildwonkey Oct 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like what you did with the instanceKeys here, I find it much clearer (especially when looking at a caller)

mildwonkey
mildwonkey approved these changes Oct 9, 2026
View reviewed changes

@mildwonkey mildwonkey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like these changes overall - the new methods feel logical and you've very nicely streamlined some previously-confusing method chains. Great test and comment updates too (as is your usual)! ⭐

@jbardin
jbardin merged commit d8e73fc into main Oct 9, 2026
17 checks passed
@jbardin
jbardin deleted the jbardin/target-patterns-H branch October 9, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mildwonkey mildwonkey mildwonkey approved these changes

Assignees

No one assigned

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jbardin @mildwonkey