Skip to content
This repository has been archived by the owner. It is now read-only.

Commit

Permalink
Fixed #237: RECS0021 - 'Virtual member call in constructor' false pos…
Browse files Browse the repository at this point in the history 
…itive.
  • Loading branch information
@Rpinski
Rpinski committed Jul 24, 2016
1 parent d537f72 commit fb6c8e6
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 97 deletions.
10 changes: 6 additions & 4 deletions ...CSharp/Diagnostics/Synced/CodeQuality/DoNotCallOverridableMethodsInConstructorAnalyzer.cs
View file
Expand Up @@ -70,14 +70,16 @@ public VirtualCallFinderVisitor(SyntaxNodeAnalysisContext nodeContext, Construct
this.type = type;
}

void Check(SyntaxNode n)
void Check(SyntaxNode n, bool skipMethods)
{
var info = nodeContext.SemanticModel.GetSymbolInfo(n);
var symbol = info.Symbol;
if ((symbol == null) || (symbol.ContainingType == null) || symbol.ContainingType.Locations.Where(loc => loc.IsInSource && loc.SourceTree.FilePath == type.SyntaxTree.FilePath).All(loc => !type.Span.Contains(loc.SourceSpan)))
return;
if (symbol is ITypeSymbol)
return;
if (skipMethods && (symbol.Kind == SymbolKind.Method))
return;
if (!symbol.IsSealed && (symbol.IsVirtual || symbol.IsAbstract || symbol.IsOverride))
{
if (symbol.Kind == SymbolKind.Property)
Expand Down Expand Up @@ -113,7 +115,7 @@ public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax no
if (node.Parent is MemberAccessExpressionSyntax || node.Parent is InvocationExpressionSyntax)
return;
if (node.Expression.IsKind(SyntaxKind.ThisExpression))
Check(node);
Check(node, true);
}

public override void VisitIdentifierName(IdentifierNameSyntax node)
Expand All @@ -123,7 +125,7 @@ public override void VisitIdentifierName(IdentifierNameSyntax node)
if (ancestors.Any(n => (n is MemberAccessExpressionSyntax) || (n is InvocationExpressionSyntax)))
return;

Check(node);
Check(node, true);
}

static bool IsSimpleThisCall(ExpressionSyntax expression)
Expand All @@ -140,7 +142,7 @@ public override void VisitInvocationExpression(InvocationExpressionSyntax node)
if (node.Parent is MemberAccessExpressionSyntax || node.Parent is InvocationExpressionSyntax)
return;
if (node.Expression.IsKind(SyntaxKind.IdentifierName) || IsSimpleThisCall(node.Expression))
Check(node);
Check(node, false);
}

public override void VisitParenthesizedLambdaExpression(ParenthesizedLambdaExpressionSyntax node)
Expand Down
228 changes: 135 additions & 93 deletions Tests/CSharp/Diagnostics/DoNotCallOverridableMethodsInConstructorTests.cs
View file
Expand Up @@ -12,15 +12,15 @@ public void CatchesBadCase()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
$Bar()$;
$this.Bar()$;
}
virtual void Bar ()
{
}
Foo()
{
$Bar()$;
$this.Bar()$;
}
virtual void Bar ()
{
}
}");
}

Expand All @@ -29,15 +29,15 @@ public void TestDisable()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
Foo()
{
#pragma warning disable " + CSharpDiagnosticIDs.DoNotCallOverridableMethodsInConstructorAnalyzerID + @"
Bar();
}
Bar();
}
virtual void Bar ()
{
}
virtual void Bar ()
{
}
}");
}

Expand All @@ -48,15 +48,15 @@ public void IgnoresGoodCase()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
Bar();
Bar();
}
void Bar ()
{
}
Foo()
{
Bar();
Bar();
}
void Bar ()
{
}
}");
}

Expand All @@ -65,15 +65,15 @@ public void IgnoresSealedClasses()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"sealed class Foo
{
Foo()
{
Bar();
Bar();
}
virtual void Bar ()
{
}
Foo()
{
Bar();
Bar();
}
virtual void Bar ()
{
}
}");
}

Expand All @@ -83,22 +83,22 @@ public void IgnoresOverriddenSealedMethods()
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"
class BaseClass
{
protected virtual void Bar ()
{
}
protected virtual void Bar ()
{
}
}
class DerivedClass : BaseClass
{
DerivedClass()
{
Bar();
Bar();
}
protected override sealed void Bar ()
{
}
DerivedClass()
{
Bar();
Bar();
}
protected override sealed void Bar ()
{
}
}");
}

Expand All @@ -107,15 +107,15 @@ public void IgnoresNonLocalCalls()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
Foo f = new Foo();
f.Bar();
}
virtual void Bar ()
{
}
Foo()
{
Foo f = new Foo();
f.Bar();
}
virtual void Bar ()
{
}
}");
}

Expand All @@ -124,14 +124,56 @@ public void IgnoresEventHandlers()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
SomeEvent += delegate { Bar(); };
}
virtual void Bar ()
{
}
Foo()
{
SomeEvent += delegate { Bar(); };
}
virtual void Bar ()
{
}
}");
}

[Test]
public void IgnoresDelegates1()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"
using System;
class Foo
{
private Action barAction;
Foo()
{
barAction = Bar;
}
virtual void Bar()
{
}
}");
}

[Test]
public void IgnoresDelegates2()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"
using System;
class Foo
{
Foo()
{
SaveBarAction(this.Bar);
}
void SaveBarAction(Action barAction)
{
}
virtual void Bar()
{
}
}");
}

Expand All @@ -158,12 +200,12 @@ public void SetVirtualPropertyThroughThis()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
$this.AutoProperty$ = 1;
}
Foo()
{
$this.AutoProperty$ = 1;
}
public virtual int AutoProperty { get; set; }
public virtual int AutoProperty { get; set; }
}");
}

Expand All @@ -172,12 +214,12 @@ public void SetVirtualProperty()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
$AutoProperty$ = 1;
}
Foo()
{
$AutoProperty$ = 1;
}
public virtual int AutoProperty { get; set; }
public virtual int AutoProperty { get; set; }
}");
}

Expand All @@ -186,12 +228,12 @@ public void GetVirtualPropertyThroughThis()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
var val = $this.AutoProperty$;
}
Foo()
{
var val = $this.AutoProperty$;
}
public virtual int AutoProperty { get; set; }
public virtual int AutoProperty { get; set; }
}");
}

Expand All @@ -200,12 +242,12 @@ public void GetVirtualProperty()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
var val = $AutoProperty$;
}
Foo()
{
var val = $AutoProperty$;
}
public virtual int AutoProperty { get; set; }
public virtual int AutoProperty { get; set; }
}");
}

Expand All @@ -214,12 +256,12 @@ public void GetVirtualPropertyWithPrivateSetter()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
var val = $AutoProperty$;
}
Foo()
{
var val = $AutoProperty$;
}
public virtual int AutoProperty { get; private set; }
public virtual int AutoProperty { get; private set; }
}");
}

Expand All @@ -228,12 +270,12 @@ public void SetVirtualPropertyWithPrivateSetter()
{
Analyze<DoNotCallOverridableMethodsInConstructorAnalyzer>(@"class Foo
{
Foo()
{
AutoProperty = 1;
}
Foo()
{
AutoProperty = 1;
}
public virtual int AutoProperty { get; private set; }
public virtual int AutoProperty { get; private set; }
}");
}

Expand Down

0 comments on commit fb6c8e6

Please sign in to comment.