This project is read-only.

Enable SuppressMessageAttribute to be used to explicitly exclude verification warnings

Apr 20, 2014 at 12:09 PM
Edited Apr 20, 2014 at 12:12 PM
Tools such as StyleCop allow explicitly suppressing known and approved violations using SuppressMessageAttribute.

I think we may have discussed this in the past but what are you thoughts on extending the Diagnostic Services to take this metadata into account?
E.g.
public class ObjectMaterializedSubscriber : IObjectMaterializedSubscriber
{
    private readonly IEventPublisher eventPublisher;

    [SuppressMessage(
        "Potential Lifestyle Mismatches", 
        "IObjectMaterializedSubscriber", 
        Target = "IEventPublisher eventPublisher",
        Justification = "This is fine although I could make it a Func<IEventPublisher> to avoid the warning.")]
    public ObjectMaterializedSubscriber(IEventPublisher eventPublisher)
    {
        this.eventPublisher = eventPublisher;
    }

    public void Subscribe(IConnector connector)
    {
        connector.Context.BaseObjectContext.ObjectMaterialized += this.ObjectContext_OnObjectMaterialized;
    }

    private void ObjectContext_OnObjectMaterialized(object sender, ObjectMaterializedEventArgs e)
    {
        // ...
    }
}
Apr 20, 2014 at 1:57 PM
Interesting idea, especially since the SuppressMethodAttribute is part of the framework. This prevents application code to take a dependency on Simple Injector, which is of course the first rule of Simple Injector club.
Apr 20, 2014 at 2:00 PM
This discussion has been copied to a work item. Click here to go to the work item and continue the discussion.
Apr 30, 2014 at 7:46 AM
I think we need to take a step back and consider alternative approaches for this implementation, since although we make use of a framework attribute, applying this attribute still implies a dependency on Simple Injector. Now however the dependency is implicit and textual. Besides, users will be poluting their code with these attributes, which would be about the same as having comments, since comments tend to get outdated and stale pretty soon when code gets refactored, or altered.

What other ways can we come up with of suppressing warnings?
Apr 30, 2014 at 10:07 AM
Edited Apr 30, 2014 at 10:11 AM
My initial thought was to move the Attribute's to the composition root at the point of registration, something like this:
[SuppressMessage(
    "Potential Lifestyle Mismatches", 
    "IObjectMaterializedSubscriber", 
    Target = "IEventPublisher eventPublisher",
    Justification = "This is fine although I could make it a Func<IEventPublisher> to avoid the warning.")]
internal void Configure(Container container)
{
    container.RegisterEnvironmentLifetime<IObjectMaterializedSubscriber, ObjectMaterializedSubscriber>();
}
But this may not be 100% reliable due to inlining.

So what about a specific method in the diagnostic services for registering items for exclusion?
container.RegisterDiagnosticServiceOverride(
    DiagnosticType.PotentialLifestyleMismatch,
    typeof(IObjectMaterializedSubscriber),
    "This is fine although I could make it a Func<IEventPublisher> to avoid the warning.");
public static void RegisterDiagnosticServiceOverride(
    this Container container,
    DiagnosticType diagnosticType,
    Type type,
    string justification)
{
    //...
}
Apr 30, 2014 at 8:41 PM
Besides the inlining issue, how would the container know where to find those diagnostic attributes. It has no notion of where it is created, and I don't think the container should do a stack walk. What we can allow on the other hand is applying attributes on the assembly level:
[assembly: SuppressMessage("SimpleInjector", "ContainerRegisteredComponent",
    Target = "MyApp.Namespace.ContainerRegisteredComponent")]
This would still allow to place those attributes close to the creation of the container (in the Composition Root). The only tricky part is how to find those attributes. Finding the assemblies might sound as simple as this query:
producers.Select(p => p.ImplementationType.Assembly).Distinct();
But it is possible that the assembly that contains the Composition Root contains no types itself that are part of the container. In that case, the Composition Root assembly would not be included in the search for attributes. The question of course is how high the odds are that this happens, but it would be quite annoying if someone hit this corner case. But the question then is, how to find out the Composition Root assembly?

The use of the RegisterDiagnosticServiceOverride is smart. This allows the extension method to be defined in the SimpleInjector.Diagnostics assembly and it keeps the core library and core API clean. The extension method can however still use the Container.Items dictionary to register the suppressions. However, I would probably go with a name that is a bit more intuitive, such as SuppressDiagnosticWarning. I like to prevent from having any methods in the framework that start with RegisterXXX, but don't actually do any component registration.

About the SuppressMessageAttributes. I think they should be placed on the Type itself, not on their constructors. And I would like to propose the following keyword literals:
[SuppressMessage("SimpleInjector", "ContainerRegisteredComponent")]
[SuppressMessage("SimpleInjector", "PotentialLifestyleMismatch")]
[SuppressMessage("SimpleInjector", "ShortCircuitedDependency")]
[SuppressMessage("SimpleInjector", "SingleResponsibilityViolation")]
Where "SimpleInjector" is the category and "ContainerRegisteredComponent" is the checkId, just as with category "Microsoft.Performance" with checkId "CA1811:AvoidUncalledPrivateCode".

Also note the classes that should be marked. This might be obvious with the Single Responsibility Violation and Container Registered Component, but might be less obvious for the other two. Here are some examples:
[SuppressMessage("SimpleInjector", "ContainerRegisteredComponent")]
public class ContainerRegisteredComponent { }

[SuppressMessage("SimpleInjector", "PotentialLifestyleMismatch")]
public class ParentClassThatDependsOnServiceWithShorterLifestyle { }

[SuppressMessage("SimpleInjector", "ShortCircuitedDependency")]
public class ClassWhereTheShortCircuitedDependencyIsInjectedIn { }

[SuppressMessage("SimpleInjector", "SingleResponsibilityViolation")]
public class ClassWithTooManyDependencies { }
So in case of the PotentialLifestyleMismatch, the SuppressMessage should be applied to the class that depends on the component with the shorter lifestyle. The attribute should not be applied to the child component. In case of the ShortCircuitedDependency, the SuppressMessage should be applied to the class that depends on the wrong component. The attribute should not be applied to the referenced component, neither to the abstraction that the warning expected. I think this is obvious when you look at the code; this is the exact behavior that the current diagnostic system uses.