Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evolve null-checking approach #19233

Closed
ajcvickers opened this issue Dec 8, 2019 · 2 comments
Closed

Evolve null-checking approach #19233

ajcvickers opened this issue Dec 8, 2019 · 2 comments
Labels
area-global closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-5.0 type-enhancement
Milestone

Comments

@ajcvickers
Copy link
Member

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
@davidroth
Copy link
Contributor

davidroth commented Dec 10, 2019

Is it planned to replace null attributes with nullable reference types in 5.0 milestone?

@ajcvickers
Copy link
Member Author

@davidroth See #14150

@ajcvickers ajcvickers modified the milestones: 5.0.0, Backlog Jun 9, 2020
@ajcvickers ajcvickers modified the milestones: Backlog, MQ Sep 16, 2020
ajcvickers added a commit that referenced this issue Oct 25, 2021
Part of #19233

Removes a lot of null checks from code that is not intended to be called directly by application code.
ajcvickers added a commit that referenced this issue Oct 28, 2021
Part of #19233

Removes a lot of null checks from code that is not intended to be called directly by application code.
ajcvickers added a commit that referenced this issue Oct 28, 2021
Part of #19233

Quite aggressive, based on not expected to be called by application code.
ajcvickers added a commit that referenced this issue Nov 2, 2021
Part of #19233

Quite aggressive, based on not expected to be called by application code.
ajcvickers added a commit that referenced this issue Nov 2, 2021
ajcvickers added a commit that referenced this issue Nov 2, 2021
ajcvickers added a commit that referenced this issue Nov 4, 2021
ajcvickers added a commit that referenced this issue Nov 4, 2021
ajcvickers added a commit that referenced this issue Nov 4, 2021
ajcvickers added a commit that referenced this issue Nov 5, 2021
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 18, 2021
@ajcvickers ajcvickers modified the milestones: MQ, 7.0.0 Nov 18, 2021
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview1 Feb 14, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview1, 7.0.0 Nov 5, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-global closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. punted-for-5.0 type-enhancement
Projects
None yet
Development

No branches or pull requests

2 participants