Skip to content

Add analyzer for detecting BestFriend usages on public declarations #2434

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

Merged
merged 3 commits into from
Feb 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.ML.CodeAnalyzer.Tests.Helpers;
using Xunit;

namespace Microsoft.ML.InternalCodeAnalyzer.Tests
{
public sealed class BestFriendOnPublicDeclarationTest : DiagnosticVerifier<BestFriendOnPublicDeclarationsAnalyzer>
{
private readonly Lazy<string> SourceAttribute = TestUtils.LazySource("BestFriendAttribute.cs");
private readonly Lazy<string> SourceDeclaration = TestUtils.LazySource("BestFriendOnPublicDeclaration.cs");

[Fact]
public void BestFriendOnPublicDeclaration()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

could we track this as a separate issue?

Yes, of course 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracked as #2480.

{
Solution solution = null;
var projA = CreateProject("ProjectA", ref solution, SourceDeclaration.Value, SourceAttribute.Value);

var analyzer = new BestFriendOnPublicDeclarationsAnalyzer();

var refs = new List<MetadataReference> {
RefFromType<object>(), RefFromType<Attribute>(),
MetadataReference.CreateFromFile(Assembly.Load("netstandard, Version=2.0.0.0").Location),
MetadataReference.CreateFromFile(Assembly.Load("System.Runtime, Version=0.0.0.0").Location)
};

var comp = projA.GetCompilationAsync().Result.WithReferences(refs.ToArray());
var compilationWithAnalyzers = comp.WithAnalyzers(ImmutableArray.Create((DiagnosticAnalyzer)analyzer));
var allDiags = compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync().Result;

var projectTrees = new HashSet<SyntaxTree>(projA.Documents.Select(r => r.GetSyntaxTreeAsync().Result));
var diags = allDiags
.Where(d => d.Location == Location.None || d.Location.IsInMetadata || projectTrees.Contains(d.Location.SourceTree))
.OrderBy(d => d.Location.SourceSpan.Start).ToArray();

var diag = analyzer.SupportedDiagnostics[0];
var expected = new DiagnosticResult[] {
diag.CreateDiagnosticResult(8, 6, "PublicClass"),
diag.CreateDiagnosticResult(11, 10, "PublicField"),
diag.CreateDiagnosticResult(14, 10, "PublicProperty"),
diag.CreateDiagnosticResult(20, 10, "PublicMethod"),
diag.CreateDiagnosticResult(26, 10, "PublicDelegate"),
diag.CreateDiagnosticResult(29, 10, "PublicClass"),
diag.CreateDiagnosticResult(35, 6, "PublicStruct"),
diag.CreateDiagnosticResult(40, 6, "PublicEnum"),
diag.CreateDiagnosticResult(47, 6, "PublicInterface"),
diag.CreateDiagnosticResult(102, 10, "PublicMethod")
};

VerifyDiagnosticResults(diags, analyzer, expected);
}
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
using System;
using Microsoft.ML;

namespace TestNamespace
{
// all of the best friend declaration should fail the diagnostic

[BestFriend]
public class PublicClass
{
[BestFriend]
public int PublicField;

[BestFriend]
public string PublicProperty
{
get { return string.Empty; }
}

[BestFriend]
public bool PublicMethod()
{
return true;
}

[BestFriend]
public delegate string PublicDelegate();

[BestFriend]
public PublicClass()
{
}
}

[BestFriend]
public struct PublicStruct
{
}

[BestFriend]
public enum PublicEnum
{
EnumValue1,
EnumValue2
}

[BestFriend]
public interface PublicInterface
{
}

// these should work

[BestFriend]
internal class InternalClass
{
[BestFriend]
internal int InternalField;

[BestFriend]
internal string InternalProperty
{
get { return string.Empty; }
}

[BestFriend]
internal bool InternalMethod()
{
return true;
}

[BestFriend]
internal delegate string InternalDelegate();

[BestFriend]
internal InternalClass()
{
}
}

[BestFriend]
internal struct InternalStruct
{
}

[BestFriend]
internal enum InternalEnum
{
EnumValue1,
EnumValue2
}

[BestFriend]
internal interface InternalInterface
{
}

// this should fail the diagnostic
// a repro for https://github.com/dotnet/machinelearning/pull/2434#discussion_r254770946
internal class InternalClassWithPublicMember
{
[BestFriend]
public void PublicMethod()
{
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Microsoft.ML.InternalCodeAnalyzer
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class BestFriendOnPublicDeclarationsAnalyzer : DiagnosticAnalyzer
{
private const string Category = "Access";
internal const string DiagnosticId = "MSML_BestFriendOnPublicDeclaration";

private const string Title = "Public declarations should not have " + AttributeName + " attribute.";
private const string Format = "The " + AttributeName + " should not be applied to publicly visible members.";

private const string Description =
"The " + AttributeName + " attribute is not valid on public identifiers.";

private static DiagnosticDescriptor Rule =
new DiagnosticDescriptor(DiagnosticId, Title, Format, Category,
DiagnosticSeverity.Warning, isEnabledByDefault: true, description: Description);

private const string AttributeName = "Microsoft.ML.BestFriendAttribute";

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)
{
context.EnableConcurrentExecution();
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);

context.RegisterCompilationStartAction(CompilationStart);
}

private void CompilationStart(CompilationStartAnalysisContext context)
{
var list = new List<string> { AttributeName, "Microsoft.ML.Internal.CpuMath.Core.BestFriendAttribute" };

foreach (var attributeName in list)
{
var attribute = context.Compilation.GetTypeByMetadataName(attributeName);

if (attribute == null)
continue;

context.RegisterSymbolAction(c => AnalyzeCore(c, attribute), SymbolKind.NamedType, SymbolKind.Method, SymbolKind.Field, SymbolKind.Property);
}
}

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

Choose a reason for hiding this comment

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

📝 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() { }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@TomFinley TomFinley Feb 8, 2019

Choose a reason for hiding this comment

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

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.)

return;

var attribute = context.Symbol.GetAttributes().FirstOrDefault(a => a.AttributeClass == attributeType);
if (attribute == null)
return;

var diagnostic = Diagnostic.Create(Rule, attribute.ApplicationSyntaxReference.GetSyntax().GetLocation(), context.Symbol.Name);
context.ReportDiagnostic(diagnostic);
}
}
}