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.

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

Conversation

engyebrahim
Copy link
Member

fix: #3824

Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

We have another FP when the methods are private (add the same private method in current and base and you will see a false positive).

@Evangelink Evangelink merged commit f731b27 into main Sep 30, 2024
6 checks passed
@Evangelink Evangelink deleted the enji/bug-fix branch September 30, 2024 13:02
@@ -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

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

@engyebrahim engyebrahim mentioned this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSTEST0036 is shown for cases where no shadowing happens
3 participants