"None" as protobuf type names breaks mypy codegen

A protobuf type message None { ... } won't appear in the .pyi file at all, and other messages which use the type will reference a non-existing global__None
If the message type is an inner message, it instead generates syntactically invalid code, e.g.
message Message {
  message None { }
...

will generate a file with global__Message.None which is not valid Python syntax. Can update with a repo showing this behavior soon. I hypothesize this happens with all keywords as well, not just None.

See also: https://github.com/dropbox/mypy-protobuf/issues/80

Here's a repo showing None used in various places inside proto files, and the generated code so you can easily see the issues: https://github.com/alkasm/mypy-protobuf-none
I split into many files so that I could reuse the name None multiple times in different contexts (which otherwise produces errors)

Great find and thank you for reproing! I was able to repro, add tests and whip up a fix (coming soon)

Added you as a contributor in that diff! Really good bugreport.
Thank you and appreciated!

@nipunn1313 great, and no problem. For the attempted solution, I agree that typing it as Any and not giving it a class will be an immediate fix to the syntactically invalid code, so that's cool (and I'm happy with the PR).
What do you think about generating the types with the usual underscore suffix? E.g. from_, to_, None_, and so on? They can still be more helpful than Any in these contexts.

On a more tangential note...
The protobuf compiler handles these cases somewhat gracefully---I generated the normal Python code for this and committed it back to the repo above if you want to take a look. For top-level messages, protoc compiler inserts the types by their string name into the globals dict, and for nested message types, uses getattr/hasattr. I don't think these sort of runtime shenanigans are currently supported by mypy. It is quite hard to google for, but I found no mention of Python reserved keywords on the mypy or typing repo issues.
FWIW I tried some naive things to combine my suffix suggestion with the runtime business in the .pyi file and indeed mypy did not like it. For example:
class Message(google.protobuf.message.Message):
    DESCRIPTOR: google.protobuf.descriptor.Descriptor = ...
    class None_(google.protobuf.message.Message):
        DESCRIPTOR: google.protobuf.descriptor.Descriptor = ...
        FIELD_FIELD_NUMBER: builtins.int
        field: typing.Text = ...

        def __init__(self,
            *,
            field : typing.Text = ...,
            ) -> None: ...
        def ClearField(self, field_name: typing_extensions.Literal[u"field",b"field"]) -> None: ...

    NONE_FIELD_NUMBER: builtins.int

    @property
    def none(self) -> global___Message.None_: ...

    def __init__(self,
        *,
        none : typing.Optional[global___Message.None_] = ...,
        ) -> None: ...
    def HasField(self, field_name: typing_extensions.Literal[u"none",b"none"]) -> builtins.bool: ...
    def ClearField(self, field_name: typing_extensions.Literal[u"none",b"none"]) -> None: ...
setattr(Message, "None", Message.None_)
global___Message = Message

This is runnable/syntactically valid, but there's no way to reference Message.None in a typing context that I could figure out. msg: "none_pb2.Message.None" yields "error: Invalid type comment or annotation", same with msg: getattr(none_pb2.Message, "None"). This doesn't even work with non-reserved names, though the error is different: "Name "none_pb2.Message.A" is not defined". All of these things are supported by the annotations syntax in general (they are valid Python expressions) but mypy is not smart enough to utilize that. Now that I'm thinking about this, I realize I have no idea how mypy figures out what variables are on the class (since the runtime work does add them to the class dictionary).

Yep - per https://developers.google.com/protocol-buffers/docs/reference/python-generated#keyword-conflicts - if the field name conflicts with a python attr - you end up with a field only accessible with getattr/setattr.
The docs are a bit sparse on what happens with message names, but from my testing, you have to do something like
from v1 import None # doesn't work
import v1
getattr(v1, "None")

This means that you won't be able to access this type from a mypy typing context.
I'll add a test of the above variety to the PR
mypy doesn't run code in the way that you describe - so lines like setattr(Message, "None", Message.None_) aren't going to be executed in mypy. We could generate types like None_ where needed, but it wouldn't be particularly useful because there wouldn't be a natural way to get ahold of the type None_ since the only way to access such a field is via getattr which is inherently not typed as such.
I could potentially imagine using overload + literal to get better typing on getattr - which might make reserved fields with the underscore strategy more useful for nested fields, but still wouldn't be helpful for module-level message names.
https://mypy.readthedocs.io/en/stable/literal_types.html
Something like
class Message(google.protobuf.message.Message):
  @overload
  def getattr(attr: Literal["None"]) -> None_: ...
  @overload
  def getattr(attr: Literal["Foo"]) -> Foo: ...

It seems like it might clutter the stubs for limited value, so I'm hesitant to really jump in with such a strategy.

We could generate types like None_ where needed, but it wouldn't be particularly useful because there wouldn't be a natural way to get ahold of the type None_ since the only way to access such a field is via getattr which is inherently not typed as such.

Hmm, well you can access the field by name when the type name is what is reserved, rather than the field name: message.none which could be of type Message.None_ . But maybe since other access methods (namely, the sanctioned getattr) would return Any, it's not a particularly useful annotation (i.e. mypy would complain about sending the getattr'd one into a function call).
The getattr overloads would probably be acceptable if they covered all the use-cases, but since they don't, I agree with your assessment. Valiant attempt though!

Hmm, well you can access the field by name when the type name is what is reserved, rather than the field name: message.none which could be of type Message.None_ .

This is a good point. I'll file an issue for supporting messages with python reserved keyword names.


Dev securityinsights microsoft.security insights 2022 04 01 preview security ml analytics settings

MSFT employees can try out our new experience at OpenAPI Hub  - one location for using our validation tools and finding your workflow.

Changelog
Add a changelog entry for this PR by answering the following questions:

What's the purpose of the update?

[ ] new service onboarding
[ ] new API version
[x] update existing version for new feature
[ ] update existing version to fix swagger quality issue in s360
[ ] Other, please clarify

When are you targeting to deploy the new service/feature to public regions? Please provide the date or, if the date is not yet available, the month. March 29, 2022
When do you expect to publish the swagger? Please provide date or, the the date is not yet available, the month. April 1, 2022
If updating an existing version, please select the specific language SDKs and CLIs that must be refreshed after the swagger is published.

[x] SDK of .NET (need service team to ensure code readiness)
[x] SDK of Python
[x] SDK of Java
[x] SDK of Js
[x] SDK of Go
[x] PowerShell
[x] CLI
[x] Terraform
[ ] No refresh required for updates in this PR

Contribution checklist:

[x] I commit to follow the Breaking Change Policy of "no breaking changes"
[x] I have reviewed the documentation for the workflow.
[x] Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist

Applicability: :warning:
If your changes encompass only the following scenarios, you should SKIP this section, as these scenarios do not require ARM review.

Change to data plane APIs
Adding new properties
All removals

Otherwise your PR may be subject to ARM review requirements. Complete the following:

[x] Check this box if any of the following apply to the PR so that label "WaitForARMFeedback" will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.

Adding a new service
Adding new API(s)
Adding a new API version
-[x] To review changes efficiently, ensure you are using OpenAPIHub to initialize the PR for adding a new version. More details, refer to the wiki.

[x] Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.

[ ] If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.

[ ] Removing API(s) in a stable version
[ ] Removing properties in a stable version
[ ] Removing API version(s) in a stable version
[ ] Updating API in a stable or public preview version with Breaking Change Validation errors
[ ] Updating API(s) in public preview over 1 year (refer to Retirement of Previews)

Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.

@jungph808 is there a reason why this API is not modeled as part of the current Settings API? Have you considered this?
It seems strange to have a settings AP that today contains settings for Anomalies, EyesOn, EntityAnalytics, Ueba, and another completely different API just for

@jungph808 is there a reason why this API is not modeled as part of the current Settings API? Have you considered this? It seems strange that there is an API that today already includes several types of settings in the product (like Anomalies, Entity Analytics, UEBA), and another completely different API just for ML Settings. What do you think?

Hi @dosegal The current Settings Api is for controlling workspace settings, whereas SecurityMLAnalyticsSettings is more granular offering customization to individual blackbox Analytics settings. We thought the resources were too different to be modeled under the same Api.


Add Templatized C++ Command Line Parser (TCLAP) header-only library port

TCLAP is a header only command line parser library.

Hi @drdanz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

Looks good!
If you can take a few minutes to sign the cla, we'll merge it right in.

@drdanz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.
Thanks, MSBOT;

Thanks!