Go optional scalars

[chriscraws]

Hi, I'm taking a first pass at adding support for optional scalars to Go.

#6471
#6014

I haven't authored tests yet, but will do so if this approach seems reasonable!

Essentially the changes are the following:

• Non-optional types are now always written
• Optional scalars are generated as *type instead of type for the object-api and the normal functional API.
• Mutators for optional scalars are the same as they were before, and just return false if the field is not present (they don't accept a pointer)

Example getter:

func (rcv *ScalarStuff) MaybeI8() *int8 {
	o := flatbuffers.UOffsetT(rcv._tab.Offset(6))
	if o != 0 {
		return rcv._tab.GetInt8(o + rcv._tab.Pos)
	}
	return nil
}

Example setter

func ScalarStuffAddMaybeI8(builder *flatbuffers.Builder, maybeI8 *int8) {
	if maybeI8 != nil {
		builder.PrependInt8(*maybeI8)
		builder.Slot(1)
	}
}

Example setter for non-optional type:

func ScalarStuffAddDefaultF64(builder *flatbuffers.Builder, defaultF64 float64) {
	builder.PrependFloat64(defaultF64)
	builder.Slot(29)
}

[CasperN]

Can you generate code for optional_scalars.fbs?

[chriscraws]

Done - not sure why I left that out of the commit

[CasperN]

I'm not a Go person, but the generated code generally looks good to me

Non-optional types are now always written

This doesn't align with the other languages. We don't put scalars in the binary if the user doesn't set the field.
Even if they do set the field, if they set it to the default value, we don't put it in the binary, unless
the user opt-ed into 'force defaults' (which is default off)

[chriscraws]

Non-optional types are now always written

This doesn't align with the other languages. We don't put scalars in the binary if the user doesn't set the field. Even if they do set the field, if they set it to the default value, we don't put it in the binary, unless the user opt-ed into 'force defaults' (which is default off)

That mainly only applies for the object-api - all fields still need explicit encoding using the imperative API, but with this change values are stored regardless of whether they are the default. That's what I interpreted from these statements from the feature proposal:

Instead of omitting zero-like values, the generated code must store them.
It aligns with the "force_defaults" + "IsFieldPresent" hacky manual approximation of this feature.

Sounds like I've misinterpreted though, so to be clear - the recommended approach is to leave 'non-optional' fields as-is (store only when the value is not the default or the implicit zero-default) and for fields that must always be written but may take any value, they need to be marked null in the schema, and I'll need to specify defaults in my code, migrating scalar fields to be optional instead.

[CasperN]

the recommended approach is to leave 'non-optional' fields as-is (store only when the value is not the default or the implicit zero-default)

Yes, that is correct.

and for fields that must always be written but may take any value, they need to be marked null in the schema, and I'll need to specify defaults in my code, migrating scalar fields to be optional instead.

Yep, that's the best way.

---

Instead of omitting zero-like values, the generated code must store them.

It aligns with the "force_defaults" + "IsFieldPresent" hacky manual approximation of this feature.

Yeah that I meant was:

• in the schema table Foo { a: int = null } calling FooAddA(b, 0) will store the zero in the binary.
  This is unlike calling BarAddA(b, 0) for table Bar { a: int }, which would not store anything.

• Some users who cared about the distinction between "A=0 in the binary" and "A is not in the binary" would use ForceDefaults and an IsFieldPresent to detect whether BarAddA(b, 0) was called. This was generally considered to be suboptimal

[chriscraws]

I see! Makes sense, thanks for the clarification - I'll update this PR to have the normal behavior for scalars and will add tests.

[chriscraws]

Thanks for looking at this - I've added tests

[CasperN]

tests LGTM

Pinging @rw, who started the go stuff

[CasperN]

@dbaileychess were you working on windows CI recently? FYI they seem to be broken, unrelated to this PR

[dbaileychess]

Hmm, nothing that extreme. Some dependency in our CI must have changed. Let me take a look.

[dbaileychess]

Attempting fix of CI in #7105

[dbaileychess]

Seems to be fixed. I restarted the checks here.

[dbaileychess]

Hmm, maybe a force push is required, It is still using the old CI workflow.

[CasperN]

Ok, seems all checks pass and the code LGTM (with the caveat that I'm not a Go person). @chriscraws, are we good to merge?

[chriscraws]

SGTM, thanks for reviewing