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

proto-loader-gen-types: Support nominal typing with type branding #2281

Merged
merged 7 commits into from Dec 5, 2022
Merged

proto-loader-gen-types: Support nominal typing with type branding #2281

merged 7 commits into from Dec 5, 2022

Conversation

LunaTK
Copy link
Contributor

@LunaTK LunaTK commented Nov 30, 2022

TL;DR

This PR introduces a boolean option branded for proto-loader-gen-types.

If the branded option is on, interface representing a Message has additional __type property, that has the fullName of originating Message as its value.

package test;

message FooRequest {
  string name = 1;
}

turns into

interface FooRequest {
  name: string;
  __type: '.test.FooRequest';
}

Details

Since typescript only supports structural typing, if any two types has the exact same structure, they can be used interchangeably.

This makes codes like below totally valid.

interface FooRequest {
  name: string;
}

interface BarRequest {
  name: string;
}

function requestFoo(param: FooRequest) { ... }
function requestBar(param: BarRequest) { ... }

const fooArg: FooRequest = { name: "this is foo" };
requestBar(fooArg);  // no compile error

Note : since method signature generated by proto-loader-gen-types is too complicated, I will use much intuitive version(i.e., requestFoo, requestBar) here.

If this behavior is not what you want, typically you can fix this in typescript by using technique called type branding.

https://github.com/microsoft/TypeScript/blob/7b48a182c05ea4dea81bab73ecbbe9e013a79e99/src/compiler/types.ts#L693-L698

interface FooRequest {
  name: string;
  __type: 'FooRequest';
}

interface BarRequest {
  name: string;
  __type: 'BarRequest';
}

function requestFoo(param: FooRequest) { ... }
function requestBar(param: BarRequest) { ... }

const fooArg: FooRequest = { name: "this is foo" } as FooRequest;
requestBar(fooArg);  // error!

Restricted vs Permissive

It is often cumbersome to use type assertion when you are providing the branded type.

For example, there is no additional effort required for using branded type returned from gRPC method, but creating a branded object requires type assertion.

interface FooRequest {
  name: string;
  __type: 'FooRequest';
}

interface FooResponse {
  age: number;
  __type: 'FooResponse';
}

function requestFoo(param: FooRequest): FooResponse { ... }

const res = requestFoo({} as FooRequest); // need type assertion to create branded object
const _res: FooResponse = res;  // no need for type assertion

I wonder if we need to include restricted interface only, or should we separate options for restricted and permissive interface respectively?

Motivating materials

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@murgatroid99
Copy link
Member

The runtime library doesn't output objects with these __type fields, and the suggested way to create branded objects is using a type assertion, instead of explicitly setting that field in the object. The result is that the field is essentially a fiction in the type system that doesn't reflect the runtime objects. My approach so far with this library has been to have the types reflect the runtime objects as closely as possible, so I'm really unsure of making a change like this.

For example, the example interface definitions would seem to imply that the following function would work:

function isFooRequest(obj: object): obj is FooRequest {
  return '__type' in obj && obj.__type === 'FooRequest';
}

but that function will usually not work because the __type field will not actually be populated.

@LunaTK
Copy link
Contributor Author

LunaTK commented Nov 30, 2022

Thanks for the reply!
If you at least allow this as an optional feature, I believe that it would be welcomed by many users in future.

The result is that the field is essentially a fiction in the type system that doesn't reflect the runtime objects.

Yes, this is indeed true because the purpose of type branding is to provide compile-time type compatibility checking, not for the runtime.

And this can also be seen as an advantage, depending on the perspective, because it doesn't imply any runtime overhead.

https://github.com/microsoft/TypeScript/blob/7b48a182c05ea4dea81bab73ecbbe9e013a79e99/src/compiler/types.ts#L697-L698
Note: the brands are never actually given values. At runtime they have zero cost.

Thus, keeping this special __type property in mind, library users should never use this property in runtime. So the type guard example you provided is a misleading use-case of branded types.

To recap, this is because the goal of branded type is to detect abnormal data flow in compile time, not for runtime type checking.

My approach so far with this library has been to have the types reflect the runtime objects as closely as possible

I agree with your idea. Normally I also do not prefer type branding, but I recently encountered with some requirements which need type-level metadata to make use of type manipulation. In this case, I had no option but to give up 100% reflective typing in order to get type-level metadata. For such use cases where you have no other options, leaving type branding at least as an option might be the last hope.

And the last word if you allow, to relief the uncomfortableness of type assertion for users to make objects, I guess we can exclude permissive interfaces from type branding.

The goal of generating two separate interfaces for each message type is to describe two separate things as narrowly as possible: the objects that users can pass to the library as input, and the objects that the library will output

Because objects that the library will output can be the automated starting point for the type branding.

@murgatroid99
Copy link
Member

Thus, keeping this special __type property in mind, library users should never use this property in runtime. So the type guard example you provided is a misleading use-case of branded types.

The problem I see is that users may incorrectly believe that the property is usable at runtime. I believe this can be resolved with a documentation comment in the generated code for each __type field that says something like:

This field is a type brand and is not populated at runtime. Instances of this type should be created using type assertions.


And the last word if you allow, to relief the uncomfortableness of type assertion for users to make objects, I guess we can exclude permissive interfaces from type branding.

I think a better approach here would be to have two separate options, one for permissive types and another for restrictive types. Maybe --inputBranded and --outputBranded.

Copy link
Contributor Author

@LunaTK LunaTK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate for your review!
I updated the two points you mentioned.

While adding the boolean option with default, I got excessive type instantiation error.

Seems like we are already right under the maximum number of inferable options due to the limitation of tsc, if I am not doing something wrong :(

I demonstrated two workarounds.
But indeed, requiring workaround doesn't make you pleasant 😭

Comment on lines 867 to 874
.option('inputBranded', {
boolean: true,
default: false,
})
.option('outputBranded', {
boolean: true,
default: false,
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another option we can take, apart from just leaving them optional.

By using option method, we can avoid excessive type instantiation error, still providing the same parsing behavior.

Unfortunately, this changes the order of help descriptions, lifting inputBranded and outputBranded to the top.

Since these options are not significantly important options, I guess they should remain at the bottom.

This can be fixed if all the other options are declared using option as well, but I am not sure if this breaks the current style convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing everything else to use option seems like a good way to handle this.

Copy link
Contributor Author

@LunaTK LunaTK Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it.

Since the printed order has not been exactly aligned with declaration order in the code and I don't know how exactly they were decided,
I adjusted declaration order a bit to preserve the original printed order.

packages/proto-loader/README.md Outdated Show resolved Hide resolved
packages/proto-loader/bin/proto-loader-gen-types.ts Outdated Show resolved Hide resolved
Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thank you for your contribution. There's just one more thing: please send another PR to document these options in this file similar to grpc/proposal#326 that documented another recent update

@LunaTK
Copy link
Contributor Author

LunaTK commented Dec 5, 2022

Great!
I made a PR for the proposal as well.

grpc/proposal#342

@murgatroid99 murgatroid99 merged commit 0c22b2a into grpc:master Dec 5, 2022
@murgatroid99
Copy link
Member

I published this in version 0.7.4

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 this pull request may close these issues.

None yet

3 participants