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

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

Closed
rehmsen opened this issue Jun 1, 2023 · 1 comment · Fixed by #81
Closed

Comments

@rehmsen
Copy link
Contributor

rehmsen commented Jun 1, 2023

I think I found a bug in my own PR that is now released as v0.9.0:

Granularity *SteppingGranularity `json:"granularity,omitempty"`

But this is not actually a struct, so it does not need to be a pointer to be omitted from JSON:

type SteppingGranularity string

I will look into a fix tomorrow.

@rehmsen
Copy link
Contributor Author

rehmsen commented Jun 2, 2023

Not all type aliases seem to be affected:

Affected types:

  • CompletionItemType
  • DataBreakpointAccessType
  • SteppingGranularity

Not affected:

  • ChecksumAlgorithm - because it is not optional
  • ExceptionBreakMode - because it is not optional
  • InvalidatedAreas - only used as slice elements - these do not need to be, and are not handled as struct pointers because the slice itself can be nil when omitted

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:

if err := json.Unmarshal(m["definitions"], &typeMap); err != nil {

rehmsen added a commit to rehmsen/go-dap that referenced this issue Jun 2, 2023
This fixes an unintentional change from google#75. Type aliases to primitives
(like string) do not need to be pointers, because their zero values
will be omitted during JSON serialization if omitempty is set.

Fixes google#80
@suzmue suzmue closed this as completed in #81 Jun 2, 2023
suzmue pushed a commit that referenced this issue Jun 2, 2023
This fixes an unintentional change from #75. Type aliases to primitives
(like string) do not need to be pointers, because their zero values
will be omitted during JSON serialization if omitempty is set.

Fixes #80
@suzmue suzmue mentioned this issue Jun 2, 2023
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 a pull request may close this issue.

1 participant