Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MSTEST0036 is shown for cases where no shadowing happens #3881

Merged
merged 6 commits into from
Sep 30, 2024
Merged

Fix MSTEST0036 is shown for cases where no shadowing happens #3881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f6f6ec
update message
engyebrahim Sep 30, 2024
3e49caa
fixed analyzer
engyebrahim Sep 30, 2024
ae0b359
added test
engyebrahim Sep 30, 2024
85a1c9c
fix private case
engyebrahim Sep 30, 2024
2756a94
added test
engyebrahim Sep 30, 2024
9d08adc
add more tetst
engyebrahim Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Analyzers/MSTest.Analyzers/DoNotUseShadowingAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ private static bool IsMemberShadowing(ISymbol member, ISymbol baseMember)
}

// Compare methods
if (member is IMethodSymbol methodSymbol && baseMember is IMethodSymbol baseMethodSymbol)
if (member is IMethodSymbol methodSymbol && baseMember is IMethodSymbol baseMethodSymbol && methodSymbol.IsGenericMethod == baseMethodSymbol.IsGenericMethod
&& !(methodSymbol.DeclaredAccessibility == Accessibility.Private && baseMethodSymbol.DeclaredAccessibility == Accessibility.Private))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also can't be shadowing if only one of the methods is private, or can it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I find the syntax confusing but @engyebrahim added the not before the parenthesis so the and clause becomes a or :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • private and private: !(true && true) = false (ok)
  • public and public: !(false && false) = true (ok)
  • private and public: !(true && false) = true (not ok, is not shadowing)

And I think the visibility check also needs to happen in the "Compare properties" section below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah good point, I overlooked last case. @engyebrahim can you please add more tests and probably fix the remaining broken part?

Copy link
Member Author

@engyebrahim engyebrahim Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public&public raises
public&private raises
image

private&private doesn't raise
private&public doesn't raise -> fixing that case
image

{
return methodSymbol.Name == baseMethodSymbol.Name &&
methodSymbol.Parameters.Length == baseMethodSymbol.Parameters.Length &&
Expand Down
2 changes: 1 addition & 1 deletion src/Analyzers/MSTest.Analyzers/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/Analyzers/MSTest.Analyzers/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ The type declaring these methods should also respect the following rules:
<value>Shadowing test members could cause testing issues (such as NRE).</value>
</data>
<data name="DoNotUseShadowingMessageFormat" xml:space="preserve">
<value>Member '{0}' is already exist in the base class</value>
<value>Member '{0}' already exists in the base class</value>
</data>
<data name="DoNotUseShadowingTitle" xml:space="preserve">
<value>Do not use shadowing</value>
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.cs.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ Typ deklarující tyto metody by měl také respektovat následující pravidla:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Člen {0} už v základní (kořenové) třídě existuje.</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Člen {0} už v základní (kořenové) třídě existuje.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.de.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,8 @@ Der Typ, der diese Methoden deklariert, sollte auch die folgenden Regeln beachte
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Der Member "{0}" ist bereits in der Basisklasse vorhanden.</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Der Member "{0}" ist bereits in der Basisklasse vorhanden.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.es.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ El tipo que declara estos métodos también debe respetar las reglas siguientes:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">El miembro ''{0}'' ya existe en la clase base</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">El miembro ''{0}'' ya existe en la clase base</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ Le type doit être une classe
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Le membre '{0}' existe déjà dans la classe de base</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Le membre '{0}' existe déjà dans la classe de base</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.it.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ Anche il tipo che dichiara questi metodi deve rispettare le regole seguenti:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Il membro '{0}' esiste già nella classe di base</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Il membro '{0}' esiste già nella classe di base</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.ja.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ The type declaring these methods should also respect the following rules:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">メンバー '{0}' は既に基本クラスに存在します</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">メンバー '{0}' は既に基本クラスに存在します</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.ko.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ The type declaring these methods should also respect the following rules:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">멤버 '{0}'이(가) 기본 클래스에 이미 있습니다.</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">멤버 '{0}'이(가) 기본 클래스에 이미 있습니다.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.pl.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ Typ deklarujący te metody powinien również przestrzegać następujących regu
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Element członkowski „{0}” już istnieje w klasie bazowej</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Element członkowski „{0}” już istnieje w klasie bazowej</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.pt-BR.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ O tipo que declara esses métodos também deve respeitar as seguintes regras:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">O membro "{0}" já existe na classe base</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">O membro "{0}" já existe na classe base</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.ru.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,8 @@ The type declaring these methods should also respect the following rules:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Элемент "{0}" уже существует в базовом классе</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Элемент "{0}" уже существует в базовом классе</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.tr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ Bu yöntemleri bildiren tipin ayrıca aşağıdaki kurallara uyması gerekir:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">Üye '{0}' zaten temel sınıfta var</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">Üye '{0}' zaten temel sınıfta var</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hans.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ The type declaring these methods should also respect the following rules:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">基类中已存在成员“{0}”</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">基类中已存在成员“{0}”</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
4 changes: 2 additions & 2 deletions src/Analyzers/MSTest.Analyzers/xlf/Resources.zh-Hant.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,8 +287,8 @@ The type declaring these methods should also respect the following rules:
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingMessageFormat">
<source>Member '{0}' is already exist in the base class</source>
<target state="translated">成員 '{0}' 已存在於基類中</target>
<source>Member '{0}' already exists in the base class</source>
<target state="needs-review-translation">成員 '{0}' 已存在於基類中</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseShadowingTitle">
Expand Down
88 changes: 88 additions & 0 deletions test/UnitTests/MSTest.Analyzers.UnitTests/DoNotUseShadowingAnalyzerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,94 @@ public void Method() { }
await VerifyCS.VerifyAnalyzerAsync(code);
}

public async Task WhenTestClassHaveSameMethodAsBaseClassMethod_ButBothArePrivate_NoDiagnostic()
{
string code = """
using Microsoft.VisualStudio.TestTools.UnitTesting;
public class BaseClass
{
private void Method() { }
}

[TestClass]
public class DerivedClass : BaseClass
{
private void Method() { }
}
""";

await VerifyCS.VerifyAnalyzerAsync(code);
}

public async Task WhenTestClassHaveSameMethodAsBaseClassMethod_ButBaseIsPrivate_Diagnostic()
{
string code = """
using Microsoft.VisualStudio.TestTools.UnitTesting;
public class BaseClass
{
private void Method() { }
}

[TestClass]
public class DerivedClass : BaseClass
{
public void [|Method|]() { }
}
""";

await VerifyCS.VerifyAnalyzerAsync(code);
}

public async Task WhenTestClassHaveSameMethodAsBaseClassMethod_ButDerivedClassIsPrivate_Diagnostic()
{
string code = """
using Microsoft.VisualStudio.TestTools.UnitTesting;
public class BaseClass
{
public void Method() { }
}

[TestClass]
public class DerivedClass : BaseClass
{
private void [|Method|]() { }
}
""";

await VerifyCS.VerifyAnalyzerAsync(code);
}

public async Task WhenTestClassHaveSameMethodAsBaseClassMethod_ButOneIsGeneric_NoDiagnostic()
{
string code = """
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
[TestClass]
public class BaseTest
{
protected TObject CreateObject<TObject>()
{
throw new NotImplementedException();
}
}

[TestClass]
public class ExtendedTest : BaseTest
{
private SomeType CreateObject()
{
throw new NotImplementedException();
}
}

public class SomeType
{
}
""";

await VerifyCS.VerifyAnalyzerAsync(code);
}

public async Task WhenTestClassHaveSameMethodAsBaseClassMethod_WithParameters_Diagnostic()
{
string code = """
Expand Down