Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[codegen/go] Fix accessors on struct ptr outputs #4456

Merged
merged 4 commits into from
Apr 21, 2020

Conversation

lukehoban
Copy link
Member

@lukehoban lukehoban commented Apr 21, 2020

The accesor methods on nestred struct Ptr outputs were previously not accepting pointer typed inputs to their applier callbacks as they should, and would thus always panic if used.

The "simple" fix would be to just accept the pointer type and blindly dereference it. But this doesn't seem like the right experience - it would make these accessors very unsafe to use in practice.

Instead, this PR implements the accessors on pointer-typed outputs as nil-coaslescing, always lifting the output type into a pointer type and flowing a nil value into the result type. This ensures the accessor will not nil-deref, and that user code can handle the nil value itself (or use .Apply directly to implement more specialized behaviour).

Before:

// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringOutput {
	return o.ApplyT(func(v BuildStorageLocation) string { return v.Bucket }).(pulumi.StringOutput)
}

After:

// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringPtrOutput {
	return o.ApplyT(func(v *BuildStorageLocation) *string {
		if v == nil {
			return nil
		}
		return &v.Bucket
	}).(pulumi.StringPtrOutput)
}

However, due to the decision to have this more usable behaviour, this is a breaking change, as some/many accessors now return a pointer type when they previously did not.

Fixes pulumi/pulumi-azure#530.

The accesor methods on nestred struct Ptr outputs were previously not accepting pointer typed inputs as they should, and would thus always panic if used.

The "simple" fix would be to just accept the pointer type and blindly dereference it.  But this doesn't seem like the right experience - it would make these accessors very unsafe to use in practice.

Instead, this PR implements the accessors on pointer-typed outputs as nil-coaslescing, always lifting the output type into a pointer type and flowing a nil value into the result type.  This ensures the accessor will not nil-deref, and that user code can handle the `nil` value itself (or use `.Apply` directly to implement more specialized behaviour).

Before:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringOutput {
	return o.ApplyT(func(v BuildStorageLocation) string { return v.Bucket }).(pulumi.StringOutput)
}
```

After:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringPtrOutput {
	return o.ApplyT(func(v *BuildStorageLocation) *string {
		if v == nil {
			return nil
		}
		return &v.Bucket
	}).(pulumi.StringPtrOutput)
}
```

However, due to the decision to have this more usable behaviour, this is a breaking change, as some/many accessors now return a pointer type when they previously did not.

Fixes pulumi/pulumi-azure#530.
@lukehoban lukehoban added the impact/breaking Fixing this issue will require a breaking change label Apr 21, 2020
Copy link
Contributor

@EvanBoyle EvanBoyle left a comment

Choose a reason for hiding this comment

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

Quick fix! 🚢

@elsesiy
Copy link
Contributor

elsesiy commented Apr 21, 2020

Thanks @lukehoban, this must be a new record 🥇

@lukehoban
Copy link
Member Author

However, due to the decision to have this more usable behaviour, this is a breaking change, as some/many accessors now return a pointer type when they previously did not.

Notably though, the existing methods were guaranteed to panic if called, so I don’t think this is actually a breaking change in practice.

@lukehoban lukehoban removed the impact/breaking Fixing this issue will require a breaking change label Apr 21, 2020
pkg/codegen/go/gen.go Show resolved Hide resolved
pkg/codegen/go/gen.go Outdated Show resolved Hide resolved
@lukehoban
Copy link
Member Author

Here's an example of codegen differences in AWS.

https://github.com/pulumi/pulumi-aws/compare/lukehoban/goaccessor#diff-1f1b5ecc107eb8868597adfb597e7e16

Manually confirmed that this builds cleanly there.

@lukehoban lukehoban merged commit 6afefe0 into master Apr 21, 2020
@pulumi-bot pulumi-bot deleted the lukehoban/goaccessorptr branch April 21, 2020 20:33
komalali pushed a commit that referenced this pull request Apr 23, 2020
* [codegen/go] Fix accessors on struct ptr outputs

The accesor methods on nestred struct Ptr outputs were previously not accepting pointer typed inputs as they should, and would thus always panic if used.

The "simple" fix would be to just accept the pointer type and blindly dereference it.  But this doesn't seem like the right experience - it would make these accessors very unsafe to use in practice.

Instead, this PR implements the accessors on pointer-typed outputs as nil-coaslescing, always lifting the output type into a pointer type and flowing a nil value into the result type.  This ensures the accessor will not nil-deref, and that user code can handle the `nil` value itself (or use `.Apply` directly to implement more specialized behaviour).

Before:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringOutput {
	return o.ApplyT(func(v BuildStorageLocation) string { return v.Bucket }).(pulumi.StringOutput)
}
```

After:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringPtrOutput {
	return o.ApplyT(func(v *BuildStorageLocation) *string {
		if v == nil {
			return nil
		}
		return &v.Bucket
	}).(pulumi.StringPtrOutput)
}
```

However, due to the decision to have this more usable behaviour, this is a breaking change, as some/many accessors now return a pointer type when they previously did not.

Fixes pulumi/pulumi-azure#530.

* Mark nested property types as requiring ptr types

* Add CHANGELOG

* More fixes
komalali pushed a commit that referenced this pull request Apr 23, 2020
* [codegen/go] Fix accessors on struct ptr outputs

The accesor methods on nestred struct Ptr outputs were previously not accepting pointer typed inputs as they should, and would thus always panic if used.

The "simple" fix would be to just accept the pointer type and blindly dereference it.  But this doesn't seem like the right experience - it would make these accessors very unsafe to use in practice.

Instead, this PR implements the accessors on pointer-typed outputs as nil-coaslescing, always lifting the output type into a pointer type and flowing a nil value into the result type.  This ensures the accessor will not nil-deref, and that user code can handle the `nil` value itself (or use `.Apply` directly to implement more specialized behaviour).

Before:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringOutput {
	return o.ApplyT(func(v BuildStorageLocation) string { return v.Bucket }).(pulumi.StringOutput)
}
```

After:

```go
// Name of your S3 bucket.
func (o BuildStorageLocationPtrOutput) Bucket() pulumi.StringPtrOutput {
	return o.ApplyT(func(v *BuildStorageLocation) *string {
		if v == nil {
			return nil
		}
		return &v.Bucket
	}).(pulumi.StringPtrOutput)
}
```

However, due to the decision to have this more usable behaviour, this is a breaking change, as some/many accessors now return a pointer type when they previously did not.

Fixes pulumi/pulumi-azure#530.

* Mark nested property types as requiring ptr types

* Add CHANGELOG

* More fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SystemAssigned Identity for Cluster can't be retrieved via stack output
4 participants