Fix transform property value to preserve nil arrays and objects

[VenelinMartinov]

The propertyvalue.Transform functions had a bug where it would always mutate nil arrays and maps into empty ones:

value=resource.PropertyValue{}

would become:

value=resource.PropertyValue{
     V:  []resource.PropertyValue{
     },
 }

This had to do with typed nil interfaces in go: https://dave.cheney.net/2017/08/09/typed-nils-in-go-2
The V in resource.PropertyValue is an interface, so it might be non-nil != nil but the underlying value is nil.

This change adds a fix and some tests for the problem.

[VenelinMartinov]

This change is part of the following stack:

• Fix transform property value to preserve nil arrays and objects #2655 ◀
  • Add property value transform for removing secrets and outputs #2656
    • Fix PF provider_server detailed diff handling #2628
      • PF detailed diff #2629
        • Add PF provider options and accurate previews flag #2663
          • Detailed diff handle secrets and outputs #2643
            • Add detailed diff recordings to PF diff tests #2657

_{Change managed by git-spice.}

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.98%. Comparing base (b1f971c) to head (6705050).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
unstable/propertyvalue/propertyvalue.go	75.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2655   +/-   ##
=======================================
  Coverage   69.97%   69.98%           
=======================================
  Files         367      367           
  Lines       44504    44512    +8     
=======================================
+ Hits        31142    31150    +8     
  Misses      11735    11735           
  Partials     1627     1627           

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[t0yv0]

This is wild. What is value=resource.PropertyValue{} ? It represents nil? A great catch. I thought I even have fuzz testing on this but missed this.

[t0yv0]

I am wondering now, why are these things valid values in Pulumi?

Could we assert against their existence or normalize them to a normal semantic form?

That is, I understand that in TF type array may have a distinct nil value, but it is surprising to me that in either Pulumi or TF space we would want to make a distinction between nil and a special nil array. I would expect that at this layer we want to have only one nil.

So whichever code constructed this value to represent a nil:

resource.PropertyValue{V: []resource.PropertyValue(nil)}

Should have constructed this value instead which canonically represents nil:

resource.PropertyValue{}

?

[VenelinMartinov]

Yeah, we shouldn't be making these in the first place. I've opened #2662 to follow up here.

However, I still think that Transform shouldn't be changing values as well, so I think this PR is still valid. Here it changes a nil to empty which are semantically different.

[t0yv0]

Yeah I'm ok with the follow up.

[iwahbe]

Do we have evidence of this happening in the wild?

I'm pretty sure we have rapid generators for resource.PropertyValue. Can we write a test that asserts TransformPropertyValue(path, identity, value) == (value, nil)?

[VenelinMartinov]

Do we have evidence of this happening in the wild?

Yeah we do get nil arrays out of MakeTerraformOutput. This is how I caught the issue.

I am not convinced that this is a supported value for resource.PropertyValues. I think the bridge is doing the wrong thing here, so Anton is correct that it should be returning a non-array nil.

I'll open a follow-up to chase down what is constructing this value but it might have other effects, so might not be trivial to change. Opened #2662

[t0yv0]

Accepting as workaround but probably need to tidy this up with contract.Assertf once the root cause is fixed.

[pulumi-bot]

This PR has been shipped in release v3.97.0.