Skip to content

The global___ name mangling breaks my type check #116

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

Closed
thejohnfreeman opened this issue Apr 19, 2020 · 9 comments
Closed

The global___ name mangling breaks my type check #116

thejohnfreeman opened this issue Apr 19, 2020 · 9 comments

Comments

@thejohnfreeman
Copy link

With mypy-protobuf 1.19, mypy passed my code. Upgrading to mypy-protobuf 1.20 changes the output to add some global___ prefixes to types, breaking the type check. If I pin my dependency to 1.19, or just strip out these prefixes, mypy passes my code again. Is there another alternative? Perhaps an option to disable the name mangling?

@nipunn1313
Copy link
Owner

Hi

I believe this is the commit which added the global__ mangling (not the one linked above
22a8ad8

This commit added mangling to fix a field name / message name aliasing issue #108

I believe the global__ mangling should not create any exposed difference to your output code, as it is merely creating an additional alias in the generated code and referencing that alias.

The mangled name should be an implementation detail - implementing an option to disable name-mangling doesn't make sense here. The original name should also be exposed

Do you have an example of a generated pyi file that has this issue, as well as the associated .proto file - so we can debug what the issue is?

Thanks for the report!

Here's an example generated file from the testsuite. As you can see, both TestMessage and global__TestMessage should be exposed.

class TestMessage(google___protobuf___message___Message):
    DESCRIPTOR: google___protobuf___descriptor___Descriptor = ...
    foo = ... # type: typing___Text

    def __init__(self,
        *,
        foo : typing___Optional[typing___Text] = None,
        ) -> None: ...
    if sys.version_info >= (3,):
        @classmethod
        def FromString(cls, s: builtin___bytes) -> TestMessage: ...
    else:
        @classmethod
        def FromString(cls, s: typing___Union[builtin___bytes, builtin___buffer, builtin___unicode]) -> TestMessage: ...
    def MergeFrom(self, other_msg: google___protobuf___message___Message) -> None: ...
    def CopyFrom(self, other_msg: google___protobuf___message___Message) -> None: ...
    def ClearField(self, field_name: typing_extensions___Literal[u"foo",b"foo"]) -> None: ...
global___TestMessage = TestMessage

@nipunn1313
Copy link
Owner

Sorry about the delay in response! Times are a bit crazy. Hope you're doing well!

@nipunn1313
Copy link
Owner

I was able to find your proto file by digging around

https://github.com/ripple/rippled/blob/develop/src/ripple/proto/org/xrpl/rpc/v1/ledger.proto

In your link, seeing an error like

xpring/proto/v1/ledger_pb2.pyi:59: error: Name 'global___LedgerSpecifier.Shortcut' is not defined

At first, I thought it might have to do with some interaction between inner enums and oneofs, so I added some tests (#122) - but those seemed to work ok.

I copied the entire ledger.proto into our testsuite and could not repro any of the mypy issues.

I suspect that the mangling is a red herring and doesn't have anything to do with the issue you're facing. Something else is going on.

If you're able to open up your .pyi file for LedgerSpecifier - that may be able to help debug.
Additionally confirming what versions of both mypy and mypy-protobuf may be helpful.

@nipunn1313
Copy link
Owner

Is it possible that you have two different versions of mypy-protobuf running? Where the pyi files were generated by one version and being referenced by another version?

@thejohnfreeman
Copy link
Author

That could be possible. I tried revisiting this issue over the weekend and wasn't able to reproduce it any more. To be honest, I'm prepared to just let it go. If I figure out how to reproduce it again (by accident because I won't be dedicating any deliberate effort), then I'll re-open this issue. Thanks for taking a look.

thejohnfreeman added a commit to thejohnfreeman/xpring-py that referenced this issue Jun 10, 2020
The problem this fixed may have been caused by a misconfigured
environment.

See: nipunn1313/mypy-protobuf#116
@thejohnfreeman
Copy link
Author

According to my poetry.lock:

  • mypy: 0.730
  • mypy-protobuf: 1.21

Seems just about impossible that I would have two different versions of mypy-protobuf running. There's only one virtual environment in my project directory, which Poetry uses, and my pyenv installation has never installed mypy-protobuf, so there is no shim for it.

Here is another demonstration of the errors. Here is how I reproduce these errors on my machine:

poetry install --extras py
poetry run invoke prebuild
poetry run mypy

The first error from mypy is this:

xpring/proto/v1/ledger_pb2.pyi:65: error: Name 'type___LedgerSpecifier.ShortcutValue' is not defined

Here is the verbatim content of that line:

     shortcut: type___LedgerSpecifier.ShortcutValue = ...

It seems that the problem in at least this case is that this declaration of shortcut appears inside the definition of LedgerSpecifier, but the definition of type___LedgerSpecifier does not appear until after the definition of LedgerSpecifier, and thus after its use in the declaration of shortcut. Abridged:

class LedgerSpecifier:
    ShortcutValue = typing___NewType('ShortcutValue', builtin___int)
    shortcut: type___LedgerSpecifier.ShortcutValue = ...
type___LedgerSpecifier = LedgerSpecifier

Perhaps the not-yet-defined name should be quoted instead?

class LedgerSpecifier:
    ShortcutValue = typing___NewType('ShortcutValue', builtin___int)
    shortcut: 'type___LedgerSpecifier.ShortcutValue' = ...
type___LedgerSpecifier = LedgerSpecifier

@thejohnfreeman
Copy link
Author

thejohnfreeman commented Jun 10, 2020

The fix where I remove all type___ prefixes (except from the definition of a type___ name) works too:

class LedgerSpecifier:
    ShortcutValue = typing___NewType('ShortcutValue', builtin___int)
    shortcut: LedgerSpecifier.ShortcutValue = ...
type___LedgerSpecifier = LedgerSpecifier

I guess the name mangler changed the prefix from global___ to type___ at some point? My past fix was to remove global___ prefixes.

@nipunn1313
Copy link
Owner

Excellent debugging! Thanks!

I changed global__ to type__ since it was not actually always at global scope - while I was investigating this issue earlier.

We do have a test case which creates a similar scenario here.

proto:
https://github.com/dropbox/mypy-protobuf/blob/master/proto/test/proto/test3.proto#L32

generated pyi:
https://github.com/dropbox/mypy-protobuf/blob/master/test/proto/test3_pb2.pyi.expected#L97

This seems to pass our tests, which does mypy over the generated .pyi file.
This leads me to suspect something more complex is going on than just the snippits pasted.

These links are both to version 1.22 (not yet released). Going back in time to version 1.21, the file looks like this

https://github.com/dropbox/mypy-protobuf/blob/3cd710c3346eb65c33e64107891299f173e58b82/test/proto/test3_pb2.pyi.expected#L114

Potential hypotheses (guessing based on what might be different):

Mypy version?
Our tests (back at 1.21) ran against mypy 0.761
Our tests (on master, soon to be 1.22) run against mypy 0.780

Different mypy flags?
Our instantiation (pretty vanilla)
https://github.com/dropbox/mypy-protobuf/blob/master/run_test.sh#L40
https://github.com/dropbox/mypy-protobuf/blob/master/mypy.ini

Different test case? Perhaps paste your entire .pyi file as generated here?
Dumping your test case into mypy-playground, it appears to pass
https://mypy-play.net/?mypy=latest&python=3.8&gist=4002b2b37938f880a515a836af5c7ada

Oooh interesting!! I think mypy version is to blame here.
Somewhere between 0.750
https://mypy-play.net/?mypy=0.750&python=3.8&gist=b605afaed49a278a76deed5fbc7f8439
And 0.760
https://mypy-play.net/?mypy=0.760&python=3.8&gist=b605afaed49a278a76deed5fbc7f8439

mypy started accepting these (hit the burger in the top right -> press run to run mypy)

@thejohnfreeman
Copy link
Author

Updating mypy fixed it! Thanks for introducing me to mypy-play.net too.

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