Do not offer to annotate derived members

[tom-englert]

Having different annotations on derived members (interface implementations or abstract overrides) will introduce Liskov violations:

• Preconditions cannot be strengthened in a subtype.
• Postconditions cannot be weakened in a subtype.

See also:

• https://youtrack.jetbrains.com/issue/RSRP-463892
• https://www.jetbrains.com/help/resharper/Code_Analysis__Value_Analysis.html

[bkoelman]

Hi @tom-englert, thanks for pointing this out. I agree that having an annotation on a derived member that is different from the annotation on the base member or interface violates Liskov.

ResharperCodeContractNullability just tries to match with how Resharper works: Putting an annotation only on the derived member is okay, but putting the same annotation on the base member causes Resharper to gray out the annotation on the derived member as unneeded. In that case, ResharperCodeContractNullability does not suggest to annotate the derived member anymore.

Please keep in mind that the base class or interface may be declared in an external assembly or another project. So I believe it's still valuable that ResharperCodeContractNullability suggests to annotate a derived member. If you do not want that, you can just annotate the base member instead and the warning goes away.

Based on the above, what change in behavior of ResharperCodeContractNullability would you like to see?

[tom-englert]

Hi @bkoelman, the problem here is that R# has the same bug.

Annotations are sole to support the developer generating fail save code. Any false positive here is extra dangerous, because it makes you believe everything is fine - but in reality it's buggy.

I see the point where the base member is declared in an external assembly, but in this case I would rather favor to suppress the warning in R#, than to add a dangerous annotation. The only robust solution would be to generate an external annotation for the external item (which would be a really cool feature!)

The problem gets even worse if you try to use bulk operations. E.g. I have all my code annotated with [NotNull], based on the analysis of CodeContracts, and now wanted to complement all unannotated items with [CanBeNull]. Since this generates thousands of entries at once, it's hard to find the false positives by manual review.

If the source of the interface is reachable, I would recommend to not offer any code fix on the derived member at all.

If you don't want to drop this for base members in external assemblies, maybe grouping this kind of code fix under a different diagnostic id with a lower severity and some extra comment in the description about the danger behind this annotation would solve this.

[bkoelman]

Thanks for the clarification. Based on your scenario I think the next two additions in ResharperCodeContractNullability would help you:

1. Add a new analyzer that reports annotation mismatches in the type hierarchy (Liskov).
2. Add a configuration switch (similar to https://github.com/bkoelman/ResharperCodeContractNullability/pull/23) that enables you to opt-in to suppress reporting in case a non-annotated base type is defined in source. "Source" here means in the same project or via a project-to-project reference in the same solution.

About external annotations: Writing those files is not the problem. ResharperCodeContractNullability already parses them. The tricky part is that Resharper requires them to be in the same directory as the external assembly. In case of a NuGet reference, that would be in the packages directory. Letting ResharperCodeContractNullability write xml files there is not very helpful, as the packages directory is usually not under source control. And in VS2017, packages are stored in the %USERPROFILE% directory, even more away from the solution. For GAC'ed assemblies, I don't think that even works. The external annotations for the .NET Framework are stored in the Resharper installation directory, also away from the solution thus not under source control.

Based on this, I feel that creating (and packaging) external annotations should be up to the developer. For example, by creating a post-build event that copies your own external annotations to a predefined location. But once they are placed side-by-side to external assemblies, the current version of ResharperCodeContractNullability already picks them up.

In case it helps: when you start Resharper in experimental mode it provides a menu option that enables you to generate external annotation files.

[tom-englert]

Configurable sounds good.

Probably 1. and 2. are related, i.e. 1. only makes sense if it's not offered by 2 - else you get ping-pong-effect.

For 2. IMO it also makes sense to not suggest annotations if the base member is declared in an external assembly. The main benefit of the annotations is on the callers side - knowing what needs to be passes to a method is essential. For the callee it's essential to know what the caller sees to make a proper implementation. If the caller sees an interface without any contract, it does not help if the callee decorates the arguments of the implementation with [NotNull], making himself believe having done a robust implementation. However [CanBeNull] is always safe.

External annotations: I agree, adding annotations for 3rd party libs is only a workaround. I played around storing some missing stuff in %localappdata%\JetBrains\plugins\JetBrains.ExternalAnnotations.10.2.15\DotFiles\ExternalAnnotations - This works fine, but only until the next update of R#, so nothing that would work out of the box from within a tool. However one could upload this to https://github.com/JetBrains/ExternalAnnotations.

Anyhow the most annoying thing is the tremendous bad quality of R#'s external annotations of the CLR, where 30% are just wrong, and none cares to fix that. In some cases that's even worse than having nothing.

[bkoelman]

ResharperCodeContractNullability has no understanding of implicit nullability (via optimistic and pessimistic modes). Also it cannot infer nullability, like Resharper does. It only knows about explicit annotations, either via base members or external annotations. Therefore 1 and 2 are unrelated.

For 2, I believe in some cases it makes sense to annotate members that have their ultimate base type defined externally. For example, suppose you implement an interface from an external assembly, then declare an abstract type and a derived type. Now would you like to get a suggestion to annotate the base member? It all depends on whether callers will use the interface or the base type. Yes this violates Liskov, but can be a reasonable compromise in a non-perfect world. Of course if you want to go the long way and start by annotating everything external first, then you would answer No. But not everybody likes to go that long way.

Let's look at an example:

// External.dll
interface I
{
    string Get();
}

// your code
public abstract class Base : I
{
    public abstract string Get();
}

public sealed class Derived : Base
{
    public override string Get()
    {
        throw new NotImplementedException();
    }
}

Now I image having a configuration setting named DisableReportInTypeHierarchies with three modes:

• None (default): reports on Base.Get and Derived.Get
• SuppressWhenBaseExistsInSource: reports only on Base.Get
• SuppressWhenBaseExistsInSourceOrAssembly: reports nowhere in the example above

External annotations: Note that you can distribute external annotations, via Resharper plugins. See here.

External annotations on the BCL are hard to get right. For example, technically Path.GetDirectoryName returns null when only a filename is passed in. However that's so uncommon that it becomes annoying that every call to extract a directory name results in "possible null reference". I've seen the annotations on such APIs change in the past, trying to balance between technical correctness and usability.

[tom-englert]

What I meant by 1 and 2 are related: 1 reports a mismatch and you remove the annotation, then 2 comes in and prompts you to add an annotation, and after adding the annotation 1 complains again - so you have a ping-pong of warning you can never get rid of.

Of course there might be cases when you want to annotate your derived members like in your sample above. However doing this would require extra thinking and probably extra documentation, so it's better not to force it by default, but leave it up to the developer to do it manual, since else this would easily trap the unexperienced, who maybe never heard of Liskov and might need some guidance.

So personally I would favor the 3rd option to be default, and leave the other options to the experts, since it does not hinder you annotating derived members, but keeps you save by not forcing you to do it.

[bkoelman]

When 1 reports a mismatch, I would expect users to either toggle the annotation or suppress the warning. But... it turns out that Resharper itself already reports mismatches in the type hierachy (for an example, see https://github.com/bkoelman/DogAgilityCompetitionManagement/blob/e03edd3aa9dc378eacc64c1a8b490cef33af1405/src/Circe/Session/FreshNotNullableReference.cs#L20).

The primary goal of ResharperCodeContractNullability is to indicate potential locations where placing an annotation helps the nullability engine of Resharper. Then if a developer decides that annotating a specific location is undesired, the warning can be suppressed from the context-menu. This workflow guides the developer in getting as much annotated as possible, while being able to opt-out.

In order to mass-annotate an existing codebase, I understand it helps to (temporarily) turn off reporting on derived members until all base members are annotated (to prevent duplicate work). A context-menu action can guide the user into this. Not reporting some locations by default is a breaking change and violates the purpose of ResharperCodeContractNullability. And I don't see how unexperienced users can be notified that some parts of the codebase still need to be annotated if by default we do not report all potential locations.

[tom-englert]

I just wanted to help making a good tool even a little bit better :smile: And I believe you got the essential points and will make the best out of this discussion :+1:

[bkoelman]

Thanks for the compliment! I appreciate you are taking the time to provide feedback.

I've started work at https://github.com/bkoelman/ResharperCodeContractNullability/tree/basetypes. The analysis engine now respects the different modes, it just takes some manual steps to setup configuration because that is not yet implemented.

I'd love to hear if this works for you. To give it a try, switch to the branch and build from source. Then run and load your project. Add the following file named ResharperCodeContractNullability.config:

<?xml version="1.0" encoding="utf-8"?>
<AnalyzerSettings xmlns:i="http://www.w3.org/2001/XMLSchema-instance" xmlns="ResharperCodeContractNullabilitySettings">
  <disableReportOnNullableValueTypes>false</disableReportOnNullableValueTypes>

  <!-- choose from: Always / AtHighestSourceInTypeHierarchy / AtTopInTypeHierarchy -->
  <typeHierarchyReportMode>AtTopInTypeHierarchy</typeHierarchyReportMode>
</AnalyzerSettings>

Next, edit the project file to change:

<None Include="ResharperCodeContractNullability.config" />

Into:

<AdditionalFiles Include="ResharperCodeContractNullability.config" />

If something works other than expected, it may help to review the tests in https://github.com/bkoelman/ResharperCodeContractNullability/commit/9c9544e1ed20321d206b75d1ba9fc9756c55d2b8.

[bkoelman]

@tom-englert I did some more testing. Consider the following: A type higher in the type hierarchy is annotated with CompilerGenerated or be defined in another project that does not reference/define the nullability attributes, or be defined in a project that currently does not compile. These are all reasons for ResharperCodeContractNullability to keep silent at that symbol. In the new scenario's that means nothing in the whole type hierarchy gets reported, which feels wrong to me. Adding extra checks may help, but I'm a bit worried about performance. What behavior would you expect in such cases?

[tom-englert]

@bkoelman having no annotations in that cases feels correct to me - it shows exactly the truth, that there is no information about nullability available, so you know you have to be careful. (annotating derived types different than the base type would be alternative truth ;-)

It's the same about using external methods or properties without annotations - which happens much, much more often than using a derived type - either you suppress R#s warnings or assume the worst and check everything for null, but you can't fix it with own annotations.

[tom-englert]

@bkoelman finally I found some time to test the new feature, using AtTopInTypeHierarchy, and I think it works really great! :+1:

[bkoelman]

I guess you noticed that settings can be shared in a solution using Include="..\..\" in the project files. Having an ultimate fallback to %PROGRAMDATA% in the vsix version might be nice, though.

[tom-englert]

Everything is possible when leveraging all features of MSBuild:

I have added all to a .targets file in C:\Program Files (x86)\MSBuild\15.0\Microsoft.Common.Targets\ImportAfter:

<?xml version="1.0" encoding="utf-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <Analyzer Include="D:\Develop\...\bin\Debug\CodeContractNullability.dll" />
    <Analyzer Include="D:\Develop\...\bin\Debug\MsgPack.dll" />
  </ItemGroup>
  <ItemGroup>
    <AdditionalFiles Include="D:\Develop\...\ResharperCodeContractNullability.config"/>
  </ItemGroup>
</Project>

Works great, instantly available in every solution, but without the drawbacks of the VSIX, and without modification of any project.

One could do the same with an "after.{solution name}.sln.targets" in the solution directory to enable just for one solution.

[tom-englert]

R# annotations have been superseded by nullable reference types.