NS2001: Is an accessible constructor strictly necessary?

[stogle]

In my unit test I am creating a substitute for this third-party public abstract class, which has only an empty internal constructor. I am getting an NS2001 warning because the constructor is not accessible from the test, however the substitute and my test appear to work as intended. All my test does with the substitute is pass it to my method-under-test, then manually check its ReceivedCalls() list. It does not set any return values or anything else. (I can't share my actual test for IP reasons, but I can create a minimal example if that would help.)

I understand the problems associated with substituting for classes rather than interfaces, and that non-virtual code is executed during my test. What I'm not clear on is the specific problems with violating NS2001 by substituting for a class with an inaccessible constructor. The rule says that to fix this violation I should add an accessible constructor to the type, but I can't do that because I don't own the type. Furthermore, NSubstitute is still able to create a substitute without an accessible constructor (presumably via reflection).

Could the documentation for the rule expand on why an accessible constructor is important and what happens when you create a substitute for a class without one?

[tpodolak]

Hi @stogle

Is an accessible constructor strictly necessary

Yes it is. Here is a minimal repro showing that NSubstitute will fail to create substitute for class with only internal ctor

public class Test
{
    [Fact]
    public void Foo()
    {
        var substitute = Substitute.For<MyAbstractClass>();
        _ = substitute;
    }
}

public abstract class MyAbstractClass
{
    internal MyAbstractClass()
    {
    }
}

When it comes to your case, it looks like Avalonia marks their assembly with [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2") attribute basically making iternal classes and constructors visible to Castle.Core(which is used by NSubstitute to create proxy classes) - thats why NSubstitute is able to instantiate a substitute. However analyzers were not able to find this annotation(even though we check for that attribute being applied to assembly) - thats why you see the warning. I will take a look if thats a bug on our end, or if for whatever reasons Roslyn is not able to find these annotations

[stogle]

Hi @tpodolak, thanks for the answer! I did not think to look for the InternalsVisibleTo attribute, but that makes total sense.

I will take a look if thats a bug on our end, or if for whatever reasons Roslyn is not able to find these annotations

Either way, I now have a proper justification for disabling the warning in this case. Thanks again.

[tpodolak]

Just did a quick check, and Roslyn doesnt return any constructors for this particular type. However this doesnt seem to be Roslyn bug, but rather some sort of compilation option of Avalonia. Internals members visibility seems to be disabled

so even though ctor can be found in the metadata

it will not surface as part of GetMembers call as

if (isOrdinaryEmbeddableStruct || module.ShouldImportMethod(methodHandle, moduleSymbol.ImportOptions))

returns false

[stogle]

I'm not that familiar with Roslyn or writing analyzers, but I think what you are saying is that NSubstitute can see this constructor in Avalonia.Base.dll at runtime, but NSubstitute.Analyzers can't see it during analysis, even though it normally sees internal constructors on other assemblies that use InternalsVisibleTo. Is that it in a nutshell?

If it would help, I could create an Issue or a Discussion in the Avalonia repository about this. I'm just not sure what to say.

Still, using a #pragma to disable this particular warning isn't a big deal for me, as long as I have a justification.