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

Jump to bottom

Potential false negative in CA1047 #5025

Closed
Youssef1313 opened this issue Apr 16, 2021 · 1 comment · Fixed by #5093
Closed

Potential false negative in CA1047 #5025

Youssef1313 opened this issue Apr 16, 2021 · 1 comment · Fixed by #5093
Labels
Bug The product is not behaving according to its current intended design False_Negative No diagnostic is reported for a problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting

Comments

@Youssef1313
Copy link
Member

Analyzer

Diagnostic ID: CA1047: CA1047: Do not declare protected members in sealed types

Describe the bug

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDeclareProtectedMembersInSealedTypes.cs

Line 52 in a0522af

if (!symbol.IsProtected() ||

roslyn-analyzers/src/Utilities/Compiler/Extensions/ISymbolExtensions.cs

Lines 78 to 81 in a0522af

public static bool IsProtected(this ISymbol symbol)
{
return symbol.DeclaredAccessibility == Accessibility.Protected;
}

This doesn't check for protected internal.

Also the following check feels redundant since this rule is specific to protected members only?

roslyn-analyzers/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/DoNotDeclareProtectedMembersInSealedTypes.cs

Lines 46 to 50 in a0522af

// FxCop compat: only analyze externally visible symbols by default.
if (!context.Options.MatchesConfiguredVisibility(Rule, symbol, context.Compilation, context.CancellationToken))
{
return;
}

@mavasani mavasani added Area-Microsoft.CodeQuality.Analyzers Bug The product is not behaving according to its current intended design False_Negative No diagnostic is reported for a problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting labels Apr 22, 2021
@mavasani
Copy link
Contributor

We need to handle both the case below:

  1. private protected
  2. protected internal

@NewellClark NewellClark mentioned this issue May 10, 2021
Merged
NewellClark added a commit to NewellClark/roslyn-analyzers that referenced this issue May 17, 2021
@NewellClark
Fix dotnet#5025
a4a206a 
- Add unit tests for C#
- Add unit tests for 'protected internal' and 'private protected'
- Fix analyzer to also flag 'protected internal' and 'private protected' members
mavasani added a commit that referenced this issue May 18, 2021
@mavasani
Merge pull request #5093 from NewellClark/#5025-false-negative-CA1047
1fa83fe 
Fix #5025
@jmarolf jmarolf mentioned this issue Jan 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug The product is not behaving according to its current intended design False_Negative No diagnostic is reported for a problematic case help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Milestone
No milestone
2 participants
@mavasani @Youssef1313