Handle mutable default arguments cleanly

[abn]

When generating code, ensure that default list/dict arguments are initialised in local scope if unspecified or None.

The following diff shows the difference in the generated source file from service.proto.

5c5
< from typing import AsyncIterable, AsyncIterator, Iterable, List, Union
---
> from typing import AsyncIterable, AsyncIterator, Iterable, List, Optional, Union
35c35
<         self, *, name: str = "", comments: List[str] = []
---
>         self, *, name: str = "", comments: Optional[List[str]] = None
36a37
>         comments = comments or []

Due to the nature of the issue, the included test simply verifies that the default list instance initialised is not the same object used in any subsequent calls. This is done so by intercepting the calls made to the underlying ServiceStub._unary_unary method and verifying that the comments instance is not reusing the object.

[nat-n]

Nice. Maybe worth having a test case for this (along the lines of https://github.com/danielgtaylor/python-betterproto/blob/master/betterproto/tests/grpc/test_grpclib_client.py#L33 )? I assume you've verified it manually?

[abn]

@nat-n I did test it manually. Although turns out I did not handle the edge case when Optional was never imported. Had to change tact to modify the code instead of the template. Added a test, although had to make use of pytest-mock since the affected code is in the generated files. This should now be good to go.

[nat-n]

less complex logic in jinja makes me feel better.

[boukeversteegh]

Ah, sorry it took me so long, but i finally get it.
The diff in the example was just a tad too short to get the context..

So:

• This is about the arguments of methods of ServiceStubs (aka Clients)
• When a Service accepts a message that contains a repeated field, a parameter was created with a default []
• If the caller does not specify the parameter, the default parameter is used
• If that parameter is then modified by the ServiceStub, it will contain the modifications upon the next call.
• This would cause unexpected and hard to debug behavior

Correct?

What I still don't understand is:

• In what way might the default parameter accidentally become modified, and leak into the next call?

From what I see, the request object created on the fly, and then passed to grpcio for serialization and sending. In theory it should not ever be modified.

[abn]

@boukeversteegh that is correct. The change is to ensure generated code adheres to best practices. Since the project aims to generate clean pythonic code at best effort, I think this is a change in that direction.

I am not across all possible scenarios, present and future, of how this code gets used to say for certain that the default value (initialised at definition) will never be mutated prior to serialisation. This is more a defense-in-depth change.

[abn]

@boukeversteegh this should be ready for review again. The recent refactor in master makes the implementation here a bit cleaner.

[boukeversteegh]

Looks good!

[abn]

@boukeversteegh @nat-n rebased and ready again; had to rebased due to improt statement collision.