nutti / fake-bpy-module

Fake Blender Python API module collection for the code completion.
MIT License
1.37k stars 97 forks source link

Issues with using context.temp_override #230

Closed Andrej730 closed 4 months ago

Andrej730 commented 4 months ago

fake-bpy-module-latest==20240522

Example is below 1) window, area, region, keywords are all actually optional arguments and first error error shouldn't occur 2) keywords is defined as a single argument instead of **keywords - therefore the errors "No parameter named 'xxx'".

import bpy

# Arguments missing for parameters "window", "area", "region", "keywords"
# No parameter named "active_object"
# No parameter named "selected_objects"
with bpy.context.temp_override(active_object=None, selected_objects=[]):
    pass

Related: temp_override in Blender docs temp_override docstring in bpy_rna_context.cc

Road-hog123 commented 4 months ago

window, area, region are all actually optional arguments

So these arguments should default to None?

Andrej730 commented 4 months ago

@Road-hog123

So these arguments should default to None?

As far as I remember, in the C-code passing None to those keywords arguments is different than not passing anything at all - https://projects.blender.org/blender/blender/src/commit/81a1570cf59dfa6634c1116ddf55da8f93d47752/source/blender/python/intern/bpy_rna_context.cc#L678-L696

E.g. if area is not passed then temp_override assumes that area doesn't need an override and can just keep the original value. But if None value passed for area then area actually will be overriden with None (I guess, we also can create some kind of constant NULLPTR to use it here - though it will be more correct but probably will be only confusing).

See example.

import bpy

context = bpy.context

with context.temp_override():
    # <bpy_struct, Area at 0x00000267F89AFD08>
    print(context.area)

with context.temp_override(area=None):
    print(context.area) # None

I'm not sure what's the best way to type it in Python but default values can't be None as None has different meaning than omitting the argument. We can use ... as probably no one will be passing it to the temp_override.

I think something like this will work. Note the *, as this function doesn't take any positional arguments and it will help avoid issue like #226. See https://projects.blender.org/blender/blender/src/commit/81a1570cf59dfa6634c1116ddf55da8f93d47752/source/blender/python/intern/bpy_rna_context.cc#L636-L642

def temp_override(
    self, *, window: Window | None = ..., area: Area | None = ..., region: Region | None = ..., **keywords: typing.Any
):
nutti commented 4 months ago

This issue is the same reason with #231.

Andrej730 commented 2 months ago

@nutti the issue persists on fake-bpy-module-latest-20240708

image

nutti commented 2 months ago

@Andrej730

This issue is fixed by https://projects.blender.org/blender/blender/pulls/124348.

Road-hog123 commented 2 months ago

@nutti

passing None to those keywords arguments is different than not passing anything at all

Additionally attempting to generate modules after this change results in a SyntaxError exception.

nutti commented 2 months ago

@Road-hog123

Where place do you encounter this error? Below code seems work to me.

>>> import ast
>>> ast.parse("def temp_override(window=None, area=None, region=None, **keywords): pass")
<ast.Module object at 0x00000256712458A0>
Road-hog123 commented 2 months ago

@nutti Unfortunately that isn't what ast is asked to parse, presumably because it gets transformed incorrectly in an earlier step:

  File "/usr/lib/python3.12/ast.py", line 52, in parse
    return compile(source, filename, mode, flags,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<unknown>", line 1
    def temp_override(window=None, area=None, region=None, **keywords=None): pass
                                                                     ^
SyntaxError: var-keyword argument cannot have default value
nutti commented 2 months ago

@Road-hog123

Ah, OK. We should need to consider not only **kwargs but also **keyword as well.

Road-hog123 commented 2 months ago

Ideally any argument name with a **-prefix

Road-hog123 commented 2 months ago

In my opinion the best solution for typing this particular method would be to get as close as possible to the actual C implementation:

def temp_override(self, **kwargs: Any): ...

This forces window, area & region to be keyword-only arguments while allowing them to be omitted without needing to specify a default value, however we lose their type information.

Using 3.12 syntax we can annotate **kwargs with a TypedDict:

class _Context_TempOverride_KWArgs(typing.TypedDict, total=False):
    window: Window
    screen: Screen
    area: Area
    region: Region

def temp_override(self, **kwargs: typing.Unpack[_Context_TempOverride_KWArgs]): ...

However it is currently an error to construct a TypedDict with extra keywords, so we would need to identify all possible other members that can be overridden (e.g. active_object).

I think we can avoid this extra work by requiring typing_extensions>=4.10, which adds support for proposed PEP 728:

class _Context_TempOverride_KWArgs(typing_extensions.TypedDict, total=False, closed=True):
    window: Window
    screen: Screen
    area: Area
    region: Region
    __extra-items__: Any

def temp_override(self, **kwargs: typing.Unpack[_Context_TempOverride_KWArgs]): ...

We would still be free to add other members like active_object at our leisure.

Thoughts?

nutti commented 2 months ago

@Road-hog123

Ideally, yes. But, I think imitating the actual C implementation is not good idea for the long maintenance. If the C implementation is changed without any notification, it is difficult to track it.

We should stick to the document based generation as far as possible.

nutti commented 2 months ago

Fixed by https://github.com/nutti/fake-bpy-module/commit/acb46a6f1b2e22f6f97a2b90c9a1db736192d26a

Andrej730 commented 2 months ago

@nutti there is still an issue with this, there should be self, * in the signature to indicate that this method takes no positional arguments.

E.g. though there are no type errors for the code below, it will result in error if you try to run it in Blender.

import bpy

context = bpy.context

# TypeError: temp_override() takes no positional arguments
# Error: Python: Traceback (most recent call last):
#   File "\Text", line 5, in <module>
with context.temp_override(None):
    print(context.window)

Also, signature shows default values as None but actually providing None would have a different result. But I'm not sure if there's any use cases that really need to provide None as one of the arguments, so I guess we can just leave it the way it is.

import bpy

context = bpy.context

with context.temp_override():
    # <bpy_struct, Area at 0x00000267F89AFD08>
    print(context.area)

with context.temp_override(area=None):
    print(context.area) # None
nutti commented 2 months ago

Submitted the additional patch. https://projects.blender.org/blender/blender/pulls/125132

Andrej730 commented 2 months ago

let's keep this issue open for now then?

nutti commented 1 month ago

Additional Patch is now merged. Thank you for following up this issue.