Add automatically generated nil-tolerant access functions to the SDK

[ThatJuanGuy]

Currently, autorest.go models use pointers everywhere, risking nil pointer segmentation faults in client codebases. Adding boilerplate nil-check code to access fields safely makes client codebases less readable and maintainable and makes unit test coverage more expensive to achieve.

For instance, consider the following code snippet as a case in point:

var provisioningState string
if resource.Properties != nil && resource.Properties.ProvisioningState != nil {
    provisioningState = *resource.Properties.ProvisioningState
}

To improve code quality, readability and maintainability, it would be great to add automatically generated nil-tolerant accessor functions to the SDK. This would simplify code at point of use and reduce segmentation fault risks.

Today, protobuf has a feature that does exactly this, and it's really useful. See examples from https://github.com/golang/protobuf/blob/5d5e8c018a13017f9d5b8bf4fad64aaa42a87308/internal/testprotos/proto3_proto/test.pb.go#L106 onwards ; generator at https://github.com/golang/protobuf/blob/5d5e8c018a13017f9d5b8bf4fad64aaa42a87308/protoc-gen-go/generator/generator.go#L1793-L1823 .

For example, protobuf would generated accessors like the following:

func (x *Resource) GetProperties() *ResourceProperties {
   if x != nil {
       return x.Properties
   }
   return nil
}
func (x *ResourceProperties) GetProvisioningState() string {
   if x != nil && x.ProvisioningState != nil {
       return *x.ProvisioningState
   }
   return ""
}

These would enable the above snippet to safely become something like:

provisioningState := resource.GetProperties().GetProvisioningState()

Implementing this feature would enhance the developer experience and promote cleaner and safer code practices when working with Go autorest clients and azure-sdk-for-go.

[ThatJuanGuy]

@jhendrixMSFT @jim-minter @serbrech

[jim-minter]

FWIW I made a sketch of what this could look like at jim-minter@accessors ; in particular see jim-minter@accessors#diff-8fca08831d20d6fe1a0efbc2893dc0ffd083101c7f79f150e4d8c606ba63712b . The sketch isn't currently quite right (it doesn't handle dereferencing "enum" strings correctly).

[jim-minter]

Another advantage of this suggestion is that, if implemented and taken to azure-sdk-for-go, all top-level ARM objects will all implement interface { GetID() string /* etc */ }. This would be useful for any code that wants to act on ARM objects generically (we have a few examples of that too).

[jim-minter]

Another angle is that we have had production panics due to log lines like log.Print(*resource.Properties.ProvisioningState) panicing. Using Get() accessors in log lines could be a great way to remove that risk.

[jhendrixMSFT]

@JeffreyRichter and I discussed this early on in the design of the track 2 SDKs. We decided against it for the following reasons (these are the ones I remember, there might have been other reasons).

• The zero-value becomes ambiguous in some cases. Numeric values are the most problematic, strings less so (this is anecdotal and might not actually be true). Maybe we put too much weight on this but, how would consumers know when a zero-value is meaningful? How much consistency is there across services?
• Using the Get prefix is very C#-ish and goes against community expectations (I get that you can't have methods and fields with the same name). We could live with this but have had complaints about our Go SDKs being too C#-ish in the past.

Additionally, fields that have the doc comment // REQUIRED should never be nil (I don't believe we've done a great job documenting this). If you see otherwise, it's a bug in the OpenAPI authoring and should be fixed.

I'll also add that the example func (x *Resource) GetProperties() *ResourceProperties doesn't solve the problem. In order for these helpers to be useful, they must never return nil, but a type's zero-value.

[jhendrixMSFT]

Also, if you're less concerned about the ambiguities of zero-values, you can achieve this today with a generic helper.

func FromPtr[T any](v *T) T {
	if v != nil {
		return *v
	}
	return *new(T)
}

Maybe this belongs in azcore if there's demand for such a construct.

[jim-minter]

I'm dubious that the "too C#-ish" argument holds much water given that Go protobufs already do exactly this 😄 -- and having ARM types implement interface { GetID() string /* etc */ } definitely has uses.

From the logging perspective, I'd rather write (for example) log.Print(resource.GetProperties().GetThing()) and have an unexpected zero value logged than write log.Print(*resource.Properties.Thing) and get an unexpected panic. If in other cases I prefer the "unambiguous value" or potential panic, I can still write *resource.Properties.Thing as I can today, or potentially *resource.GetProperties().Thing (Go protobufs enable all these granular approaches).

Also as nesting increases, resource.GetProperties().GetThing() is much more sane to write than azcore.FromPtr(azcore.FromPtr(azcore.FromPtr(resource).Properties).Thing) and doesn't imply all the shallow copying.

I'll also add that the example func (x *Resource) GetProperties() *ResourceProperties doesn't solve the problem. In order for these helpers to be useful, they must never return nil, but a type's zero-value.

I think there's a key misunderstanding here, because it /does/ solve the problem, and it /does/ return the zero value of the *ResourceProperties (pointer!) type.

See https://go.dev/play/p/OQI1T7uFLIE as an example.

Say you have var resource *Resource (resource is a nil pointer of type *Resource), then resource.GetProperties().GetThing() will safely return the zero value of Thing if it is not fully popualted. Rather than panicing like resource.Properties would, resource.GetProperties() returns a nil pointer of type *ResourceProperties. It is x *Resource that is being nil-checked before dereferencing, not x.Properties.

[jhendrixMSFT]

Agreed on the value of the "too C#-ish" argument. I only bring it up to ensure that we look at solutions through the lense of remaining idiomatic to the language and patterns established throughout the community.

I missed that ResourceProperties.GetThing() performs the nil check of the containing type. So what you propose would work.

[jhendrixMSFT]

If we were to add these, how would a customer know when they need to care about an ambiguous zero-value and need to perform the nil check anyways?

[serbrech]

If we were to add these, how would a customer know when they need to care about an ambiguous zero-value and need to perform the nil check anyways?

If you want to know that the *string is nil, it's your choice and you can check that. the property is not made private, we only add a convenience accessor func.

That convenience accessor func is particularly useful when traversing a deeply nested structure to reach the value of a field.
It is very common in azure datamodel, if only because everything sits under Properties, and then some more structs under that to ensure extensibility of the API.

Name *string
GetName() string

The above makes it clear IMO. The value of Name is a pointer, and as such, can be nil.
if you use GetName(), you get back a string ("") but you can check for nil if you need to. that's up to the dev.
note that you could keep GetName() *string, you would still achieve the traversability. value types are leaf nodes of the contract, so it's not where we get the benefit.

As stated above, this is how protobuf generated code works and it has not caused us any confusion in the years we've been using it.