[FxCop 1.36, Custom rule] How to find all references to an specific node?

Question

Hello!
Is it possible to find all references to concrete node (caller/callee list, field load/store, delegate creation etc) without manual enumerating over all statements?

Sample simple rule to represent scenario:
Restrict load/store of fields if they're exposed via property getter/setter respectively.

It seems, there will be two-phase run:
1) override check on TypeNode: iterate over all properties and store fields they're exposing
2) override check on member: store current member and visit all statements of each. If member references exposed field - there's an problem.

What did I miss? Would it be thread-safe?

Also, is there good manual except one at http://www.binarycoder.net/? It's quite untrivial to drill through FxCop code model.

Answer 1

Is it possible to find all references to concrete node (caller/callee list, field load/store, delegate creation etc) without manual enumerating over all statements?

With the FxCop introspection engine, visiting all statements is your only option.  (The flow analysis engine available in previous versions and re-introduced in VS2010 may allow other approaches.)  However, depending on what you're targeting, you don't necessarily need to enumerate over all statements yourself.  In some cases, overriding the Visit____ methods will expose sufficient information for implementing a rule.

Sample simple rule to represent scenario:
Restrict load/store of fields if they're exposed via property getter/setter respectively.

It seems, there will be two-phase run:
1) override check on TypeNode: iterate over all properties and store fields they're exposing
2) override check on member: store current member and visit all statements of each. If member references exposed field - there's an problem.

What did I miss? Would it be thread-safe?

A couple of things you're missing:

1. There's no need to check a field if it is generated by the compiler since the original source code could not have referenced it.  The RuleUtilities.IsCompilerGenerated method can help you detect this case.
2. It is non-trivial to determine if a field is a backing field for a property.  Both the getter and the setter can contain quite a bit more logic than simply accessing the field.
3. With your proposed approach, there is no guarantee that a wrapping property would be identified before a member that uses a field is visited.

Also, is there good manual except one at http://www.binarycoder.net/? It's quite untrivial to drill through FxCop code model.

That's pretty much the most complete single reference that exists at this time.  There is no official SDK.

Answer 2

Thanks a lot!

>In some cases, overriding the Visit____ methods will expose sufficient information for implementing a rule.

Amm... please confirm:
1) BaseIntrospectionRule is passive, so Visit* methods would not be executed if I don't call them via overloading one of Check* methods, right?

2) Also, if I wishing to pass subnodes I need to call overrided (base.Visit...)

So, BaseIntrospectionRule derives from BinaryReadOnlyVisitor because there's need to override visit logic,  and exposing visitor as a protected settable property will be to hard to discover by customers, right?  

>A couple of things you're missing:
1. Yes, done.
2. I'll use very simple approach - detect field access and ensure property type assignable from/to field type. With small tricks,yes;)
3. Why? I'll call Visit* twice: first time to detect all fields (no validation at this moment), second time - to validate all members (problem detection goes there).

Answer 3

Amm... please confirm:
1) BaseIntrospectionRule is passive, so Visit* methods would not be executed if I don't call them via overloading one of Check* methods, right?

That's correct.  The usual approach for your scenario (inspecting contents of methods) is to invoke the Visit method over the target member from the Check(Member) overload.  e.g.:

public override ProblemCollection Check(Member member)
{
	this.Visit(member);
	return this.Problems;
}

2) Also, if I wishing to pass subnodes I need to call overrided (base.Visit...)

I'm not entirely sure what you mean by this.  However, I suspect that the answer is "no".  You should only need to call the Visit method once to trigger visiting, although you will need to override each the Visit____ method for each node type that you wish to intercept during execution of the visitor.

So, BaseIntrospectionRule derives from BinaryReadOnlyVisitor because there's need to override visit logic,  and exposing visitor as a protected settable property will be to hard to discover by customers, right?  

Given that there is no documentation, we can only guess about the intent.  As implemented, I actually find that the behaviour does not improve discoverability in any way over the more conventional visitor implementations, so my own guess would lie elsewhere.

Why? I'll call Visit* twice: first time to detect all fields (no validation at this moment), second time - to validate all members (problem detection goes there).

Sorry, but I'm still not seeing how you're planning on controlling the sequence using this approach.  I can only see two feasible solutions:

1. Rely on the fact that Check(TypeNode) is run before Check(Member) to allow identifying of target fields from Check(TypeNode), or
2. Identify target fields from the first Check(Member).

Given that there's no SDK and, hence, no guaranteed execution sequence, my own preference would be approach #2, although ymmv...

Answer 4

Thanks again!
to finish all tje stuff:
>I'm not entirely sure what you mean by this.  However, I suspect that the answer is "no".  You should only need to call the Visit method once to trigger visiting, although you will need to override each the Visit____ method for each node type that you wish to intercept during execution of the visitor.

Oups;) It was meaned "If I override Visit method I need to call base.Visit to continue introspection of subnodes". And, as reflector shows, it's really so. At this moment, at least.

>Sorry, but I'm still not seeing how you're planning on controlling the sequence using this approach.  I can only see two feasible solutions:

Yes, already detected - there's no warranty the current behavior will not change in future.

As it's done now:
1) Refused to use VisitXXX, just iterating over instructions is enough and simpler, as it's easy to store state during introspection:

    private IEnumerable<Field> GetFieldAccess(Method method, bool load)
    {
      return from instruction in method.Instructions
             where instruction.OpCode == (load ? OpCode.Ldfld : OpCode.Stfld)
             let field = (Field)instruction.Value
             where field.DeclaringType == method.DeclaringType
                && !RuleUtilities.HasCustomAttribute(field, typeof(CompilerGeneratedAttribute).FullName, false)
             select field;
    }
    private IEnumerable<Field> GetExposedFields(Method accessor, bool load, PropertyNode container)
    {
      return from field in GetFieldAccess(acessor, load)
             where load ?
                   container.Type.IsAssignableTo(field.Type) :
                   field.Type.IsAssignableTo(container.Type)
             select field;
    }

2) Caching exposed fields on demand and storing typeNode being validated to prevent waste recalculation.

    private void ResetExposedFields(TypeNode type)
    {
      if (currentType != type)
      {
        getExposedFields.Clear();
        setExposedFields.Clear();
        currentType = type;
        foreach (PropertyNode property in type.Members.OfType<PropertyNode>())
        {
          FillExposed(property, true);
          FillExposed(property, false);
        }
      }
    }
    private void FillExposed(PropertyNode property, bool load)
    {
      Method acessor = load ? property.Getter : property.Setter;
      if (acessor != null)
      {
        foreach (Field field in GetExposedFields(acessor, load, property))
        {
          HashSet<PropertyNode> props;
          Dictionary<Field, HashSet<PropertyNode>> exposedFields = load ? this.getExposedFields : this.setExposedFields;
          if (!exposedFields.TryGetValue(field, out props))
          {
            props = new HashSet<PropertyNode>(comparer);
            exposedFields.Add(field, props);
          }
          props.Add(property);
        }
      }
    }

3. Using custom comparer to compare node (not overrided Equals - fail).

    private class Comparer: IEqualityComparer<PropertyNode>, IEqualityComparer<Field>
    {
      private static string GetFullName(Member node)
      {
        return node == null ? string.Empty :
          node.DeclaringType.DeclaringModule.ContainingAssembly.StrongName +
          node.FullName;
      }

      #region IEqualityComparer<PropertyNode> Members
      public bool Equals(PropertyNode x, PropertyNode y)
      {
        return GetFullName(x) == GetFullName(y);
      }

      public int GetHashCode(PropertyNode obj)
      {
        return GetFullName(obj).GetHashCode();
      }
      #endregion

      #region IEqualityComparer<Field> Members
      public bool Equals(Field x, Field y)
      {
        return GetFullName(x) == GetFullName(y);
      }

      public int GetHashCode(Field obj)
      {
        return GetFullName(obj).GetHashCode();
      }
      #endregion
    }

4. The validtion itself is simple:

    public override ProblemCollection Check(Member member)
    {
      Method method = member as Method;
      if (method != null)
      {
        ResetExposedFields(method.DeclaringType);
        ValidateFieldReferences(method, method.DeclaringMember as PropertyNode, true);
        ValidateFieldReferences(method, method.DeclaringMember as PropertyNode, false);
      }
      return Problems;
    }

    private void ValidateFieldReferences(Method method, PropertyNode container, bool load)
    {
      foreach (Field field in GetFieldAccess(method, load))
      {
        var exposedFields = load ? this.getExposedFields : this.setExposedFields;
        HashSet<PropertyNode> props;
        if (exposedFields.TryGetValue(field, out props))
        {
          if (container == null || !props.Contains(container))
          {
            Problems.Add(new Problem(
              new Resolution(load ? "Reading exposed field!" : "Writing exposed field!"),
              field));
          }
        }
        else
        {
          Problems.Add(new Problem(
            new Resolution(load ? "Reading nonexposed field!" : "Writing nonexposed field!"),
            field));
        }
      }
    }

Last question: I push target node into Resolution ctor - why FxCop references to first source line as a source of trouble???

Answer 5

I push target node into Resolution ctor - why FxCop references to first source line as a source of trouble???

You're providing the field itself rather than the instruction that uses the field to the Problem constructor.  Even if FxCop did properly populate the source context data for a field, this still wouldn't be the source context that you would want reported by your rule.  You'll need to grab the instruction context from in your GetFieldAccess method.