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

remove Debug.Assert null checks #6101

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SimonCropp
Copy link
Contributor

Changes

So i suspect the Debug.Assert null checks pre-date nullability. IMO with nullability they are redundant. essentialy for public APIs we should guard agains null with ArgumentNullExceptions, for non public APIs we should rely on the compiler for nullability checks

this also has the side effect of allow us to remove 100+ null suppressions. the reason being that in a debug build, if u do a Debug.Assert(variable != null then the compiler wants u to suppress nulls for all all subsequent usages of that variable

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@SimonCropp SimonCropp requested a review from a team as a code owner January 24, 2025 11:53
@github-actions github-actions bot added documentation Documentation related pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Zipkin Issues related to OpenTelemetry.Exporter.Zipkin NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package labels Jan 24, 2025
@github-actions github-actions bot added the pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package label Jan 24, 2025
@CodeBlanch
Copy link
Member

I don't agree with this direction. Always best practice to validate your assumptions! A simple ! voids all nullable analysis 🤣 ❤️ and thanks for trying to improve things @SimonCropp but there won't be an approval from me on this one.

@SimonCropp
Copy link
Contributor Author

but those assumptions are validated by the compiler

also the debug check for null pattern is not consistently applied the majority of internal methods do not apply this pattern. and many methods only apply it on a subset of parameters

@SimonCropp
Copy link
Contributor Author

also the “always validate your assumptions” argument doesn’t hold water when u consider we are only validating in a debug build. none of the code i removed has any effect on the deployed release build.

lets take the scenario where one of the nullability assumptions is incorrect (ie the compiler has a bug or code is being executed with relection). we will only get the null check behaviour in a local debug build when running tests. not in CI or prod.

now if we remove the debug checks. the result in a local debug test run will be a nice stack trace with a line number pointing to the null variable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Exporter.Console Issues related to OpenTelemetry.Exporter.Console NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry.Exporter.Prometheus.AspNetCore Issues related to OpenTelemetry.Exporter.Prometheus.AspNetCore NuGet package pkg:OpenTelemetry.Exporter.Zipkin Issues related to OpenTelemetry.Exporter.Zipkin NuGet package pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants