feat(spec): preserve referenced macros during removal

[Thien Trung Vuong (trungams)]

Summary

Removing a section or subpackage can also remove %define or %global declarations that happen to live inside it. When surviving content still references one of those macros, the rendered spec is left with a dangling reference and often fails much later during the build.

This PR teaches the structural editor to preserve the complete declarations that surviving content still needs. It follows transitive dependencies and keeps declaration ordering intact. When a relocation is ambiguous or clearly unsafe, the removal fails without modifying the spec.

Motivation

Issue #203 documents this with QEMU: %define testsdir lives in the tests subpackage, but %install continues to use it after that subpackage is removed. The macro declaration must survive even though the package sections do not.

Changes

• Select removed macro declarations referenced by surviving content.
• Follow transitive dependencies at the relevant use order.
• Preserve complete multiline declarations and original declaration order.
• Scan section-header arguments while excluding the section marker itself.
• Avoid duplicate hoists when a safe surviving definition already supplies the binding.
• Log automatic macro preservation.
• Reject unsafe cases with ErrUnsafeMacroHoist without modifying the spec, including:
  • ambiguous conditional declarations;
  • unsafe eager %global dependencies or redefinitions;
  • selected dynamic macro names;
  • self-referential selected %global declarations;
  • incompatible same-name bindings.
• Add QEMU-shaped spec-remove-subpackage drops %define macros inside the removed subpackage that are referenced elsewhere #203 coverage and focused rejection tests.

Validation

• mage build
• mage unit

The completed structural editor also rendered the full Azure Linux evaluation corpus without parser or overlay errors, including automatic preservation of QEMU's testsdir macro.

Known limitations

This is conservative structural and lexical analysis, not RPM macro evaluation. If azldev cannot establish a safe binding, it rejects the removal instead of guessing. Conditional same-name bindings that would require evaluating the condition remain best-effort.

[Copilot]

🟡 Changes recommended

Unresolved integration and macro-analysis correctness issues remain.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds structural RPM macro hoisting when sections or subpackages are removed, preserving referenced declarations and rejecting unsafe relocations.

Changes:

• Tracks transitive macro dependencies and preserves declaration order.
• Rejects ambiguous or unsafe hoists without modifying the spec.
• Adds focused tests and regression fixtures.

File summaries

File	Summary
internal/rpm/spec/tree_hoist.go	Implements macro analysis, validation, and hoisting.
internal/rpm/spec/tree_hoist_internal_test.go	Adds focused hoisting and rejection tests.
internal/rpm/spec/testdata/specs/subpackage-define-transitive.spec	Covers transitive macro preservation.
internal/rpm/spec/testdata/specs/subpackage-define-shadowed.spec	Covers shadowed definitions.
internal/rpm/spec/testdata/specs/subpackage-define-referenced.spec	Covers direct macro references.
internal/rpm/spec/testdata_test.go	Adds fixture-based integration coverage.
internal/rpm/spec/structural_tree_api.go	Integrates hoisting with section removal.

Review details

Suppressed comments (7)

internal/rpm/spec/structural_tree_api.go:188

• This changes the user-facing behavior of both spec-remove-section and spec-remove-subpackage, but docs/user/reference/config/overlays.md still only documents section deletion. Please document that referenced macro declarations may be hoisted and that conservative ErrUnsafeMacroHoist cases abort without changing the spec.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/structural_tree_api.go:188

• An empty handle slice now reaches hoistReferencedMacros, where no removed root ancestor exists and ErrUnsafeMacroHoist is returned. Before this hook, validateSectionRemoval followed by removeSections made RemoveSections(nil) a no-op; retain that behavior for callers that build a selection dynamically and can have no matches.

	if err := t.hoistReferencedMacros(sections); err != nil {
		return err
	}

internal/rpm/spec/tree_hoist.go:466

• Dynamic invocations are only checked against removed declaration names here; this never resolves a dynamic target to a surviving lazy definition and traverses that definition's dependencies. For example, with surviving %define suffix name and %define rootname %{dep}, a removed %define dep value, and echo %{root%{suffix}}, the dynamic target is rootname so dep is never selected and removal leaves a dangling %{dep} when rootname expands. Resolve or conservatively reject matching surviving definitions at the invocation order.

	for _, reference := range facts.references {
		visit(effectiveMacroDefinition(facts.all[reference.name], reference.order), reference.order)
	}

internal/rpm/spec/tree_hoist.go:616

• This treats macro-looking text in comments as a live reference. RPM does not expand a # comment, so a removed parameterized or conditional declaration plus a surviving line such as # %{helper} will select that declaration and then fail with ErrUnsafeMacroHoist, even though no surviving content needs it. Skip comment-only lines before collecting references.

				if !removed {
					addReferences(line, order)
				}

internal/rpm/spec/tree_hoist.go:361

• A selected %global can depend on a selected lazy %define, but this early return prevents validation from traversing that dependency. If %define base %{dep} is removed, %global root %{base} is later in the removed sections, and surviving %define dep new lies between the first removed section and root, the original eager root sees new while the hoisted root expands before it and sees a different binding; this shape is currently accepted and silently changes the spec. Validate the complete eager dependency closure at both the original and insertion orders, or reject this case.

		if definition == nil || definition.global {
			return nil

internal/rpm/spec/tree_hoist.go:532

• This rejects every surviving dependency of a selected %global, even when that dependency is already defined before the hoist insertion point. For example, a preamble %define dep /usr/lib followed by a removed %global root %{dep} and a surviving echo %{root} is safe—the hoisted global still follows dep—but this returns ErrUnsafeMacroHoist. Compare the candidate's order with insertionOrder instead of treating all surviving candidates as unsafe.

			if !candidate.removed || candidate.order >= definition.order {
				return fmt.Errorf("cannot safely hoist eager '%%global' macro %#q before dependency %#q:\n%w",
					definition.block.Name, dependency, ErrUnsafeMacroHoist)
			}

internal/rpm/spec/tree_hoist.go:225

• facts.undefined records only whether a name was ever undefined, so any %undefine causes a selected declaration to be rejected. A preamble %undefine foo followed by a removed %define foo value is safe: the hoisted definition remains after the preamble and still follows the undefine, but this returns ErrUnsafeMacroHoist. Track directive order/context and reject only when the relocation crosses a relevant %undefine.

		if facts.undefined[definition.block.Name] {
			return fmt.Errorf("cannot safely hoist macro %#q across '%%undefine':\n%w",
				definition.block.Name, ErrUnsafeMacroHoist)
		}

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.