Add analyzer for detecting BestFriend usages on public declarations #2434

mareklinka · 2019-02-06T16:30:41Z

As proposed in #2415, the BestFriend attribute should never be applied to public declarations. While this is not a problem per se it still introduces unnecessary noise. I implemented a code analyzer that will flag these occurrences as errors whenever a [BestFriend] attribute is applied to a:

public class/struct/enum/interface
public method/property/field/delegate
public constructor

I didn't implement an automated code fix for this diagnostic as fixing the problem is trivial - either remove the attribute or make the declaration internal. If we still want a code fix to be created I'll gladly take care of that as well.

Fixes #2415.

codecov · 2019-02-06T17:07:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d1a2962). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2434   +/-   ##
=========================================
  Coverage          ?   71.22%           
=========================================
  Files             ?      787           
  Lines             ?   140993           
  Branches          ?    16112           
=========================================
  Hits              ?   100417           
  Misses            ?    36109           
  Partials          ?     4467

Flag	Coverage Δ
#Debug	`71.22% <100%> (?)`
#production	`67.57% <ø> (?)`
#test	`85.3% <100%> (?)`

TomFinley

Cool, thanks @mareklinka !! I get the feeling eventually we're going to start paying for these analyses over the semantic model.

Ivanidzo4ka · 2019-02-07T00:28:26Z

tools-local/Microsoft.ML.InternalCodeAnalyzer/BestFriendOnPublicDeclarationsAnalyzer.cs

+        private const string Format = "The " + AttributeName + " should not be applied to publicly visible members.";
+
+        private const string Description =
+            "The " + AttributeName + " attribute is only valid on internal identifiers.";


valid on internal identifiers. [](start = 57, length = 30)

Well, technically this rule is only about public declaration, so no one will stop evil me to put [BestFriend]on private declarations, and this description wouldn't be right.
I'm not saying you should verify private declarations, it's just description and Title slightly out of sync :)

Ivanidzo4ka

sharwell · 2019-02-07T00:33:57Z

test/Microsoft.ML.CodeAnalyzer.Tests/Code/BestFriendOnPublicDeclarationTest.cs

+        private readonly Lazy<string> SourceDeclaration = TestUtils.LazySource("BestFriendOnPublicDeclaration.cs");
+
+        [Fact]
+        public void BestFriendOnPublicDeclaration()


💡 We should really get this switched over to Microsoft.CodeAnalysis.Testing

I feel like the whole analyzer test project could benefit from this. However, this extends beyond the scope of the original issue - could we track this as a separate issue?

could we track this as a separate issue?

Yes, of course 😄

Tracked as #2480.

sharwell · 2019-02-07T00:36:42Z

tools-local/Microsoft.ML.InternalCodeAnalyzer/BestFriendOnPublicDeclarationsAnalyzer.cs

+
+        public override void Initialize(AnalysisContext context)
+        {
+            context.RegisterSemanticModelAction(Analyze);


❗️ Needs to call EnableConcurrentExecution

💡 Should call ConfigureGeneratedCodeAnalysis

sharwell · 2019-02-07T00:39:08Z

tools-local/Microsoft.ML.InternalCodeAnalyzer/BestFriendOnPublicDeclarationsAnalyzer.cs

+
+        public override void Initialize(AnalysisContext context)
+        {
+            context.RegisterSemanticModelAction(Analyze);


❕ For efficiency purposes, this should be a symbol analyzer, not a semantic model analyzer

sharwell · 2019-02-07T00:41:01Z

tools-local/Microsoft.ML.InternalCodeAnalyzer/BestFriendOnPublicDeclarationsAnalyzer.cs

+
+            // Get the symbols of the key types we are analyzing. If we can't find it
+            // there is no point in going further.
+            var bestFriendAttributeType = comp.GetTypeByMetadataName(attributeName);


💡 Ideally this will be called once in a compilation start action and used directly in the symbol analyzer

sfilipi · 2019-02-07T06:07:30Z

@mareklinka thanks for taking care of this.
Have you accepted the license? I see the license/cla in pending state.

mareklinka · 2019-02-07T06:42:52Z

Have you accepted the license? I see the license/cla in pending state.

I definitely should have it signed. I contributed to corefx as well some time ago so it should be sorted out. I also double checked yesterday (I saw the pending check as well) and the .NET foundation CLA page says I signed it. Not sure what's going on.

mareklinka · 2019-02-07T06:51:08Z

Have you accepted the license? I see the license/cla in pending state.

Just pushed new changes and now it reports correctly. Weird.

sharwell · 2019-02-07T16:44:20Z

tools-local/Microsoft.ML.InternalCodeAnalyzer/BestFriendOnPublicDeclarationsAnalyzer.cs

+
+        private void AnalyzeCore(SymbolAnalysisContext context, INamedTypeSymbol attributeType)
+        {
+            if (context.Symbol.DeclaredAccessibility != Accessibility.Public)


📝 This will report a diagnostic on something like the following. You'll want to review and determine if this is desirable.

internal class NotPublicType { [BestFriend] public void DeclaredPublicButNotExposed() { } }

That is a good point. The effective visibility is internal here but a declaration like this is still not "correct". How do you imagine we handle this, @TomFinley?

We could either just keep reporting this as incorrect (simple 😄) or recursively compute the effective visibility. That would require us to walk all the way to the assembly level for each symbol. That might get expensive.

Hi just saw this. If it will report something like this, that is good, not bad. So I'd say @mareklinka's change here is desirable, if it in fact does warn on this.

Indeed, it's unclear to me what the above snippet would do? My best guess is, the thinking that references to NotPublicType should fail, but references to NotPublicType.DeclaredPublicButNotExposed should succeed? If my guess as to what the point is is correct, that is against the spirit of #1519 which is about making it obvious when one of these "cross-assembly-infrastructure" types and members were being declared. So the snippet's usage of [BestFriend] is functionally not useful (since it serves no purpose), which means that I'd think it should warn.

We could either just keep reporting this as incorrect (simple 😄) or recursively compute the effective visibility. That would require us to walk all the way to the assembly level for each symbol. That might get expensive.

It may be expensive, but perhaps more relevant it's against the intended spirit of #1519. The thing I was trying to avoid when we decided, "well, we need to use InternalsVisibleTo to help lock down the public vs. infrastructure code" is to avoid the chaos that would result if usage of internals no longer was useful to distinguish ML.NET wide infrastructure code vs. assembly specific infrastructure code. For it to serve that purpose though, it is crucial that it be explicit. (I think it's fairly important that the maintainers of this code thing of ML.NET wide accessibility of internal classes as somehow something "special." It definitely does not serve my needs to make it easy, since then I expect it will become a wild free for all.)

Add analyzer for detecting BestFriend usages on public declaration

7c7f24e

mareklinka changed the title ~~Add analyzer for detecting BestFriend usages on public declaration~~ Add analyzer for detecting BestFriend usages on public declarations Feb 6, 2019

TomFinley approved these changes Feb 6, 2019

View reviewed changes

Ivanidzo4ka reviewed Feb 7, 2019

View reviewed changes

Ivanidzo4ka approved these changes Feb 7, 2019

View reviewed changes

sharwell reviewed Feb 7, 2019

View reviewed changes

Rewrite analyzer as Symbol analyzer

525ce38

sharwell reviewed Feb 7, 2019

View reviewed changes

mareklinka mentioned this pull request Feb 8, 2019

Microsoft.ML.InternalCodeAnalyzer should use Microsoft.CodeAnalysis.Testing for harness setup #2480

Closed

Add test to prove IntClass.PubMember reports a warning

5e0d7c5

sfilipi merged commit 07580a8 into dotnet:master Feb 9, 2019

ghost locked as resolved and limited conversation to collaborators Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add analyzer for detecting BestFriend usages on public declarations #2434

Add analyzer for detecting BestFriend usages on public declarations #2434

mareklinka commented Feb 6, 2019

codecov bot commented Feb 6, 2019 •

edited

Loading

TomFinley left a comment

Ivanidzo4ka Feb 7, 2019

Ivanidzo4ka left a comment

sharwell Feb 7, 2019

mareklinka Feb 7, 2019

sharwell Feb 7, 2019

mareklinka Feb 8, 2019

sharwell Feb 7, 2019

mareklinka Feb 7, 2019

sharwell Feb 7, 2019

mareklinka Feb 7, 2019

sharwell Feb 7, 2019

sfilipi commented Feb 7, 2019

mareklinka commented Feb 7, 2019

mareklinka commented Feb 7, 2019

sharwell Feb 7, 2019

mareklinka Feb 7, 2019

TomFinley Feb 8, 2019 •

edited

Loading

Add analyzer for detecting BestFriend usages on public declarations #2434

Add analyzer for detecting BestFriend usages on public declarations #2434

Conversation

mareklinka commented Feb 6, 2019

codecov bot commented Feb 6, 2019 • edited Loading

Codecov Report

TomFinley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi commented Feb 7, 2019

mareklinka commented Feb 7, 2019

mareklinka commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2019 •

edited

Loading

TomFinley Feb 8, 2019 •

edited

Loading