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

Integer should be generated as an int32 #113

Open
raiju opened this issue Apr 23, 2020 · 3 comments
Open

Integer should be generated as an int32 #113

raiju opened this issue Apr 23, 2020 · 3 comments

Comments

@raiju
Copy link

raiju commented Apr 23, 2020

What happened?

The integer conjure type refers to a 32-bit signed integer. Go generates the int type, which is a platform/arch dependent keyword. When run on a 64 bit system, it's usually equivalent to int64 (see https://tour.golang.org/basics/11).

See the example yaml and the generated go code

What did you want to happen?

Generate an int32 instead.

@nmiyake
Copy link
Contributor

nmiyake commented May 1, 2020

Thanks for filing this issue. In short, what you said is correct, and this is something that we were aware of when writing conjure-go.

@bmoylan and I discussed this and determined there was a balance between correctness and usability, and initially opted on the side of usability.

Generally, numeric values in a codebase are overwhelmingly of the type int. Making the Conjure integer type int32 would require type conversion extensively throughout a codebase to use Conjure.

It was our opinion that, even if we made the generated type for a Conjure "integer" int32, it wouldn't necessarily lead to more correct code -- in a vast majority of the cases, clients are likely to just convert directly from int to int32, which wouldn't actually address overflow issues.

However, certainly open to updating the decision based on feedback/data. However, even if we do implement this, it will need to be opt-in until a major version change, as generated code will encounter compile failures if we change this.

@raiju
Copy link
Author

raiju commented May 9, 2020

The issue is that for all numbers above 2^31 the generated wire format is wrong. This isn't about overflows, this makes it impossible to communicate certain values. One of the uses of the integer type is to pass through a checksum. If the first bit of the checksum is set, the client will fail to decode the sent data & be unable to read the data. In other words, 50% of all states are undecodable.

This issue isn't one of aesthetics. If we e.g. wrap the encoding to generate the correct signed value for values > 2^31 that's fine. I don't have an opinion on code-base internals. But the generated wire format must match up.

@nmiyake
Copy link
Contributor

nmiyake commented May 20, 2020

Chatted with @raiju directly.

The most correct solution would certainly be to generate int32, but this would be a pretty wide-spread break. We could consider this as a breaking change, but would have to think long and hard about it.

We could also consider a solution that would address just the wire format part for clients/servers. For example:

  1. Update codegen on structs to do bounds checking on int values as part of json.Marshshal and json.Unmarshal
  2. For any alias types of int, codegen JSON marshal/unmarshal that bounds-checks
  3. In all generated client and server code, generate bounds-checking code for all int parameters

The set of those 3 steps together should be sufficient to ensure that our generated code never produces or accepts an out-of-bounds int over the wire. Note that this solution is also somewhat similar to #123 (comment), in that it involves generating custom marshal/unmarshal code that performs stricter verification against the standard.

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

No branches or pull requests

2 participants