Support Field(default_factory) in validate_arguments

[thomascobb]

Change Summary

Add preliminary support for Field to validate_arguments

I expect there will be issues with my approach and edge cases, this is a starting point for a discussion

Related issue number

fix #1359

Checklist

• Unit tests for the changes exist
• Tests pass on CI and coverage remains at 100%
• Documentation reflects the changes where applicable
• changes/<pull request or issue id>-<github username>.md file added describing change
  (see changes/README.md for details)

[codecov]

Codecov Report

Merging #2176 (8f5fcee) into master (fc18f8e) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2176      +/-   ##
==========================================
- Coverage   99.97%   99.90%   -0.08%     
==========================================
  Files          23       25       +2     
  Lines        4500     5030     +530     
  Branches      909     1030     +121     
==========================================
+ Hits         4499     5025     +526     
  Misses          1        1              
- Partials        0        4       +4     

Impacted Files	Coverage Δ
pydantic/decorator.py	100.00% <100.00%> (ø)
pydantic/fields.py	100.00% <0.00%> (ø)
pydantic/schema.py	100.00% <0.00%> (ø)
pydantic/validators.py	100.00% <0.00%> (ø)
pydantic/annotated_types.py	100.00% <0.00%> (ø)
pydantic/_hypothesis_plugin.py	100.00% <0.00%> (ø)
pydantic/__init__.py	100.00% <0.00%> (ø)
pydantic/main.py	99.05% <0.00%> (ø)
pydantic/typing.py	100.00% <0.00%> (+0.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc18f8e...8f5fcee. Read the comment docs.

[samuelcolvin]

This looks like a good start, I'm almost unnerved by how simple the change is.

Are we sure there aren't any edge cases where behaviour has changed?

Has has this had a significant adverse effect on performance?

The other feature which would be cool here would be use of Annotated[], see #2129, e.g. something like:

@validate_arguments
def foo(dt: Annotated[datetime, Field(default_factory=datetime.now)]):
    print(dt)

@JacobHayes might have some input here.

Maybe that's a separate PR, but if it reduced the logic change I would personally prefer to only support Annotated and not Field as a default argument.

[samuelcolvin]

Is it possible this would actually be fixed by #2147?

[JacobHayes]

#2147 doesn't fix it alone - the model gets instantiated, but the __fields_set__ filter removes the defaulted values, causing TypeError: foo() missing 1 required positional argument: 'dt': https://github.com/samuelcolvin/pydantic/blob/8bad7bc91105616dfcc23ce1d68fba12f328a2a3/pydantic/decorator.py#L178-L179

Seems like there could be a few paths, but this small tweak on top of #2147 fixes things for all existing tests + this new one (after moving default value -> Annotated):

diff --git a/pydantic/decorator.py b/pydantic/decorator.py
index 98bc4e2..85dfdd9 100644
--- a/pydantic/decorator.py
+++ b/pydantic/decorator.py
@@ -81,6 +81,7 @@ class ValidatedFunction:
             )
 
         self.raw_function = function
+        self.raw_function_parameters = parameters
         self.arg_mapping: Dict[int, str] = {}
         self.positional_only_args = set()
         self.v_args_name = 'args'
@@ -176,7 +177,9 @@ class ValidatedFunction:
         return values
 
     def execute(self, m: BaseModel) -> Any:
-        d = {k: v for k, v in m._iter() if k in m.__fields_set__}
+        d = {
+            k: v for k, v in m._iter() if k in m.__fields_set__ or (k in self.raw_function_parameters and v is not None)
+        }
         kwargs = d.pop(self.v_kwargs_name, None)
         if kwargs:
             d.update(kwargs)

I'm not sure how solid the k in self.raw_function_parameters and v is not None part is though, particularly if a default_factory may return None or something.

One downside to the Annotated approach is that linters warn about "No value for argument" or "Too few arguments". Assigning ... as the default will satisfy pylint, but not mypy (whereas = Field(...) will "satisfy" mypy with the -> Any return).

[thomascobb]

We've already removed default from Field with the use of Annotated, how about removing default_factory too?

@validate_arguments
def foo(dt: datetime = default_factory(datetime.now)):
    print(dt)

We would still need the Annotated type so we could use the other validation parts of Field:

@validate_arguments
def foo(dt: Annotated[datetime, Field(description="What time is it")] = default_factory(datetime.now)):
    print(dt)

We could define default_factory as:

@dataclass
class DefaultFactory:
    factory: Callable

def default_factory(factory: Callable[[], T]) -> T:
    # We will check for default values of type DefaultFactory in validate_arguments and call it at the right time
    df = DefaultFactory(factory)
    # But lie here so mypy thinks we actually passed a default value of the correct type
    return cast(T, df)

[thomascobb]

@samuelcolvin I'm happy to discard this PR and have a go at making default_factory on top of #2147 if you think that's a good approach?

[samuelcolvin]

Humm, I'm not sure about DefaultFactory, I think Field(..., default_factory=datetime.now) is more explicit and clearer for most people.

I'm not clear whether this PR should remain open or closed in favour of #2147? Or needs to wait for #2147 to be merged?

[thomascobb]

I've rebased on top of master (and so #2147) and this PR now becomes much simpler. Most cases are handled with #2147, e.g. foo(a: Annotated[int, Field(description="Something")] = 34):. We still need a small tweak to support default_factory, which is included in this PR.

The issue with mypy still stands as noted in #2176 (comment)

There are two fixes:

1. Pass Field() as default value e.g. foo(a: int = Field(default_factory=lambda: 99)):
2. Pass something with a matching type or Any in as the default e.g. foo(a: Annotated[int, Field(default_factory=lambda: 99)] = ANY):

Both work with this PR (and are in the test).

@samuelcolvin which of the 2 approaches should I put in the docs?

[samuelcolvin]

This looks great.

@thomascobb, foo(a: int = Field(default_factory=lambda: 99)) would be good in the docs.

[samuelcolvin]

Great, thank you so much. 🎉