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

xdr: optional typedef generates an invalid xdr_generated.go #61

Open
abuiles opened this issue Jul 29, 2020 · 4 comments · Fixed by #64
Open

xdr: optional typedef generates an invalid xdr_generated.go #61

abuiles opened this issue Jul 29, 2020 · 4 comments · Fixed by #64

Comments

@abuiles
Copy link
Contributor

abuiles commented Jul 29, 2020

Protocol 14 includes a new type called SponsorshipDescriptor which is defined as an alias to an Optional AccountId

typedef AccountID* SponsorshipDescriptor;

The code generated by xdrgen produces an invalid output (see failure here stellar/go@3b2053c)

As a temporary workaround, I removed the marshaling/unmarshaling methods from SponsorshipDescriptor which solves the problem and makes the type behave as expected, which is an optional field -- when reading from a raw XDR it will try to read the first 4 bytes to determine if it is void or not and then read the content.

abuiles/go@5c48fa9

This unblocks stellar/go#2864 and allows us to move forward with Protocol 14 support.

This seems like a bug on xdrgen when compiling to go (it does the right thing in JS), maybe for optional typedefs we don't need marshaling and unmarshalling since we can delegate to the struct being referenced.

I also put a simple demo here https://github.com/abuiles/xdr-test/blob/master/src/main.go which shows a similar XDR definition working fine.

@bartekn
Copy link
Contributor

bartekn commented Jul 31, 2020

I think this affects all languages generated by xdrgen. Should we move the issue there?

@abuiles
Copy link
Contributor Author

abuiles commented Jul 31, 2020

@bartekn It does the right thing for JavaScript but I didn't check the other languages.

@leighmcculloch leighmcculloch transferred this issue from stellar/go Apr 15, 2021
@leighmcculloch
Copy link
Member

leighmcculloch commented Apr 15, 2021

I transferred this issue from the stellar/go repo to the stellar/xdrgen repo because I hit the same issue and started opening an issue and initially didn't see the issue because it was in the go repo. Regardless of whether this impacts all languages I think this is something we need to fix in xdrgen, not stellar/go.

To add some more details to this issue the specific errors I get are below.

I'm using:

# github.com/stellar/go/xdr
xdr/xdr_generated.go:1599:6: invalid receiver type SponsorshipDescriptor (SponsorshipDescriptor is a pointer type)
xdr/xdr_generated.go:1606:6: invalid receiver type *SponsorshipDescriptor (SponsorshipDescriptor is a pointer type)
xdr/xdr_generated.go:1612:2: cannot use (*SponsorshipDescriptor)(nil) (type *SponsorshipDescriptor) as type encoding.BinaryMarshaler in assignment:
        *SponsorshipDescriptor does not implement encoding.BinaryMarshaler (missing MarshalBinary method)
xdr/xdr_generated.go:1613:2: cannot use (*SponsorshipDescriptor)(nil) (type *SponsorshipDescriptor) as type encoding.BinaryUnmarshaler in assignment:
        *SponsorshipDescriptor does not implement encoding.BinaryUnmarshaler (missing UnmarshalBinary method)

@leighmcculloch leighmcculloch linked a pull request Sep 18, 2021 that will close this issue
leighmcculloch added a commit that referenced this issue Sep 21, 2021
### What
Skip the Go generation of the binary interface on optional types.

### Why
xdrgen generates invalid Go code when the input XDR definitions contain an optional typedef in the form:
```
typedef UnderlyingType* NamedType
```

The problem with this for the Go generator is that it results in the following Go type definition:
```go
type NamedType *UnderlyingType
```

That type definition is valid, although uncommon in Go applications, because types that name a pointer type are not allowed to have methods.

Xdrgen defines the `MarshalBinary` and `UnmarshalBinary` functions for every type is generates, and so those generated functions cause the output of xdrgen to have a compile error.

It would be great to solve this problem properly, but it is complex because we need to implement the pointer type as a non-pointer type, and then lift the pointer into any reference location. Fixing this problem is low priority, as evidenced by how long #61 has been open for.

While we wait for the problem to be prioritized it would be preferable if xdrgen generated valid Go code that compiles without modification, because it is very tedious when iterating on generated to XDR to have to manually edit it after every generation. This change achieves that.

Temporary fix for #61.

### Known Limitations

I couldn't see any tests for generated code that could be expanded on to test this, so I've tested this manually with the Stellar XDR definitions. If there's a clear, and narrowly focused, way that I can improve testing here, please let me know.
@leighmcculloch
Copy link
Member

PR #64 hacked around this issue so at least we now continue to build valid Go code, and marshaling/unmarshaling still works, we just do it a bit differently for this type. So the issue isn't completely resolved, but in practice it shouldn't cause us problems with generating Go code.

I kept the issue open though in case we want to find a more elegant solution or something better.

Related to this issue I opened a proposal at golang/go#48592 proposing that receiver pointer base types become supported in Go. That would be the ultimate win for this issue I think, but we'll see.

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.

3 participants