7

Given the following piece of code as an example:

public class Thing
{
    public int Item { get; }

    public Thing(int item)
    {
        Item = Item; // note incorrect assignment: rhs should be item, the passed-in arg, hence analyzer should warn
    }

    public Thing(Thing other)
    {
      Item = other.Item; // correct assignment, should NOT trigger analyzer
    }
}

I'm writing a Roslyn analyzer to detect and report these cases of possible mistaken self-assignment, pertinent portions below:

public override void Initialize(AnalysisContext context)
{
    context.RegisterSyntaxNodeAction(MistakenSelfAssignment, SyntaxKind.SimpleAssignmentExpression);
}

private static void MistakenSelfAssignment(SyntaxNodeAnalysisContext context)
{
    var assignment = context.Node as AssignmentExpressionSyntax;
    if (assignment == null)
    {
        return;
    }

    var leftToken = GetIdentifierToken(assignment.Left);
    var rightToken = GetIdentifierToken(assignment.Right);

    if (leftToken != null && leftToken.IsEquivalentTo(rightToken)) // this never works
    {
        var diagnostic = Diagnostic.Create(Rule, assignment.GetLocation());
        context.ReportDiagnostic(diagnostic);
    }
}

private static SyntaxToken GetIdentifierToken(ExpressionSyntax syntax)
{
    var identifierName = syntax as IdentifierNameSyntax;
    if (identifierName != null)
    {
        return identifierName.Identifier;
    }

    var identifierAccess = syntax as MemberAccessExpressionSyntax;
    if (identifierAccess != null)
    {
        return identifierAccess.Name.Identifier;
    }

    return default(SyntaxToken);
}

But I can't figure out how to determine if the LHS and RHS of the assignment are the same token - SyntaxToken.IsEquivalentTo appears to be the method I want, but it always returns false, as do SyntaxToken.Equals and ==.

What's the correct way to determine if a token is referring to itself?

Ian Kemp
  • 28,293
  • 19
  • 112
  • 138
  • 1
    SyntaxFactory.AreEquivalent? – Marcus Sep 28 '16 at 07:58
  • @Marcus that sort of works, but it also considers things like `Item = other.Item` as equivalent, which is not what I want. I'm only interested in cases where a variable or property is being assigned its own value. – Ian Kemp Sep 28 '16 at 08:38
  • Can you post the entire code subject to analysis? Or the part where the comparison fails. By looking at the implementation I can´t see how `IsEquivalentTo` can possibly fail to deliver accurate results. – Marcus Sep 28 '16 at 09:10
  • @Marcus updated the example class. If I change `leftToken.IsEquivalentTo(rightToken)` to `SyntaxFactory.AreEquivalent(leftToken, rightToken)` then it warns on both the standard and copy constructors, which is wrong. Please let me know if that's clear enough? – Ian Kemp Sep 28 '16 at 09:46
  • It would help a lot if you could post the `SyntaxToken` contents (the entire object tree if possible) at the row where `IsEquivalentTo` is evaluated. – Marcus Sep 28 '16 at 10:05
  • It's arguably a bug in the compiler that it doesn't emit a warning here, since we'd emit a warning if these were locals or parameters. I've emailed the team to understand why. – Jason Malinowski Oct 03 '16 at 22:40

1 Answers1

1

I don't think you can do this on the SyntaxToken level. At first, I thought that the semantic model would help you here, but in both cases, the symbols refer to the same thing, so you cannot use that to differentiate.

However, what you can do, is just investigate the SimpleAssignmentExpression, check if both operands are identifiers, and check their equivalency through the same SyntaxFactory.AreEquivalent() that Marcus mentioned. I got to this (see this gist for a full LINQPad query):

Let's say you write this method:

private static bool IsAssignmentBad(AssignmentExpressionSyntax assignmentNode)
{
    if (!assignmentNode.IsKind(SyntaxKind.SimpleAssignmentExpression))
    {
        return false;
    }

    var lhs = assignmentNode.Left;
    if (!lhs.IsKind(SyntaxKind.IdentifierName))
    {
        return false;
    }

    var rhs = assignmentNode.Right;
    if (!rhs.IsKind(SyntaxKind.IdentifierName))
    {
        return false;
    }

    return SyntaxFactory.AreEquivalent(lhs, rhs);
}

Then running it with this gives what you want, I think:

var tree = CSharpSyntaxTree.ParseText(
@"public class Thing
{
    public int Item { get; }

    public Thing(int item)
    {
        Item = Item; // note incorrect assignment: rhs should be item, the passed-in arg, hence analyzer should warn
    }

    public Thing(Thing other)
    {
        Item = other.Item; // correct assignment, should NOT trigger analyzer
    }
}");

var root = tree.GetRoot();

var incorrectAssignment = root.DescendantNodes().OfType<AssignmentExpressionSyntax>().First();
var correctAssignment = root.DescendantNodes().OfType<AssignmentExpressionSyntax>().Last();

var b1 = IsAssignmentBad(correctAssignment); // doesn't consider the assignment bad
var b2 = IsAssignmentBad(incorrectAssignment); // this one does
Ties
  • 1,205
  • 2
  • 15
  • 28
  • The `if (!lhs.IsKind(SyntaxKind.IdentifierName))` check is probably too strong. The program text might be `this.Item = Item`, and I guess the OP would want that flagged as well. – Kris Vandermotten Oct 12 '16 at 08:00
  • True, didn't think of that one. I'll update the answer later (I'm on a Mac now, so cannot test the code). – Ties Oct 12 '16 at 19:51