bkoelman / ResharperCodeContractNullability

Reports diagnostics, helping you to annotate your source tree with (Item)NotNull / (Item)CanBeNull attributes.
https://www.jetbrains.com/resharper/help/Code_Analysis__Code_Annotations.html
Apache License 2.0
26 stars 4 forks source link

Warning where nullability annotation would be redundant (Nullable<T>) #21

Closed starkcolin closed 8 years ago

starkcolin commented 8 years ago

A nullability warning (RNUL) on an object of type Nullable is redundant.

This is consistent with ReSharper's own nullability analysis - there is no context menu command available to annotate a Nullable with either NotNull or CanBeNull.

Something like ((int?)null).ToString() will not throw a NullReferenceException, so there is no benefit to having a CanBeNull annotation. A NotNull annotation does not seem logical either on a variable of type int?.

Unless there is a specific benefit to having a NotNull/CanBeNull annotation on a Nullable that I'm not thinking of, I propose that the warning should not be displayed for objects of this type.

bkoelman commented 8 years ago

Thanks for bringing this to my attention.

Resharper runs its nullability analysis engine to determine whether to suggest the "Not Null" and "Can Be Null" context menu options. Part of that analysis is looking into the target method call stack, to see if any code paths exist that return null. That makes it much smarter than suggestions from ResharperCodeContractNullability. Resharper's engine stops at assembly boundaries. To get stable results, you need to add a class library to your project to get them. In this case, however, they are never offered for nullable value types.

Although ((int?)null).ToString() will not throw, someNullable.Value will. A warning in such cases is still valuable.

It may be helpful when overriding a third party class that returns a nullable value, to annotate that with [NotNull] if the overriden behavior never returns null.

My testing shows that adding [NotNull] produces extra checks. For example, when the implementation of a method returns null while it has a [NotNull] return annotation:

image

Furthermore, it looks like [CanBeNull] on a nullable return value has the same effect as omitting the attribute.

image

But... adding NotNull is not entirely consistent. Resharper warns on a null comparison only in the first case:

image

So many inconsistencies.... I'm not sure what the right approach is here.

starkcolin commented 8 years ago

Seeing as ReSharper seems to treat a missing annotation in the same way as [CanBeNull], it seems unnecessary to warn the user that [CanBeNull] is missing.

Personally, I think that a [NotNull] annotation on a nullable value type is an uncommon enough use case that users don't need to be warned if [NotNull]/[CanBeNull] are missing on nullable value types. I think it should generally be assumed that a nullable value type is implied to have a [CanBeNull] annotation.

However, you could make it optional to display warnings on nullable value types so that each user can decide which case is best for them.

bkoelman commented 8 years ago

@starkcolin See the linked PR for info how to suppress warnings on nullable value types. Feel free to check if this meets your expectations by installing the latest MyGet prerelease build.

bkoelman commented 8 years ago

This fix is included in https://github.com/bkoelman/ResharperCodeContractNullability/releases/tag/v1.0.5.

starkcolin commented 8 years ago

This fix works, but only after setting the build action of ResharperCodeContractNullability.config to AdditionalFiles, a step which must be done manually.

By default, the Build Action is set to None, and it has no effect. Applying the fix when the file already exists, but does not have its Build Action set, results in the creation of ResharperCodeContractNullability.config.1, and then ResharperCodeContractNullability.config.2, etc.

Is it possible to set the Build Action when creating the file during the fix?

bkoelman commented 8 years ago

Yes, I know. That's why there's a "Important: manual steps needed" help text. There is currently no working API to do that automatically. You can track the associated Roslyn bug in the PR if you are interested. Op 3 mrt. 2016 01:52 schreef "starkcolin" notifications@github.com:

This fix works, but only after setting the build action of ResharperCodeContractNullability.config to AdditionalFiles, a step which must be done manually.

By default, the Build Action is set to None, and it has no effect. Applying the fix when the file already exists, but does not have its Build Action set, results in the creation of ResharperCodeContractNullability.config.1, and then ResharperCodeContractNullability.config.2, etc.

Is it possible to set the Build Action when creating the file during the fix?

— Reply to this email directly or view it on GitHub https://github.com/bkoelman/ResharperCodeContractNullability/issues/21#issuecomment-191515439 .