Shopify / protoboeuf

Experimenting with a protobuf implementation
MIT License
24 stars 2 forks source link

Add option to generate sorbet types #99

Closed TobiasBales closed 2 weeks ago

TobiasBales commented 3 weeks ago

This adds a --typed or -t flag that generates sorbet signatures for the generated files.

I decided to use inline signatures over ERBs since that allows for autocompletion to jump to the actual code instead of the rbi file and also helps discoverability the other way around (seeing the types when looking at the code).

I extracted all type related things into a module that is mixed into CodeGen, EnumCompiler and MessageCompiler and then added the calls to the relevant places. It is not the prettiest design but I am not sure of a better solution.

One big question mark There is one change in the generated code, I replaced the NONE type to indicate an omitted field with nil since it changes the signature from T.any(String, NONE) to T.nilable(String), which, looking at it from the type perspective makes a lot more sense. I briefly chatted with @tenderworks about it and he was not 100% sure if nil should be fine or not but could not remember cases where it was not. It passes all current tests - if this is breaking expectations in some way I will gladly change it back to NONE instead (and add some tests that ensure it does not get changed).

Tests I added a single test that generates code from a protobuf file and compares it against a checked in generated file (test/fixtures/typed_test.correct.rb). This means whenver the generated code changes this test will break.

The alternative would have been a complex test harness for somehow validating that all relevant methods have signatures and that they actually pass srb tc (which means adding sorbet as a dependency). This seemed like a reasonable trade-off for now but I am very open to changing it to whatever is the preferred approach.

I'll gladly squash the commits but figured they might in some cases be helpful for reviewing.

tenderlove commented 2 weeks ago

There is one change in the generated code, I replaced the NONE type to indicate an omitted field with nil since it changes the signature from T.any(String, NONE) to T.nilable(String), which, looking at it from the type perspective makes a lot more sense.

I was worried there were cases where we need to care that someone specifically set nil, but it doesn't really make sense. Only optional fields need to test NONE, and if someone specifically passes nil, they get would get the default value for that field. I think this makes sense as an API and using nil as our sentinel value is fine.

I added a single test that generates code from a protobuf file and compares it against a checked in generated file (test/fixtures/typed_test.correct.rb). This means whenver the generated code changes this test will break.

The alternative would have been a complex test harness for somehow validating that all relevant methods have signatures and that they actually pass srb tc (which means adding sorbet as a dependency). This seemed like a reasonable trade-off for now but I am very open to changing it to whatever is the preferred approach.

We probably should test that the generated code has all the Sorbet types it needs. Can we add Sorbet as a development dependency? It seems like Sorbet won't read from stdin, so maybe we could write to temp files and type check those (just run srb tc and check that it exited cleanly).

If you want to implement that, I think I'd prefer it, but having these "golden image" tests is fine for now. LMK what you think and we can go from there!

TobiasBales commented 2 weeks ago

There is one change in the generated code, I replaced the NONE type to indicate an omitted field with nil since it changes the signature from T.any(String, NONE) to T.nilable(String), which, looking at it from the type perspective makes a lot more sense.

I was worried there were cases where we need to care that someone specifically set nil, but it doesn't really make sense. Only optional fields need to test NONE, and if someone specifically passes nil, they get would get the default value for that field. I think this makes sense as an API and using nil as our sentinel value is fine.

I added a single test that generates code from a protobuf file and compares it against a checked in generated file (test/fixtures/typed_test.correct.rb). This means whenver the generated code changes this test will break. The alternative would have been a complex test harness for somehow validating that all relevant methods have signatures and that they actually pass srb tc (which means adding sorbet as a dependency). This seemed like a reasonable trade-off for now but I am very open to changing it to whatever is the preferred approach.

We probably should test that the generated code has all the Sorbet types it needs. Can we add Sorbet as a development dependency? It seems like Sorbet won't read from stdin, so maybe we could write to temp files and type check those (just run srb tc and check that it exited cleanly).

If you want to implement that, I think I'd prefer it, but having these "golden image" tests is fine for now. LMK what you think and we can go from there!

I added sorbet as a dev dependency, the problem is that a lot of the generated code does not pass sorbet (the byte banging uses values that we know not to be nil but could be nil from a sorbet perspective and adding runtime overhead seems highly undesirable). We could check it with # typed: false but then sorbet does not check anything and only provides signatures (the current setup), in order to ensure that all methods have signatures we'd need to set it to typed: strict. I am going to see if I can make it work without adding runtime overhead, I just had an idea 🤞

TobiasBales commented 2 weeks ago

And my idea did not work out as I had hoped, oh well

tenderlove commented 2 weeks ago

And my idea did not work out as I had hoped, oh well

Ugh. That's ok, thank you for trying. Lets just merge this as-is. Maybe we can figure out something later. Thanks for doing this work!