Issue 850 fix imports in pyi files

[juraj-juraj]

This pull request fixes issue #850.

Imports in pyi file generated by grpc_tools.protoc are different, thus rewrite rules do not catch it.
New rule was added to account for that.

[cpcloud]

@juraj-juraj Sorry for the delay!

Would you mind adding a test case, or describing it here if you don't want to do that?

[juraj-juraj]

@cpcloud This change basically adds an import statement. So because of that, the tests will fail. I think to test this enough would be to just fix the tests. Sadly I wasn't able to make tests work, otherwise I would've done it.

[cpcloud]

Ok, I can take a look at the failures.

[cpcloud]

This is currently generating invalid Python code.

[cpcloud]

Ok, I got the tests passing, but I think the rules are incorrect.

The rewrite should only apply to pyi files, whereas now the patterns apply to every file.

[cpcloud]

@juraj-juraj can you paste here the following:

1. a proto file you used to trigger the bug
2. the codegen command you ran, e.g., buf/protoc/grpc_tools.protoc invocation
3. the protol invocation

From those I can build a test to verify the desired behavior

[juraj-juraj]

I'm including whole hierarchy of protofiles used protofiles.zip. The bug is only apparent in pyi files, where import simply isn't fixed. The rule which I introduced applies to all files, not just .pyi. I didn't know whether it would break something, and it worked for me. So that's why it is fixed like it is.
The protoc invocation:

 python3 -m grpc_tools.protoc -I./ --python_out=./src/phx_grpc_interface --grpc_python_out=./src/phx_grpc_interface --pyi_out=./src/phx_grpc_interface ./phonexia/common/core.proto ./phonexia/common/health_check.proto ./phonexia/technologies/genderid/v1beta/gid.proto ./phonexia/technologies/speakerid/v1beta/sid.proto

The protol invocation:

protol -o ./src/phx_grpc_interface --in-place protoc --proto-path=./ ./phonexia/common/core.proto ./phonexia/common/health_check.proto ./phonexia/technologies/genderid/v1beta/gid.proto ./phonexia/technologies/speakerid/v1beta/sid.proto

[v-chernyshev]

Just stumbled upon this issue in one of our projects. The proposed rewrite rule doesn't seem right:

old=f"from {from_} import {part}_pb2 as _{part}_pb2",
new=f"from {leading_dots}{from_} import _{part}_pb2",

There is no bare _{part}_pb2 in the generated code. I think the new rule should be from {leading_dots}{from_} import {part}_pb2 as _{part}_pb2 instead.

But the bigger problem is that this change does not account for name collisions. Imagine that you have packages pkg_1 and pkg_2 that both define a message type msg. And these are then imported into pkg_3.msg. The non-transformed pyi output for pkg_3.msg would contain something like this:

from pkg_1 import msg_pb2 as _msg_pb2
from pkg_2 import msg_pb2 as _msg_pb2_1

The _N suffix will be incremented for each subsequent collision.

[metasyn]

👋 What is the conclusion here?

[c1moore]

@cpcloud Are there any plans to move forward with this?