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

CA1062 should no longer suggest to use Code Contracts. #3134

Closed
kmgallahan opened this issue Dec 27, 2019 · 5 comments · Fixed by #6327
Closed

CA1062 should no longer suggest to use Code Contracts. #3134

kmgallahan opened this issue Dec 27, 2019 · 5 comments · Fixed by #6327
Assignees
Labels
Bug The product is not behaving according to its current intended design good first issue help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@kmgallahan
Copy link

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

v2.9.8 (Latest)

Diagnostic ID

Example: CA1062

Suggestion

The description currently includes:

If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.

The Code Contract project is no longer being developed, and this rule should not recommend that it be used.

@julealgon
Copy link

Isn't stuff like Contract.Assume(x != null) part of the standard framework now though?

I understand the assembly rewriter and realtime analyzer are completely deprecated, but these libraries that are part of the framework could still be seen as "Code Contracts", couldn't they?

@kmgallahan
Copy link
Author

kmgallahan commented Jan 7, 2020

Without runtime checking, suggesting to -

  • throw an ArgumentNullException
  • add a Code Contract (contract from System.Diagnostics.Contracts, not the named Code Contracts project)

do not have the same result, which is to fix violations as mentioned in the CA1062 docs:

To fix a violation of this rule, validate each reference argument against null.

If the rule read "CA1062: Suggest to callers not to pass a null argument" then it would be fine.

The docs also don't mention Code Contracts at all.

@mavasani mavasani added Area-Microsoft.CodeQuality.Analyzers Bug The product is not behaving according to its current intended design help wanted The issue is up-for-grabs, and can be claimed by commenting labels May 5, 2020
@mavasani mavasani added this to the Unknown milestone May 5, 2020
@Evangelink
Copy link
Member

@kmgallahan Just to confirm I got all of your suggestions right. You'd like to have the doc website updated to also mention System.Diagnostics.Contracts maybe also include an example and then you'd like the message and description of the rule to be updated.

@mavasani Are you fine with these changes?

@kmgallahan
Copy link
Author

kmgallahan commented Jul 13, 2020

@Evangelink @mavasani There are 3 problems:

  1. The rule states: CA1062: Validate arguments of public methods
  2. The Code Contracts project is dead and doesn't work with VS 2017/2019
  3. System.Diagnostics.Contracts requires using the binary rewriter of the Code Contracts project (as mentioned here) to enforce run-time contracts

In my opinion there should be zero suggestions to use a dead project that doesn't work with VS 2017/2019, therefore the rule as it appears when broken in VS should not read:

If appropriate, throw an ArgumentNullException when the argument is null or add a Code Contract precondition asserting non-null argument.

Further, suggesting to use System.Diagnostics.Contracts doesn't seem appropriate either, as it doesn't do anything during run-time.

In my opinion either one of these needs to be done:

A. If the rule documentation continues to mention System.Diagnostics.Contracts, then it needs to be "softened" to read more along the lines of: "CA1062: Suggest to callers not to pass a null argument" (since the contracts don't do anything during run-time)
B. The rule needs to be "hardened" to apply to run-time enforcement by removing all references to the Code Contracts project and System.Diagnostics.Contracts

Bottom line: if the goal is to enforce run-time null checking, then mentioning contracts in any form is not helpful.

@chriskuech
Copy link

+1, I don't think System.Diagnostics.Contracts work to resolve CA1062 (I'm using VS2019), but there isn't documentation for me to know whether this is an analyzer bug or user error. Documentation should be updated to explicitly mention if Contracts can be used, as well as if/how custom Contracts can be written.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug The product is not behaving according to its current intended design good first issue help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants