Add a TransformPropertyValueDirectional property value walker

[VenelinMartinov]

This PR adds a new property value walker function to the propertyvalue module: TransformPropertyValueDirectional.

This is different to TransformPropertyValue in two ways:

• it visits nodes both on the way up and on the way down
• the transform function can return a SkipChildrenError to prevent the recursion from walking the children of the value currently being visited.

This is necessary for #2761 to handle set value comparisons, as we need to be able to recursively walk a property value without recursing into nested sets (as nested sets can be reordered during planning).

[VenelinMartinov]

This change is part of the following stack:

• Add a TransformPropertyValueDirectional property value walker #2760 ◀
  • Use non-computed property value comparison for matching detailed diff set inputs to plan #2761

_{Change managed by git-spice.}

[codecov]

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.61%. Comparing base (c4215af) to head (5627094).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
unstable/propertyvalue/propertyvalue.go	85.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2760   +/-   ##
=======================================
  Coverage   68.60%   68.61%           
=======================================
  Files         322      322           
  Lines       41223    41238   +15     
=======================================
+ Hits        28281    28294   +13     
- Misses      11347    11351    +4     
+ Partials     1595     1593    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iwahbe]

This function conflates a bunch of stuff. Traversal order is not related to supporting a LimitDescentError. The idiomatic Go API for tree walking looks like this:

var SkipChildren = skipChildren{}

type Transformer = func(
	resource.PropertyPath, resource.PropertyValue, entering bool,
) (resource.PropertyValue, error)

func TransformPropertyValue(
	path resource.PropertyPath,
	transformer Transformer,
	value resource.PropertyValue,
) (resource.PropertyValue, error)

We can then use TransformPropertyValue to construct all the functions we need:

func TransformPropertyValuePreorder(
	path resource.PropertyPath,
	transformer func(
		resource.PropertyPath, resource.PropertyValue,
	) (resource.PropertyValue, error),
	value resource.PropertyValue,
) (resource.PropertyValue, error) {
	return TransformPropertyValue(path, func(
		path resource.PropertyPath, value resource.PropertyValue, entering bool,
	) (resource.PropertyValue, error) {
		if entering { return transformer(path, value) } 
		return value, nil 
	}, value)
}

Can we keep to a general function to handle the traversal?

[VenelinMartinov]

Traversal order is not related to supporting a LimitDescentError

Not sure I understand what you mean here - if you visit children before their parents then a visitor doesn't get a chance to return a LimitDescentError.

The API you suggested is cleaner and I have indeed seen the pattern with entering elsewhere, so happy to refactor it like that.

[VenelinMartinov]

Refactored the module and kept just one traversal function. LMK what you think!

[t0yv0]

Yeah I recall seeing entering somewhere but I can't recall where. This latest version is OK-ish to reason about. The boolean needs to be called entering otherwise the API causes boolean blindness.

Do you have refs to Go libs where you've seen this?

I'd suspect idiomatic Go would be further reliant on mutation :) But there's e.g. https://github.com/hashicorp/go-cty/blob/v1.4.1-0.20200414143053-d3edf31b6320/cty/walk.go#L87 that does not mutate.

[t0yv0]

I've just found https://github.com/fogfish/golem for light weekend reading, this is going to be fun but we probably don't want to go too far down that route.

[VenelinMartinov]

https://github.com/fogfish/golem this looks pretty fun indeed

[VenelinMartinov]

pulumi-terraform-bridge/pkg/tfgen/parse/extension.go

Line 118 in f02a167

err := ast.Walk(node, func(node ast.Node, entering bool) (ast.WalkStatus, error) {

The Markdown parsing library we use uses the entering pattern for its recursion.

[t0yv0]

This is generally a bad idea I think.

Once you have variations on the recursion pattern they get difficult to explain, and it is much easier to maintain an actually direct recursive function than to share a recursion pattern. And when debugging you need to step through the concrete function and the general recursion function that is not very intuitive, this is not great.

I'd inline this.

If inlining introduces too much repetitive code into the call site, I'd try one pattern I have seen work well that looks like this. In the library you have an API that looks like this:

module propertyvalue
type Tag // opaque
Unpack(PropertyValue) (Tag, []PropertyValue)
Pack(Tag, []PropertyValue) PropertyValue

This API lets you see the children of a PropertyValue, and then substitute them and get a similar-ish PropertyValue with different children. Note that this is a functional API (I'm borrowing from something I've seen in F# and Haskell), perhaps an imperative walker can be simpler but to be safe it'd need a deep clone function as well.

Continuing with this, if you have a complex transformation that's recursive, you write it like this:

func mytr(p PropettyValue) PropertyValue {
    // handle concrete cases you care about
    if pnext, ok := tr(p); ok {
       return p
    }
    // now recur for cases you don't care about, e.g. Secret, Output, etc, etc.
    tag, children := pv.Unpack(p)

    var trchildren []PropertyValue
    for _, c := range children {
        trchildren = append(trchildren, mytr(c))
    }
    return pv.Pack(tag, trchildren)
}

If you want to modify the recursion pattern like walking up or down or special casing, you then work within mytr function and don't need any more library edits.

[t0yv0]

https://dl.acm.org/doi/10.1145/604174.604179 is a reference here. Realized in https://hackage.haskell.org/package/uniplate for example.

[t0yv0]

https://hackage.haskell.org/package/uniplate-1.6.13/docs/Data-Generics-Uniplate-Operations.html in particular

[t0yv0]

Name the fields here.

[t0yv0]

The latest version with entering is a bit complex but OK to reason about, I think I've seen such APIs in the wild, there is precedent. I'll let Ian review.

[iwahbe]

I agree with @t0yv0's comment, otherwise LGTM.

[iwahbe]

This needs a comment explaining what it does.

[iwahbe]

I'd rather this be called TransformPropertyValue, but we can change that in a subsequent PR.

[VenelinMartinov]

superseded by #2780 as that matches the recursion pattern necessary in #2761 much better