Description
openedon Dec 8, 2019
As discussed with the team last week:
In EF6 we had explicit checks for null arguments in all public and internal members, compiling out the checks on internal members for release builds.
In EF Core we stopped doing the checks on internal members. This did not significantly impact our ability to understand the code or debug issues.
Currently, we check for null arguments on an "public" API. "Public" means any member that can be called from code outside of the EF assemblies and isn't marked as "internal". So, members implementing a member of public interface is public. Likewise, protected members are public surface because classes outside of EF can override and call these methods.
However, consider that:
- We missed doing this for the new query pipeline until now.
- This hasn't resulted in a significant impact on our ability to understand or debug issues.
- Even though many of these methods are technically public, in reality they are only called by EF code. For example, consider the many protected methods in visitors.
- Even when they are called externally, this is often by provider code, not application code.
- The "professional experience" observed has not been significantly degraded by people seeing "NullReferenceExceptions" instead of argument exceptions.
- We are moving in the direction of using more static analysis for null-checking. Currently we use attributes, but also in the future we may use nullable reference types.
- This means most of the correctness for nulls is based on static analysis, and the exceptions at runtime have even less value.
Given all this, we discussed changing the default approach for null argument checking:
- We will still use attributes/static analysis everywhere
- We will only check arguments for null in API that is designed to be called from application code