Search code examples
c#code-analysisstylecop

CA1033 with Properties


When I run code analysis (VS2013) with the 'Microsoft Managed Recommend Rules' rule set, the only warnings I get for my class library are of type CA1033: 'Interface methods should be callable by child types'. But I don't understand the rule in this situation:

/// An object that has a chemical formula
public interface IChemicalFormula
{ 
    /// The chemical formula of the object
    ChemicalFormula ChemicalFormula {get;}       
}

public class ChemicalFormula: IChemicalFormula
{         
    ChemicalFormula IChemicalFormula.ChemicalFormula
    {
        get { return this; }
    }
}

The docs recommends making a protected method with the same name so that deriving types can access it, but you cannot name a method the same as the enclosing type. They also recommend making the class sealed, but I don't want it to be sealed in this case. Is this a time just to ignore this rule, or is there an appropriate way to handle it?

EDIT

To add clarification why the class/interface is designed this way, I have another class, Peptide that contains a IChemicalFormula[] array to store modifications. Not every modification necessarily derives directly from ChemicalFormula, but they need to implement the IChemicalFormula interface. Therefore, if I modify an instance of a peptide withsome molecule (H2O for example), then ChemicalFormula class needs to also implement IChemicalFormula.


Solution

  • This is the description of the rule:

    Consider a base type that explicitly implements a public interface method. A type that derives from the base type can access the inherited interface method only through a reference to the current instance (this in C#) that is cast to the interface. If the derived type re-implements (explicitly) the inherited interface method, the base implementation can no longer be accessed. The call through the current instance reference will invoke the derived implementation; this causes recursion and an eventual stack overflow.

    I think you should consider evaluating the usage of this property. A good example where TDD could be used to figure out the interface. There are some possible usages (and some invalid ones) below. I am not yet sure what you intend to achieve by looking at those.

    In your example, let's say another class, NewChemicalForumla is derived from ChemicalForumula, and references ChemicalFormula, what does that mean?

    public class NewChemicalFormula: ChemicalFormula
    {         
        public void Method()
        {
            Console.WriteLine("{0}", ChemicalFormula.GetType());       // Compile error
            Console.WriteLine("{0}", this.ChemicalFormula.GetType());  // Effectively same as above, compile error
            Console.WriteLine("{0}", ((IChemicalFormula)this).ChemicalFormula.GetType()); // Works, is that what you intend?
        }
    }
    

    Now from outside the class, there are two possibilities:

    1. When you have a handle to a derived class:

      new NewChemicalFormula().ChemicalFormula.GetType() // Error
      

      or

      // This works, is that what you intend to achieve?
      ((IChemicalFormula)new NewChemicalFormula()).ChemicalFormula.GetType()  
      
    2. When you have a handle to the IChemicalFormula already. In this case, ChemicalFormula seems redundant:

      IChemicalFormula formula = new NewChemicalFormula();
      Console.WriteLine("{0}", formula.GetType());                 // Works, returns NewChemicalFormula
      Console.WriteLine("{0}", formula.ChemicalFormula.GetType()); // Works, returns NewChemicalFormula
      Console.WriteLine("{0}", formula.ChemicalFormula.Method());  // Compile error
      

    formula.ChemicalFormula.Method() leads to an error because you must cast it to NewChemicalFormula before you can use Method(). Just because the property returns this doesn't help solve this problem.

    So the FXCop warning is worth considering, and evaluating the design.