nutti / fake-bpy-module

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

mesh.materials is missing possible None type #254

Open Andrej730 opened 2 weeks ago

Andrej730 commented 2 weeks ago

fake-bpy-module-latest==20240618

We can add None to the mesh.materials but if we try to iter over it, it's missing possible None type.

import bpy
from typing import assert_type, Union

mesh = bpy.data.meshes["Test"]
mesh.materials.append(None)

a = list(mesh.materials)
# "assert_type" mismatch: expected "list[Material | None]" but received "list[Material]"
assert_type(a, list[Union[bpy.types.Material, None]])

Test snippet that should pass:

# pyright: reportUnnecessaryTypeIgnoreComment=error

import bpy
from typing import assert_type, Union

mesh = bpy.data.meshes["Test"]
material = bpy.data.materials["Material"]

assert_type(list(mesh.materials), list[Union[bpy.types.Material, None]])
assert_type(mesh.materials.values(), list[Union[bpy.types.Material, None]])
mesh.materials.find("Material")
mesh.materials.find(None)  # type: ignore [reportArgumentType]
mesh.materials.get("Material")
mesh.materials.get(None)  # type: ignore [reportArgumentType]
mesh.materials.pop(None)  # type: ignore [reportArgumentType]
mesh.materials.pop(5)  # type: ignore [reportCallIssue]
mesh.materials.pop(index=5)
mesh.materials.__contains__("Material")
mesh.materials.__contains__(material)  # type: ignore[reportArgumentType]
print(material in mesh.materials)  # type: ignore[reportArgumentType]

# __setitem__
mesh.materials[0] = None
mesh.materials[0] = material
mesh.materials["Material"] = None  # type: ignore[reportArgumentType]
mesh.materials["Material"] = material
Andrej730 commented 2 weeks ago

Found a bit more issues with mesh.materials:

  1. mesh.materials.values is missing possible None values. Note that keys and items are also missing possible None values but for keys and items it is correct. 🙃
  2. mesh.materials.find allows None value though in Blender it's not actually allowed
  3. mesh.materials.get allows None value though in Blender it's not actually allowed
  4. mesh.materials.pop allows None value though in Blender it's not actually allowed
  5. mesh.materials.pop allows supplying index a positional argument but in Blender this argument is keyword only and Blender will raise an Exception if it's supplied positionally.
import bpy

obj = bpy.data.objects["Cube"]
mesh = obj.data
assert isinstance(mesh, bpy.types.Mesh)
mesh.materials.append(None)

print(mesh.materials.values()) # [bpy.data.materials['Material'], None]
values = mesh.materials.values()
# Type of "values" is "list[Material]"
# Should have been: list[Material | None]
# reveal_type(values)

# though this has a correct typing - those methods do skip None
print(mesh.materials.keys()) # ['Material']
print(mesh.materials.items()) # [('Material', bpy.data.materials['Material'])]

# no type errors for two statements below
# in Blender actually will fail with the same error:

# Error: Python: TypeError: bad argument type for built-in operation
# The above exception was the direct cause of the following exception:
# Traceback (most recent call last):
#   File "\Text", line 6, in <module>
# SystemError: <built-in method find of bpy_prop_collection object at 0x0000023092A7A3D0> returned a result with an exception set

print(mesh.materials.find(None))
print(mesh.materials.get(None))

Another example:

>>> C.object.data.materials.pop(index=None)
Traceback (most recent call last):
  File "<blender_console>", line 1, in <module>
TypeError: IDMaterials.pop(): error with keyword argument "index" -  Function.index expected an int type, not NoneType

>>> C.object.data.materials.pop(5)
Traceback (most recent call last):
  File "<blender_console>", line 1, in <module>
TypeError: IDMaterials.pop(): required parameter "index" to be a keyword argument!
Road-hog123 commented 2 weeks ago

Mesh.materials is documented as an IDMaterials bpy_prop_collection of Material—no mention of None. bpy_prop_collection.values() is then working as intended for a bpy_prop_collection that does not contain None.

Issues 2 to 5 are issues with bpy_prop_collection (and bpy_struct in the case of pop())—I would suggest opening a separate issue for them.

I'll look into drafting a Blender docs PR to add None to places where material slots can be empty.

Andrej730 commented 1 week ago

Found 1 more issue - a bit confusing one. When we use mesh.materials.__contains__ it doesn't allow any types besides the strings, which is correct. But if we do material in mesh.materials it does allow a Material type though in Blender it will still result in error as it's still the same __contains__ method and only strings are allowed.

import bpy

mesh = bpy.data.meshes["Cube"]
material = bpy.data.materials["Material"]

print("Material" in mesh.materials)
# Shows a type error which is okay.
mesh.materials.__contains__(material)

# Traceback (most recent call last):
#   File "<blender_console>", line 1, in <module>
# TypeError: bpy_prop_collection.__contains__: expected a string or a tuple of strings
# Doesn't show a type error though it won't work in Blender.
print(material in mesh.materials)
nutti commented 1 week ago

@Road-hog123 @Andrej730 @JonathanPlasse

This discussion may relate to #243 .

Mesh.materials is documented as an IDMaterials bpy_prop_collection of Material—no mention of None.

I think this can not be handled from the documentation because never None and other options are generated by the internal flag. Before fixing this issue, we should consider the strategy to find which arguments/return/attributes are optional (accept None) or not. Current strategy is here.

The code can be found at https://github.com/nutti/fake-bpy-module/blob/991583418cf2026dc1603a247b0d84f98983e17e/src/fake_bpy_module/transformer/data_type_refiner.py#L490-L552 https://github.com/nutti/fake-bpy-module/blob/991583418cf2026dc1603a247b0d84f98983e17e/src/fake_bpy_module/transformer/data_type_refiner.py#L571-L580

Could you give me the advice to improve the strategy to fix this issue? Optional data type is annoying point due to the inconsistent information on documentation.

Road-hog123 commented 1 week ago

Optional data type is annoying point due to the inconsistent information on documentation.

Yeah, I suspect the only way to properly fix this is to make the documentation consistent—I hope to find some time and energy to really dive into issues like this soon.

I'll look into drafting a Blender docs PR to add None to places where material slots can be empty.

As I feared, this is non-trivial, so I can't do it right now. 😞

With regards to:

5. mesh.materials.pop allows supplying index a positional argument but in Blender this argument is keyword only and Blender will raise an Exception if it's supplied positionally.

@nutti how best to declare arguments as keyword-only?

Andrej730 commented 1 week ago

how best to declare arguments as keyword-only?

perhaps this issue could help - https://github.com/nutti/fake-bpy-module/issues/226

Update. Found one more problem - it's regarding mesh.materials.__setitem__. Updated the tests in the first post.

# Argument of type "None" cannot be assigned to parameter "value" of type "Material" in function "__setitem__"
  "None" is incompatible with "Material"
# fails but shouldn't
mesh.materials[0] = None

# type check fails and should keep failing as it will raise an error in Blender
# TypeError: bpy_prop_collection[key]: invalid key, must be a string or an int, not str
mesh.materials["Material"] = None

It's a bit tricky and requires an overload to resolve...

@overload
def __setitem__(self, key: int, value: GenericType1 | None): ...
@overload
def __setitem__(self, key: str, value: GenericType1): ...
def __setitem__(self, key: int | str, value: GenericType1 | None): ...
nutti commented 1 week ago

Maybe #226 is a bit complicated because this uses the transformers.

Does below syntax work to specify the keyword-only argument in mod file?

.. function:: some_func(arg_1, arg_2, *, kwonly_arg_1, kwonly_arg_2)
Road-hog123 commented 1 week ago

mesh.materials['Material'] = None fails because assignment to string keys is not supported by bpy_prop_collection—even `mesh.materials['Material'] = mesh.materials['Material'] will fail with the same error. I have opened #264 to remove that support from this module, and Blender PR #123577 to fix the unhelpful error message.

The problem with mesh.materials[0] = None is not that __setitem__ does not accept None in addition to GenericType, but that GenericType is not Material | None for IDMaterials—all of the inherited methods that accept or return GenericType will be wrong when the value passed as GenericType is wrong.

While investigating in the code, I did find a function pyrna_prop_collection_type_check which includes this snippet:

if (value == Py_None) {
  if (RNA_property_flag(self->prop) & PROP_NEVER_NULL) {
    PyErr_Format(PyExc_TypeError,
                 "bpy_prop_collection[key] = value: invalid, "
                 "this collection doesn't support None assignment");
    return -1;
  }

  return 0; /* None is OK. */
}

I don't know if the generation code has access to the flags, but it would seem that for bpy_prop_collection there exists a flag that declares whether GenericType should be a union with None.

Does below syntax work to specify the keyword-only argument in mod file?

.. class:: IDMaterials

   .. method:: pop(self, *, index)

This had no effect on the generated modules, but it seems like a reasonable proposal.