Skip to content

Enable CA1062 (validate arguments of public methods) to catch missing null-argument guards at build time #199

Description

@daviburg

Summary

Enable CA1062 ("Validate arguments of externally visible methods") as a build-enforced analyzer rule so missing null-argument guards on public/protected APIs are caught deterministically at build time instead of ad hoc during code review.

Motivation

Missing ArgumentNullException guards on public entry points are a recurring manual review nit (including from the Copilot PR reviewer). Each occurrence currently costs a review round-trip: reviewer spots it, author adds ArgumentNullException.ThrowIfNull(...), re-review. CA1062 catches the entire class at compile time, consistently, before a PR is ever opened.

This rule is disabled by default in the SDK's current analysis mode (AnalysisMode=Default), so it never fires today even though EnableNETAnalyzers=true and TreatWarningsAsErrors=true are set in eng/build/Engineering.props.

Evidence (from a local AnalysisMode=All build)

CA1062 fires on hand-written production code that skips argument validation, e.g. in ConnectorClientBase:

  • CallConnectorAsync<TResponse>(HttpMethod method, ...)method not validated before use.
  • CallConnectorAsync(HttpMethod method, ...) (no-body overload) — same.

New code such as ConnectorTriggerPayload already validates via ArgumentNullException.ThrowIfNull, so it is clean — which shows the rule aligns with the direction the codebase is already moving.

Scope / considerations

  • Generated code is in scope. The generated clients are produced by our own CodefulSdkGenerator (BPM repo) and are a core part of the value proposition; they must not ship missing guards. Enabling CA1062 will surface any generated entry points lacking validation — if it flags generated code, the fix belongs in the generator's emit templates, not a per-file suppression.
  • Severity: start as warning (which TreatWarningsAsErrors escalates to an error) or suggestion for a soft rollout, then promote.
  • Rollout: enable via .editorconfig (dotnet_diagnostic.CA1062.severity = warning) rather than raising the whole AnalysisMode, to keep the change targeted and avoid the ~17k unrelated diagnostics that AnalysisMode=All produces.
  • Note: CA1062 has known limitations (it may not track validation performed in a called helper), so expect a few false positives to suppress with justification.

Acceptance criteria

  • dotnet_diagnostic.CA1062.severity set in .editorconfig.
  • Existing hand-written violations fixed (add ArgumentNullException.ThrowIfNull).
  • Generated-code violations (if any) fixed at the generator, or a documented, justified suppression policy agreed.
  • Build stays green with the rule enforced.

Filed from analysis on PR #196; not part of that PR's scope.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions