locked
Trouble with MethodCall Operands in FxCop 1.36

    Question

  • I am writing an FxCop rule that tries to find all method calls to a specific logging API. It tries to verify that a number of the parameters to this call are literals, and produces documentation for all the calls made.

    For the most part, checking that Operands on the MethodCall object are Literal instances does the trick just fine, but I get into trouble when an overload with a 'params object[] arguments' is used. As soon as the 'params' occur, *all* arguments to the call are only reported back as 'NodeType.Pop'.

    I found a post somewhere that discussed walking the 'Instructions' and using a call to 'RuleUtilities' to walk backwards over the arguments from the call site, but this API call appears to have been discontinued in 1.36, so that is not helping me either.

    Is there any way to find the method argument literal values in this scenario that does not involve completely custom-writing the method walking/method stack analysis code?
    Thursday, October 29, 2009 3:12 AM

Answers

  • Here's something a bit more complex that should hit all the nested block types (although it hasn't been exhaustively tested).  As before, you'll need to implement IsCandidateMethod to

    internal sealed class YourRule : IntrospectionRule
    {
    	private static readonly IList<StatementHandler> _statementHandlers = new List<StatementHandler>()
    		{
    			new BlockHandler(),
    			new TryHandler(),
    			new CatchHandler(),
    			new FaultHandlerHandler(),
    			new FilterHandler(),
    			new FinallyHandler(),
    			new SwitchHandler(),
    new BranchHandler(), new ExpressionHandler() }; public YourRule() : base("YourRule") { } public override ProblemCollection Check(Member member) { Method method = member as Method; if (method != null) { this.StatementChecker = new Action<Statement, Block>(this.CheckMethodCalls); this.ProblemReporter = new Action<Literal>(this.AddProblem); this.CheckMethodCalls(method.Body, null); } return this.Problems; } private Action<Statement, Block> StatementChecker { get; set; } private Action<Literal> ProblemReporter { get; set; } private void CheckMethodCalls(Statement statement, Block containingBlock) { foreach (StatementHandler handler in YourRule._statementHandlers) { if (handler.HandleStatement(statement, containingBlock, this.StatementChecker, this.ProblemReporter)) { break; } } } private void AddProblem(Literal literal) { this.Problems.Add(new Problem(this.GetResolution(literal.Value), literal)); } #region Statement handlers private abstract class StatementHandler { protected StatementHandler() { } internal abstract bool HandleStatement( Statement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter); } private abstract class StatementHandler<TStatement> : StatementHandler where TStatement : Statement { protected StatementHandler() { } internal override bool HandleStatement( Statement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { bool handled = false; TStatement typedStatement = statement as TStatement; if (typedStatement != null) { this.HandleStatementCore(typedStatement, containingBlock, statementChecker, problemReporter); handled = true; } return handled; } protected abstract void HandleStatementCore( TStatement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter); } private sealed class BlockHandler : StatementHandler<Block> { internal BlockHandler() { } protected override void HandleStatementCore(Block statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { foreach (Statement child in statement.Statements) { statementChecker(child, statement); } } } private sealed class TryHandler : StatementHandler<TryNode> { internal TryHandler() { } protected override void HandleStatementCore(TryNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); statementChecker(statement.FaultHandler, containingBlock); statementChecker(statement.Finally, containingBlock); foreach (CatchNode catchNode in statement.Catchers) { statementChecker(catchNode, containingBlock); } } } private sealed class CatchHandler : StatementHandler<CatchNode> { internal CatchHandler() { } protected override void HandleStatementCore(CatchNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FaultHandlerHandler : StatementHandler<FaultHandler> { internal FaultHandlerHandler() { } protected override void HandleStatementCore(FaultHandler statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FilterHandler : StatementHandler<Filter> { internal FilterHandler() { } protected override void HandleStatementCore(Filter statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FinallyHandler : StatementHandler<FinallyNode> { internal FinallyHandler() { } protected override void HandleStatementCore(FinallyNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class BranchHandler : StatementHandler<Branch> { internal BranchHandler() { } protected override void HandleStatementCore(Branch statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Target, containingBlock); } } private sealed class SwitchHandler : StatementHandler<SwitchInstruction> { internal SwitchHandler() { } protected override void HandleStatementCore(SwitchInstruction statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { foreach (Block childBlock in statement.Targets) { statementChecker(childBlock, containingBlock); } } } private sealed class ExpressionHandler : StatementHandler<ExpressionStatement> { internal ExpressionHandler() { } protected override void HandleStatementCore(ExpressionStatement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { MethodCall call = statement.Expression as MethodCall; if (call != null) { Method callee = (Method)((MemberBinding)call.Callee).BoundMember; if (this.IsCandidateMethod(callee)) { this.CheckParameters(call, problemReporter); this.CheckParameters(statement, containingBlock, problemReporter); } } } private bool IsCandidateMethod(Method method) { throw new NotImplementedException(); } private void CheckParameters(MethodCall call, Action<Literal> problemReporter) { foreach (Expression operand in call.Operands) { this.CheckExpression(operand, problemReporter); } } public void CheckParameters(ExpressionStatement callStatement, Block callBlock, Action<Literal> problemReporter) { var statements = from s in callBlock.Statements where (s.SourceContext.StartLine >= callStatement.SourceContext.StartLine) && (s.SourceContext.EndLine <= callStatement.SourceContext.EndLine) && (!s.Equals(callStatement)) select s; foreach (Statement statement in statements) { ExpressionStatement expressionStatement = statement as ExpressionStatement; if (expressionStatement != null) { this.CheckExpression(expressionStatement.Expression, problemReporter); } else { AssignmentStatement assigmentStatement = statement as AssignmentStatement; if (assigmentStatement != null) { if (assigmentStatement.Target is Indexer) { this.CheckExpression(assigmentStatement.Source, problemReporter); } } } } } private void CheckExpression(Expression expression, Action<Literal> problemReporter) { if (expression != null) { Literal literal = expression as Literal; if (literal != null) { problemReporter(literal); } else { if (expression.NodeType == NodeType.Box) { this.CheckExpression(((BinaryExpression)expression).Operand1, problemReporter); } } } } } #endregion }
    • Marked as answer by Roahn Luo Wednesday, November 04, 2009 11:16 AM
    Friday, October 30, 2009 1:13 PM

All replies

  • Here's something that should work for most scenarios (you'll need to implement IsCandidateMethod to identify your target API calls).  Finding "nested" statements based on their SourceContext is a bit of a hack, but it shouldn't result in too many false positives unless you regularly place more than one real statement on a given line.

    public override ProblemCollection Check(Member member)
    {
    	Method method = member as Method;
    	if (method != null)
    	{
    		this.CheckMethodCalls(method.Body);
    	}
    
    	return this.Problems;
    }
    
    private void CheckMethodCalls(Block block)
    {
    	
    	foreach (Statement statement in block.Statements)
    	{
    		Block blockStatement = statement as Block;
    		if (blockStatement != null)
    		{
    			this.CheckMethodCalls(blockStatement);
    		}
    		else
    		{
    			ExpressionStatement expressionStatement = statement as ExpressionStatement;
    			if (expressionStatement != null)
    			{
    				MethodCall call = expressionStatement.Expression as MethodCall;
    				if (call != null)
    				{
    					Method callee = (Method)((MemberBinding)call.Callee).BoundMember;
    					if (this.IsCandidateMethod(callee))
    					{
    						this.CheckParameters(call);
    						this.CheckParameters(expressionStatement, block);
    					}
    				}
    			}
    		}
    	}
    }
    
    private void CheckParameters(MethodCall call)
    {
    	foreach (Expression operand in call.Operands)
    	{
    		this.CheckExpression(operand);
    	}
    }
    
    public void CheckParameters(ExpressionStatement callStatement, Block callBlock)
    {
    	var statements = from s in callBlock.Statements
    					 where (s.SourceContext.StartLine >= callStatement.SourceContext.StartLine) &&
    						(s.SourceContext.EndLine <= callStatement.SourceContext.EndLine) &&
    						(!s.Equals(callStatement))
    					 select s;
    
    	foreach (Statement statement in statements)
    	{
    		ExpressionStatement expressionStatement = statement as ExpressionStatement;
    		if (expressionStatement != null)
    		{
    			this.CheckExpression(expressionStatement.Expression);
    		}
    		else
    		{
    			AssignmentStatement assigmentStatement = statement as AssignmentStatement;
    			if (assigmentStatement != null)
    			{
    				if (assigmentStatement.Target is Indexer)
    				{
    					this.CheckExpression(assigmentStatement.Source);
    				}
    			}
    		}
    	}
    }
    
    private bool IsCandidateMethod(Method method)
    {
    	throw new NotImplementedException();
    }
    
    private void CheckExpression(Expression expression)
    {
    	Literal literal = expression as Literal;
    	if (literal != null)
    	{
    		this.AddProblem(literal);
    	}
    	else
    	{
    		if (expression.NodeType == NodeType.Box)
    		{
    			this.CheckExpression(((BinaryExpression)expression).Operand1);
    		}
    	}
    }
    
    private void AddProblem(Literal literal)
    {
    	this.Problems.Add(new Problem(this.GetResolution(literal.Value), literal));
    }
    Thursday, October 29, 2009 2:46 PM
  • That does an excellent job at finding all the literals in the context of the call that they belong to. I am having some trouble matching the literals back to the method parameters that they belong to.

    In essence what I am trying to do is make a lookup from parameter names to literal values (if available), and then the core logic of the rule checks which parameters have literals and whether they satisfy all requirements for a call of the type made.

    Is there a way to go from the 'Literal' back to the parameter that it matches?
    Thursday, October 29, 2009 10:59 PM
  • Ah, actually... it also fails to find calls in non-standard places... if I have calls in exception handlers for instance they do not get found; haven't tried if there is any other kind of nesting it has trouble with either.

    I am starting to get the sinking feeling that I'll need to look at the raw instruction stream and try to make the most of that... which means I'll have to make a lookup for the stack behaviour of all IL instructions to be able to parse over complex arguments... :/
    Thursday, October 29, 2009 11:06 PM
  • Here's something a bit more complex that should hit all the nested block types (although it hasn't been exhaustively tested).  As before, you'll need to implement IsCandidateMethod to

    internal sealed class YourRule : IntrospectionRule
    {
    	private static readonly IList<StatementHandler> _statementHandlers = new List<StatementHandler>()
    		{
    			new BlockHandler(),
    			new TryHandler(),
    			new CatchHandler(),
    			new FaultHandlerHandler(),
    			new FilterHandler(),
    			new FinallyHandler(),
    			new SwitchHandler(),
    new BranchHandler(), new ExpressionHandler() }; public YourRule() : base("YourRule") { } public override ProblemCollection Check(Member member) { Method method = member as Method; if (method != null) { this.StatementChecker = new Action<Statement, Block>(this.CheckMethodCalls); this.ProblemReporter = new Action<Literal>(this.AddProblem); this.CheckMethodCalls(method.Body, null); } return this.Problems; } private Action<Statement, Block> StatementChecker { get; set; } private Action<Literal> ProblemReporter { get; set; } private void CheckMethodCalls(Statement statement, Block containingBlock) { foreach (StatementHandler handler in YourRule._statementHandlers) { if (handler.HandleStatement(statement, containingBlock, this.StatementChecker, this.ProblemReporter)) { break; } } } private void AddProblem(Literal literal) { this.Problems.Add(new Problem(this.GetResolution(literal.Value), literal)); } #region Statement handlers private abstract class StatementHandler { protected StatementHandler() { } internal abstract bool HandleStatement( Statement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter); } private abstract class StatementHandler<TStatement> : StatementHandler where TStatement : Statement { protected StatementHandler() { } internal override bool HandleStatement( Statement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { bool handled = false; TStatement typedStatement = statement as TStatement; if (typedStatement != null) { this.HandleStatementCore(typedStatement, containingBlock, statementChecker, problemReporter); handled = true; } return handled; } protected abstract void HandleStatementCore( TStatement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter); } private sealed class BlockHandler : StatementHandler<Block> { internal BlockHandler() { } protected override void HandleStatementCore(Block statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { foreach (Statement child in statement.Statements) { statementChecker(child, statement); } } } private sealed class TryHandler : StatementHandler<TryNode> { internal TryHandler() { } protected override void HandleStatementCore(TryNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); statementChecker(statement.FaultHandler, containingBlock); statementChecker(statement.Finally, containingBlock); foreach (CatchNode catchNode in statement.Catchers) { statementChecker(catchNode, containingBlock); } } } private sealed class CatchHandler : StatementHandler<CatchNode> { internal CatchHandler() { } protected override void HandleStatementCore(CatchNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FaultHandlerHandler : StatementHandler<FaultHandler> { internal FaultHandlerHandler() { } protected override void HandleStatementCore(FaultHandler statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FilterHandler : StatementHandler<Filter> { internal FilterHandler() { } protected override void HandleStatementCore(Filter statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class FinallyHandler : StatementHandler<FinallyNode> { internal FinallyHandler() { } protected override void HandleStatementCore(FinallyNode statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Block, containingBlock); } } private sealed class BranchHandler : StatementHandler<Branch> { internal BranchHandler() { } protected override void HandleStatementCore(Branch statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { statementChecker(statement.Target, containingBlock); } } private sealed class SwitchHandler : StatementHandler<SwitchInstruction> { internal SwitchHandler() { } protected override void HandleStatementCore(SwitchInstruction statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { foreach (Block childBlock in statement.Targets) { statementChecker(childBlock, containingBlock); } } } private sealed class ExpressionHandler : StatementHandler<ExpressionStatement> { internal ExpressionHandler() { } protected override void HandleStatementCore(ExpressionStatement statement, Block containingBlock, Action<Statement, Block> statementChecker, Action<Literal> problemReporter) { MethodCall call = statement.Expression as MethodCall; if (call != null) { Method callee = (Method)((MemberBinding)call.Callee).BoundMember; if (this.IsCandidateMethod(callee)) { this.CheckParameters(call, problemReporter); this.CheckParameters(statement, containingBlock, problemReporter); } } } private bool IsCandidateMethod(Method method) { throw new NotImplementedException(); } private void CheckParameters(MethodCall call, Action<Literal> problemReporter) { foreach (Expression operand in call.Operands) { this.CheckExpression(operand, problemReporter); } } public void CheckParameters(ExpressionStatement callStatement, Block callBlock, Action<Literal> problemReporter) { var statements = from s in callBlock.Statements where (s.SourceContext.StartLine >= callStatement.SourceContext.StartLine) && (s.SourceContext.EndLine <= callStatement.SourceContext.EndLine) && (!s.Equals(callStatement)) select s; foreach (Statement statement in statements) { ExpressionStatement expressionStatement = statement as ExpressionStatement; if (expressionStatement != null) { this.CheckExpression(expressionStatement.Expression, problemReporter); } else { AssignmentStatement assigmentStatement = statement as AssignmentStatement; if (assigmentStatement != null) { if (assigmentStatement.Target is Indexer) { this.CheckExpression(assigmentStatement.Source, problemReporter); } } } } } private void CheckExpression(Expression expression, Action<Literal> problemReporter) { if (expression != null) { Literal literal = expression as Literal; if (literal != null) { problemReporter(literal); } else { if (expression.NodeType == NodeType.Box) { this.CheckExpression(((BinaryExpression)expression).Operand1, problemReporter); } } } } } #endregion }
    • Marked as answer by Roahn Luo Wednesday, November 04, 2009 11:16 AM
    Friday, October 30, 2009 1:13 PM
  • Hello jerry,

    Have you tried Nicole's suggestion? How about your issue? I'm marking the reply as answer if you don't mind. Hope you could understand.

    Best regards,
    Please remember to mark the replies as answers if they help and unmark them if they provide no help.
    If you have any feedback, please tell us.
    Welcome to the All-In-One Code Framework!
    Wednesday, November 04, 2009 11:18 AM
  • I found the same problem with an FxCop rule that I wrote. The solution presented below doesn't really solve the problem--it just visits everything without any structure.

    I submitted this to Microsoft Connect and it has been acknowledged as a bug. "We have identified the issue the cause of this. We will consider incorporating this in a future release of Visual Studio."

    http://connect.microsoft.com/VisualStudio/feedback/details/543036/methodcall-operands-in-fxcop-1-36

    Tuesday, March 23, 2010 2:24 AM