dotnet / roslyn-analyzers

MIT License
1.59k stars 466 forks source link

CA1062 editorconfig issue with null_check_validation_methods in other project in solution #3451

Open andez2000 opened 4 years ago

andez2000 commented 4 years ago

I have created a small dotnetstandard 2.1 library project in a solution. I want to test out using Nullable Reference Types. As part of this, I want to have appropriate compilation errors and warnings.

TLDR; I want to know how to setup the CA1062 code quality settings in .editorconfig correctly.

Library Project

I have added the following nuget packages to the project:

<ItemGroup>
      <PackageReference Include="Microsoft.CodeAnalysis.Analyzers" Version="2.9.8">
          <PrivateAssets>all</PrivateAssets>
          <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      </PackageReference>
      <PackageReference Include="Microsoft.CodeAnalysis.FxCopAnalyzers" Version="2.9.8">
          <PrivateAssets>all</PrivateAssets>
          <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      </PackageReference>
      <PackageReference Include="Microsoft.CSharp" Version="4.7.0" />
        <PackageReference Include="Ardalis.GuardClauses" Version="1.4.2" />
  </ItemGroup>

This includes the various code analysis packages and also Steve Smith's nice Guard Clauses library.

Nullable Reference Types has been enabled using <Nullable>enable</Nullable> in the project.

And I have a class which in the real world would be a nice implementation that actually did something:

using System;
using MyGuards;

namespace EditorConfigIssues
{
    public static class TestEditorConfig
    {
        public static void TestMethod(MyParam input)
        {
            string x = input.Check;
            Console.WriteLine(x);
        }
    }

    public class MyParam
    {
        public MyParam(string check) => Check = check;

        public string Check { get; }
    }
}

Guard Library Project

In the solution I have added a simple Guard library which is another dotnetstandard 2.1 project.

using System;

namespace MyGuards
{
    public static class FakeGuard
    {
        public static void Validate(object obj)
        {
            if (obj == null)
                throw new ArgumentNullException();
        }
    }
}

NOTE: This is not in competition of the GuardClauses library - just using to contrast editorconfig with!

.editorconfig

I have added an .editorconfig to the root of solution with the following diagnostic checks:

dotnet_diagnostic.CA1062.severity = error
dotnet_code_quality.CA1062.exclude_extension_method_this_parameter = true

So with this in place, when I compile I get the following: Errors

So everything is as I expect so far. I am not using any guards yet.

Fixing it up - Steve Smith's Guard Library

So lets try and implement the guard clauses from Steve Smiths Guard Library to get rid of the error.

So we add the following to TestEditorConfig.TestMethod:

Guard.Against.Null(input, nameof(input));

and tweak the .editorconfig with:

dotnet_code_quality.CA1062.null_check_validation_methods = Ardalis.GuardClauses.Guard.Against.Null

Excellent, all is good. The error has disappeared. Initial error gone

Fixing it up - my own guard library

But now I want to use my own guard. So we replace the initial guard check in TestEditorConfig.TestMethod with:

FakeGuard.Validate(input);

and replace the null_check_validation_methods in .editorconfig with:

dotnet_code_quality.CA1062.null_check_validation_methods = FakeGuard.Validate

The error is now back.
enter image description here

Question(s)

  1. The question is, what do I need in order to use a project with guards from the same solution?
  2. Why am I getting warnings for the other CA1062 in the Error Window?
    The keyword "dotnet_code_quality.CA1062.exclude_extension_method_this_parameter" is unknown
    The keyword "dotnet_code_quality.CA1062.null_check_validation_methods" is unknown

I have been checking out this link MS Docs Code Quality and tried various combinations for the FakeGuard, including:

Curiously, in a different solution, then I don't get any complaints about the CA1062 editorconfig settings in the Error Window. And in there I have been unable to get the null_check_validation_methods working - hence putting together this solution. This has been bugging me for a month or two, but finally getting the energy to put things together at the moment.

EditorConfig Bug?

If I copy the FakeGuard file to the Library project, then the error message disappears. But why does this not work in a different project in the solution.

andez2000 commented 4 years ago

Checking whether this is a duplicate of https://github.com/dotnet/roslyn-analyzers/issues/2578 which has own unit test to cover this scenario.

mavasani commented 4 years ago

@andez2000

dotnet_code_quality.CA1062.null_check_validation_methods = FakeGuard.Validate

Can you instead try specifying the option value as a full documentation comment ID, as explained here: https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#null-check-validation-methods?

  1. Enable XML doc comments for the project containing FakeGuard
  2. Build project and open the XML doc file to find the documentation comment ID for FakeGuard.Validate method, it should start with M: in the doc file.
  3. Supply the option as dotnet_code_quality.CA1062.null_check_validation_methods = <%doc_comment_id%>

If the issue still repros, can you provide a small snippet of your FakeGuard definition?

andez2000 commented 4 years ago

I tried the above and the error still appears. I tried copying various fragments of the doc comment id.

xml doc snippet:

<member name="M:MyGuards.FakeGuard.Validate(System.Object)">

FakeGuard

using System;

namespace MyGuards
{
    /// <summary>
    /// Checks stuff.
    /// </summary>
    public static class FakeGuard
    {
        /// <summary>
        /// No more nulls.
        /// </summary>
        /// <param name="obj"></param>
        public static void Validate(object obj)
        {
            if (obj == null)
                throw new ArgumentNullException();
        }
    }
}

.editorconfig

dotnet_diagnostic.CA1062.severity = error
dotnet_code_quality.CA1062.exclude_extension_method_this_parameter = true
dotnet_code_quality.CA1062.null_check_validation_methods = M:MyGuards.FakeGuard.Validate(System.Object)

usage

using System;
using MyGuards;

namespace EditorConfigIssues
{
    public static class TestEditorConfig
    {
        public static void TestMethod(MyParam input)
        {
            FakeGuard.Validate(input);

            string x = input.Check;
            Console.WriteLine(x);
        }
    }

    public class MyParam
    {
        public MyParam(string check)
        {
            Check = check;
        }

        public string Check { get; }
    }
}

NOTE: It did seem to work when I copied the FakeGuard class to the project that uses the FakeGuard class.

mavasani commented 4 years ago

Great, thanks. Looks like a bug. Maybe you can workaround meanwhile using ValidatedNotNullAttribute: https://github.com/dotnet/roslyn-analyzers/blob/d02078fe8f2a1051d41b9c5cd6899b2ef6d41649/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs#L2016-L2032

mavasani commented 4 years ago
using System;

[AttributeUsage(AttributeTargets.Parameter)] 
internal sealed class ValidatedNotNullAttribute : Attribute { } 

namespace MyGuards
{
    /// <summary>
    /// Checks stuff.
    /// </summary>
    public static class FakeGuard
    {
        /// <summary>
        /// No more nulls.
        /// </summary>
        /// <param name="obj"></param>
        public static void Validate([ValidatedNotNull] object obj)
        {
            if (obj == null)
                throw new ArgumentNullException();
        }
    }
}
andez2000 commented 4 years ago

OK thanks... The ValidatedNotNullAttribute seems to work. The error has now gone thanks.

Evangelink commented 4 years ago

@mavasani Reading this thread I think that everything is now handled and so the ticket can be closed.

mavasani commented 4 years ago

@Evangelink The workaround of using ValidatedNotNullAttribute works, but we need to ensure the editorconfig based configuration also works.

SebastianSchumann commented 3 years ago

I still have the problem using VS 16.8.3 with Microsoft.CodeAnalysis.NetAnalyzers 5.0.1. The build error list does not list this error but if I open a file that contains such a null-guard test method the error is still reported as an error.

lonix1 commented 3 years ago

Hi @mavasani

I read the docs and docs.

The workaround above won't work when the validator is in a non-static class. A more sophisticated approach to validation would not use one mega static class, so you'd have something similar to:

[AttributeUsage(AttributeTargets.Method)]                        // not I changed it from Parameter to Method
internal sealed class ValidatedNotNullAttribute : Attribute { }

public class Validator<T> {
  public Validator(T value) => Value = value;
  protected readonly T Value;
  [ValidatedNotNull]public virtual void NotNull() { if (Value == null) throw new ArgumentNullException(); }
}

public class StringValidator : Validator<string> {
  public StringValidator(string value) : base(value) { }
  // validation methods for not empty / contains / shorter than / etc...
}

public static class ValidatorExtensions {
  public static StringValidator Validate(this string value) => new StringValidator(value);
  public static IntValidator Validate(this int value) => new IntValidator(value);
  public static FooValidator Validate(this Foo value) => new FooValidator(value);
}

Used like this:

var value = "foo";
value.Validate().NotNull().NotEmpty().IsWhatever();

This design allows for chaining validators, error messages, customizing exceptions, etc.

I tried all the following in my .editorconfig:

dotnet_code_quality.CA1062.null_check_validation_methods = NotNull
dotnet_code_quality.CA1062.null_check_validation_methods = Validator<T>.NotNull
dotnet_code_quality.CA1062.null_check_validation_methods = Validator<object>.NotNull
dotnet_code_quality.CA1062.null_check_validation_methods = Validator<string>.NotNull
dotnet_code_quality.CA1062.null_check_validation_methods = T:ValidatorExtensions.Value`1().NotNull()

But the real validation method is on an instance (Validator<T>.NotNull()) returned by a static method (Validate()) - and the analyzer doesn't dig so deep.

This bug is really bad, as editorconfig is now the standard, and there's no way to deal with these warnings other than to turn them off completely (bad idea!)

In the meantime is there a workaround for this use case? Thanks!

andez2000 commented 3 years ago

I thought the ValidatedNotNull attribute had to be applied to public parameters to methods.

This rule can lead to false positives if your code calls special null-check validation methods in referenced libraries or projects. You can avoid these false positives by specifying the name or signature of null-check validation methods.

The analysis assumes that arguments passed to these methods are non-null after the call.

This attribute works well with ArgGuards.

I did go the chain route based on the above library. Perhaps try:

public static StringValidator Validate([ValidatedNotNull] this string value) => new StringValidator(value);
...

public class Validator<T> {
  public Validator([ValidatedNotNull] T value) => Value = value;
  protected readonly T Value;
  public virtual void NotNull() { if (Value == null) throw new ArgumentNullException(); }
}

public class StringValidator : Validator<string> {
  public StringValidator([ValidatedNotNull] string value) : base(value) { }
}

I think the IntValidator extending the Validator is a wrong abstraction as int's are your primitive value types so NotNull does not fit here (perhaps NotDefault?).

lonix1 commented 3 years ago

@andez2000 Thanks that for that nice idea! :-) Unfortunately it doesn't work:

// we use your approach, add [ValidatedNotNull] here:
public static StringValidator Validate([ValidatedNotNull] this string value) => new StringValidator(value);

// ...then:
var value = "foo";
value
  .Validate()            // tells analyzer "it's not null" so shut up
  .NotNull()             // but null check actually happens here, and developer can forget to do this
  .NotEmpty()
  .IsWhatever();

Regarding the int validator stuff, it was just an example (it doesn't do null checks, it has validations notgt, gt, gte, etc.)

TLDR: We need a robust approach for case where null checking is not part of a mega static class.

owns commented 3 years ago

using the full documentation comment ID worked for me: M:MyNamespace.GuardAgainst.Null``1(``0,System.String)

  1. Find the dll and corresponding xml documentation. Mine was a nuget package, so I just drug around my nuget cache (e.g. ~/.nuget/package/MyPackage/MyVersion/lib/MyTargetFramework/MyDll.xml) The implementation:
    public class GuardAgainst
    {
    public static T Null<T>(T? value, string paramName = "Value")
    {
        ...
    }
    }