google / go-dap

Go implementation of the Debug Adapter Protocol
Apache License 2.0
127 stars 22 forks source link

SteppingGranularity is not a struct but a string alias and should not use a pointer #80

Closed rehmsen closed 1 year ago

rehmsen commented 1 year ago

I think I found a bug in my own PR that is now released as v0.9.0: https://github.com/google/go-dap/blob/d1322a70064e1fabb2ed53e2a576f1da5e1213fa/schematypes.go#L914

But this is not actually a struct, so it does not need to be a pointer to be omitted from JSON: https://github.com/google/go-dap/blob/d1322a70064e1fabb2ed53e2a576f1da5e1213fa/schematypes.go#L1880

I will look into a fix tomorrow.

rehmsen commented 1 year ago

Not all type aliases seem to be affected:

Affected types:

Not affected:

The tricky part is that we process stuff in the order it is in the JSON file, so by the time a property of type SteppingGranularity is encountered and emitted, we have not yet parsed the definition of that type, so we do not know if it is going to be a struct or a primitive type alias.

One possibility would be to hard code the known type aliases somewhere - that does not feel ideal, but we are doing it already for cases where the spec defines an omitted optional boolean to be interpreted as true, and we used to also do this for structs.

The other option is to make a second pass to collect these before starting to emit in source order. It looks like we are anyways not streaming anything, and unmarshal all definitions before starting with any emitting here: https://github.com/google/go-dap/blob/d1322a70064e1fabb2ed53e2a576f1da5e1213fa/cmd/gentypes/gentypes.go#L595