Explicitly raise an error if field names clashes with types

[Viicos]

Change Summary

Linked: #8240.

I'm raising inside from_annotated_attribute so I don't have to duplicate the logic for dataclass, BaseModel etc. But unfortunately I can't provide info to the user regarding the field causing the issue :/

Related issue number

Checklist

• The pull request title is a good summary of the changes - it will be used in the changelog
• Unit tests for the changes exist
• Tests pass on CI
• Documentation reflects the changes where applicable
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

[codspeed-hq]

CodSpeed Performance Report

Merging #8243 will not alter performance

_{Comparing Viicos:error-name-clashes (b0231e1) with main (2e459bb)}

Summary

✅ 10 untouched benchmarks

[Viicos]

This unfortunately fails with the following test:

pydantic/tests/test_edge_cases.py

Lines 1373 to 1386 in b430e5a

	def test_type_on_annotation():
	class FooBar:
	pass
	class Model(BaseModel):
	a: int = int
	b: Type[int]
	c: Type[int] = int
	d: FooBar = FooBar
	e: Type[FooBar]
	f: Type[FooBar] = FooBar
	g: Sequence[Type[FooBar]] = [FooBar]
	h: Union[Type[FooBar], Sequence[Type[FooBar]]] = FooBar
	i: Union[Type[FooBar], Sequence[Type[FooBar]]] = [FooBar]

Although a: int = int and d: FooBar = FooBar doesn't really make sense, this PR will break any existing code similar to this. So raising an error here might be tricky to implement.

[samuelcolvin]

we should use a pydantic user error with a link to a section describing the problem and solution.

[sydney-runkle]

@Viicos,

Circling back to this solution - I do think we should go ahead and implement something along the lines of what you have here. Could you use a PydanticUserError here instead, as Samuel mentioned?

Happy to review once you make that change 👍

[Viicos]

Yes I think for now this would be the best we can do. It doesn't cover all cases (e.g. this and this) but this is still better than nothing. Will apply changes soon. Regarding an actual workaround for this issue, the idea from alex could probably work although as I said it introduces a significant amount of complexity

[sydney-runkle]

Thanks! Yep, let's stick with this for now, then can consider a more complex solution in the future.

[samuelcolvin]

It would also be nice if we could provide a useful error for the case of

class City(BaseModel):
    date: int = 1
    foo: date

[sydney-runkle]

Do we want to consider a case like second one here as well?

from datetime import date
from typing import Union

from pydantic import BaseModel, Field

class Foo(BaseModel):
    date: date = Field(default_factory=date.today)

f = Foo()
#> RecursionError: maximum recursion depth exceeded while getting the repr of an object

class FooWithUnion(BaseModel):
    date: Union[date, None] = Field(default=None)

f = FooWithUnion()
#> RecursionError: maximum recursion depth exceeded while getting the repr of an object

[Viicos]

Thanks for the feedback, I'll try to come up with a solution covering all cases. Hopefully it won't involves hacky stuff as this might get tricky

[Viicos]

Do we want to consider a case like second one here as well?

@sydney-runkle where you able to run this code snippet? Union is a special form and the arguments are checked to be sure they are a type. I got the following on my end: TypeError: Union[arg, ...]: each arg must be a type. Got FieldInfo ...

Note that setting a default value of None will pass:

from datetime import date
from typing import Union

from pydantic import BaseModel

class FooWithUnion(BaseModel):
    date: Union[date, None] = None

FooWithUnion.model_json_schema()
#> {'properties': {'date': {'default': None, 'title': 'Date', 'type': 'null'}}, 'title': 'B', 'type': 'object'}
# ^ type: null!

And in this case the issue is silently ignored, so having an explicit error will be beneficial in this case as well

[sydney-runkle]

@Viicos,

You're right, I get the following:

from datetime import date
from typing import Union

from pydantic import BaseModel, Field

class FooWithUnion(BaseModel):
    date: Union[date, None] = Field(default=None)

f = FooWithUnion()
#> Union[arg, ...]: each arg must be a type. Got FieldInfo(annotation=NoneType, required=False).

Agreed, a more explicit error would be helpful 😄

[Viicos]

The only way that comes to mind currently (that could support all provided cases) would be to parse the AST, which is less than ideal (performance wise, and it adds maintenance burden). Maybe @alexmojaki you have a better/clever idea?

Or maybe comparing field names against the global module namespace? But the case provided here won't be taken into account

[sydney-runkle]

Makes sense. Curious to hear what @alexmojaki thinks as well.

The case that I mentioned isn't as urgent. As long as we cover the original case and the case that @samuelcolvin mentioned above, that's a pretty significant improvement over what we have now.

[alexmojaki]

I'm flattered 😄

Here's an idea of how it might be possible to actually fix the problem: main...alexmojaki:pydantic:field-type-clash

[Viicos]

Again, @alexmojaki really clever solution. I've played a bit and this looks promising. I'll continue experimenting and will try to come up with an implementation. Thanks again!

Edit: to be clear, this would fix the problem in the first place, which may still not be desirable. But the fix proposed here seems straightforward to implement so it's worth trying imo

[Viicos]

So I've played a bit with it, here is an implementation following @alexmojaki's idea: main...Viicos:pydantic:allow-name-clashes

To be able to support more than FieldInfo as a default value, I'm mangling all attributes (except __annotations__).

That's why I had to alter two things:

• We need a custom __setitem__ __getitem__ in case class attributes are accessed in the body:

class Model(BaseModel):
    def func(self):
        pass

    func2 = func  # Python will try to get "func" from our custom namespace, where "func" is still prefixed/mangled

• Do not "override" the custom namespace by a plain dict in dynamic model creations.

All tests passes with this implementation, but testing that it can actually use name that clashes is harder as it does not work inside functions (see here).

I'll let maintainers decide if they want to support this and continue with this solution, otherwise we can continue on this PR and try to see how we could raise better errors.

[alexmojaki]

Does this work with postponed/string annotations? Maybe to support using non-field variables in the class body (which I didn't) you could keep localns in get_cls_type_hints_lenient but exclude model_fields.

We need a custom __setitem__ in case class attributes are accessed in the body:

Do you mean __getitem__? If so, does this mean the __getitem__ is called when evaluating normal expressions in the class body, but not when evaluating annotations? That's weird.

All tests passes with this implementation, but testing that it can actually use name that clashes is harder as it does not work inside functions (see #7327 (comment)).

Is it fair to say that in every case where it doesn't work inside functions, it has never worked, and it also doesn't work with plain classes or standard dataclasses?

You could maybe make it work inside functions if you're willing to take the black magic further:

def foo():
  class A:
    pass

  class Meta(type):
    @classmethod
    def __prepare__(metacls, name, bases):
      import inspect
      frame = inspect.currentframe().f_back
      return dict(frame.f_locals)

  class B(metaclass=Meta):
    A: A

  return B


print(foo().__annotations__)

Or just use one of those test fixtures that makes a module or something.

[Viicos]

Do you mean __getitem__? If so, does this mean the __getitem__ is called when evaluating normal expressions in the class body, but not when evaluating annotations? That's weird.

Yes sorry I meant __getitem__. Actually I've just noticed it is also called for annotations, which defeats the purpose of having this name mangling in the first place (my current implementation doesn't solve the issue currently). So supporting other assignments than FieldInfo might be too risky.

Is it fair to say that in every case where it doesn't work inside functions, it has never worked, and it also doesn't work with plain classes or standard dataclasses?

Yes iirc Python uses either the local namespace or the module namespace. It was discussed somewhere but can't find the related issue.

[alexmojaki]

import inspect
from datetime import date


class D(dict):
    def __getitem__(self, key):
        if not key[:2] == '__' == key[-2:]:
            frame = inspect.currentframe().f_back
            assert frame.f_locals is self

            def namespaces():
                # Get namespaces from enclosing scopes
                outer_frame = frame
                while outer_frame.f_back and outer_frame.f_code in outer_frame.f_back.f_code.co_consts:
                    outer_frame = outer_frame.f_back
                    yield outer_frame.f_locals

                yield frame.f_globals
                yield frame.f_builtins

            for namespace in namespaces():
                try:
                    value = namespace[key]
                except KeyError:
                    pass
                else:
                    if key in self:
                        print(f'Found {key=} in outer namespace, using that instead of value in class')
                    return value
        return super().__getitem__(key)


class M(type):
    @classmethod
    def __prepare__(metacls, name, bases):
        return D()


def bar():
    # Check that we can access variables from enclosing scopes
    # even if Python doesn't create a normal closure for them.
    E = 10

    # Check that inner scopes take precedence over enclosing scopes.
    A = 11

    def foo():
        try:
            # Verify that no closure was created for E
            # Replacing the line below with just `E` makes a closure and fails the assertion.
            # Same for replacing `E: E` further down with `e: E`.
            eval('E')
        except NameError:
            pass
        else:
            raise AssertionError('E should not be defined in foo')

        A = 12

        class C_(metaclass=M):
            B = 13

            A: A = 1
            b: B = 2
            date: date = 3
            int: int = 4
            x: int = 5
            y: int = x
            E: E = 6

        return C_

    C = foo()
    # print(C.A, C.b, C.date, C.int, C.x, C.y)
    # print(C.__annotations__)
    assert C.A == 1
    assert C.b == 2
    assert C.date == 3
    assert C.int == 4
    assert C.x == 5
    assert C.y == 5
    assert C.__annotations__ == {
        'A': 12,
        'b': 13,
        'date': date,
        'int': int,
        'x': int,
        'y': int,
        'E': 10,
    }


def spam():
    # Check that stack frames that aren't enclosing scopes aren't used.
    date = 99

    bar()


spam()

[alexmojaki]

To explain the above code (which I've just updated): we can use the custom namespace to override Python's usual scoping rules and check for a local class variable last (i.e. after enclosing scopes, globals, and builtins) rather than first. Of course that could be weird and surprising in some cases, but we can easily emit warnings/errors when relevant so that the user can see what's happening, and this can be as configurable as we want so that upgrading is still safe. I can explain more but right now I need to go to sleep 😆

[Viicos]

This looks interesting but I don't know if this level of complexity will be accepted? :/

[sydney-runkle]

@Viicos,

Circling back to this solution - I do think we should go ahead and implement something along the lines of what you have here. Could you use a PydanticUserError here instead, as Samuel mentioned?

Happy to review once you make that change 👍

[sydney-runkle]

@Viicos,

I know you're busy (helping us with other things, partially 🚀 ), but if you have any time to make a minor change here, we'd love to get this change in with the 2.6.0 release early next week 💯.

[Viicos]

I know you're busy (helping us with other things, partially 🚀 ), but if you have any time to make a minor change here, we'd love to get this change in with the 2.6.0 release early next week 💯.

Will take a look this evening/weekend!

[Viicos]

As said here: I just removed the now invalid cases

[sydney-runkle]

Looks great :), thanks!

[Viicos]

Regarding the other mentioned use cases, it could also be implemented (more easily) in a linter plugin, e.g. with flake8 or ruff. There are some interesting rules that could also be applied, e.g. enforcing field: Annotated[<type>, Field(...)] instead of field: <type> = Field(...), or any use case where people with large code bases would like to enforce consistency.

[sydney-runkle]

@Viicos,

Looks like a test is still failing - could you please take a look at that before we merge?

[sydney-runkle]

@Viicos,

Hmm, still some linting and testing issues - are the make commands working for you locally?

[Viicos]

@samuelcolvin @sydney-runkle

I created flake8-pydantic yesterday, with a bunch of handy rules. One of them hopefully covers most of the use cases we encountered.

Let me know if you have other rules in mind that could be implemented!

[sydney-runkle]

@Viicos,

Nice! That's great!