Omit setting grpc fields to zero defaults

[MichaelUrman]

The expected behavior for zero-valued defaults bothered me; it needlessly compared the proto message fields against zeros only to set the goa fields to zeros. This PR explicitly tests a variety of built-in types and avoids the compare and set.

It's possible the isNonZero would be better written with reflect, to reduce its code. Or moved to, say, expr.AttributeType, so that we could instead write something like this in protobuf_transform:

- if tdef := tgtc.DefaultValue; tdef != nil && ta.TargetCtx.UseDefault && isNonZero(tdef) {
+ if tgtc.HasNonzeroDefault() && ta.TargetCtx.UseDefault {
+ 	tdef := tgtc.DefaultValue

[MichaelUrman]

I was slightly surprised to learn the following code issues an error. Should I file a bug report, and/or attempt to address it?

    		Field(14, "typed", Int64, func() {
			Meta("struct:field:type", "time.Duration", "time")
			Default(0 * time.Second)
		})

testing.go:18: [testdata/dsls.go:932] default value 0 is incompatible with attribute of type int64 in attribute

Namely it appears that a default of time.Duration(0) is not accepted, despite the field type being overridden to time.Duration. (In the meantime, it seems that isNonZero doesn't have to verify underlying types.)

[MichaelUrman]

While writing this I had several misgivings over the new-ish defaults behavior in gRPC at all. On the one hand, it means that setting a nonzero default makes it impossible to send a zero value over gRPC. On the other hand, without it a nonzero default silently does something you may not expect. I don't have a good resolution to offer here, but suspect we should err towards allowing it, and just clarify the documentation for dsl.Default(). If there's enough context to prove an unexpected interaction, perhaps issue a warning, or create a linter for this.

I'm assuming any of that would be better off in a separate issue or PR. Let me know if you think it should be here.

[raphael]

The PR looks good - thank you. Could you please rebase on the latest v3?

As you probably know the challenge with gRPC is that the encoding doesn't support the notion of "missing" vs "has default value" since primitive values used in a gRPC message cannot be nil. We had a proposal to tackle this explicitly: #2221 - however this never got implemented. The proposal allows designers to define the desired behavior (i.e., allow or prevent a primitive field to have a zero value).

[MichaelUrman]

Rebased.

Yes, the missing means zero, zero means default, etc. as covered on 2221 is why I was surprised to see missing mapped to defaults in v3. I must have missed the fanfare when it was introduced. :)

Any thoughts on the inability to specify a default value of the type specified by Meta("struct:field:type")? Since the error seems to come from Default(), which presumably can't see a later Meta call, I expect at best a non-trivial fix.

[raphael]

Wrt specifying default values for types equipped with Meta("struct:field:type") - Maybe there's a way to improve IsCompatible to look for that metadata and use reflection to figure out that the name of the Go literal value matches the name provided to Meta. See for example

goa/expr/types.go

Line 324 in 951acc0

func (p Primitive) IsCompatible(val interface{}) bool {

(every type implements this method).

[MichaelUrman]

I don't think IsCompatible has enough information. Maybe you can use eval.Current().(*expr.AttributeExpr) like its caller dsls.Default, but either location only works if Meta("struct:field:type", ...) precedes Default(...). To become order-independent, we have to defer verification, say until after dsl.Attribute's call to eval.Execute. This alters error reporting, at least for imported types. (I chose to still validate defaults of builtin types in the original location.)

- testing.go:18: [testdata/dsls.go:935] default value "" is incompatible with attribute of type int64 in attribute
+ testing.go:18: [../../dsl/attribute.go:236] default value _ is incompatible with attribute of type int64 in attribute

The attempt did find one existing error worth fixing. I'm not satisfied with this approach yet so have kept it off the PR for now: f4b5a44

[raphael]

Yeah the type compatibility check for default values would have to be moved to the validation step of the DSL execution, probably here: https://github.com/goadesign/goa/blob/v3/expr/attribute.go#L189 (and it probably should have been there from the get go). Thank you again for this PR, merged!