Preserve exception instances and their context in ValidationError

[adriangb]

Fixes pydantic/pydantic#6504

Selected Reviewer: @davidhewitt

[adriangb]

please review cc @lig

[codecov]

Codecov Report

Merging #753 (daa5854) into main (fac2a40) will increase coverage by 0.01%.
The diff coverage is 94.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #753      +/-   ##
==========================================
+ Coverage   93.64%   93.65%   +0.01%     
==========================================
  Files          99       99              
  Lines       13929    13970      +41     
  Branches       25       25              
==========================================
+ Hits        13044    13084      +40     
- Misses        879      880       +1     
  Partials        6        6              

Impacted Files	Coverage Δ
python/pydantic_core/core_schema.py	96.64% <ø> (ø)
src/recursion_guard.rs	95.65% <ø> (ø)
src/validators/function.rs	91.36% <ø> (ø)
src/errors/types.rs	84.30% <81.25%> (+0.24%)	⬆️
src/errors/validation_exception.rs	94.10% <100.00%> (+0.08%)	⬆️
src/lib.rs	100.00% <100.00%> (ø)

---

Continue to review full report in Codecov by Sentry.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #753 will degrade performances by 60.91%

_{Comparing preserve-exceptions (daa5854) with main (964066b)}

Summary

❌ 2 regressions
✅ 124 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	main	preserve-exceptions	Change
❌	test_definition_model_core	1.8 ms	4.6 ms	-60.91%
❌	test_field_function_validator	789.9 µs	1,840.2 µs	-57.07%

[adriangb]

maybe @samuelcolvin if you want to review since David is out today

[lig]

Thanks! I like the repr tests. So, replacing string in error with an exception instance should break anything in Pydantic.

Still, we should test this with Pydantic tests before merging, I think.

[adriangb]

Agreed, we'll have to update some tests, looks like it's about 20 it shouldn't be too bad.

[davidhewitt]

I think the Option<PyObject> feels like a shortcut for the sake of default that has created a little bit of pain in the rest of the implementation.

I'd like to suggest having the following type:

struct InnerError(PyErr);

// to use for the error "context" default in EnumIter
impl Default for InnerError {
    fn default() -> Self {
        Self(PyException::new_err(()))
    }
}

impl ToPyObject for InnerError {
    fn to_object(&self, py: Python<'_>) -> PyObject {
        self.0.to_object(py)
    }
}

If you use that instead of error: Option<PyObject>, I think the rest of the code will come out cleaner.

[adriangb]

Is that really better? The default seems to be just as meaningless as Option. It's also not a trivial swap, it needs to implement Clone and since InnerError is not wrapped by a Py that's not derivable (I think).

[davidhewitt]

Ah, I'd missed the need for a Clone implementation too. Could be this:

impl Clone for InnerError {
    fn clone(&self) -> Self {
        Python::with_gil(|py| Self(self.0.clone_ref(py)))
    }
}

However I take your point that this is now also quite verbose.

The default seems to be just as meaningless as Option.

At the moment I believe the example_context for these errors will now be {"error": None}. I admit I don't know what the use case of example_context is. Up to you whether None or Exception() makes more sense there.

not necessary to make these changes

[adriangb]

Well, in theory, I think the default option for these members of the enum will never be exposed to users. If we had a bug and they did, I don’t think one of the other would be more meaningful to a user experiencing the bug. Neither would crash or anything like that. Obviously, the better option would be to prove to the compiler that this is the case but I imagine that would require extensive refactoring.

[adriangb]

please review

[adriangb]

Looks like I need to sync up with main, I’ll do that later today but let me know if everything else looks good @davidhewitt so I can handle that async

[davidhewitt]

LGTM 👍