search

DotNetAnalyzers / NullParameterCheckRefactoring

Null Parameter Check Refactoring for Reference Type Parameters
MIT License
14 stars 4 forks source link

Use Code Contracts when the rewriter is enabled #2

Open sharwell opened 9 years ago

sharwell commented 9 years ago

When the Code Contracts rewriter is enabled (see DotNetAnalyzers/Proposals#1), the code fix behavior should change in two ways:

  1. The added code should be Contract.Requires<ArgumentNullException>(p != null, nameof(p));
  2. The refactoring should only apply to a base method, and not to methods which override a base implementation and/or implement an interface method.
ddur commented 8 years ago

Equality operators can be overridden

  1. The added code should be Contract.Requires<ArgumentNullException>(!object.ReferenceEquals(p, null), nameof(p));
ddur commented 8 years ago

2) Why? Overridden methods can reference invalid argument before (and if) calling base implementation.

sharwell commented 8 years ago

Equality operators can be overridden

In which case, comparison with null is provided by the overridden operator and not (necessarily) by strict reference equality. The form in the original comment remains correct, where the alternate form which explicitly calls ReferenceEquals may or may not be correct.

Why? Overridden methods can reference invalid argument before (and if) calling base implementation.

The code contracts rewriter automatically inserts the precondition checks from base methods into the compiled bytecode of derived methods. In addition, an error is reported if a derived method contains calls to Contract.Requires since derived methods should not be adding new requirements that are not already part of the base contract.

ddur commented 8 years ago

1) IMHO, in this case comparison is explicitly predefined by contract prototype. In order to throw Contract.Requires<ArgumentNullException> argument is expected to be a null, not the overridden value. This contract is provided to prevent <NullReferenceException>, not to obey overridden operator. If I want to let null reference go through the code, then I do not need equality check or contract. For example, I made class where I override equality of empty set and null reference. Still need null reference contract.requires check.

2) True.

BTW, error in derived method can be prevented. Base contract can be wrong/closed code, and must be possible to override. I noticed long time ago that ICollection<T>.CopyTo(array... contract was wrong (it is corrected now)

ddur commented 8 years ago

In one sentence, thinking "out of the box": 1) Throwing <ArgumentNullException> "may or may not be correct".