Add the BestFriend attribute for restricting cross-assembly internal access (#1520)

* Add the `BestFriend` attribute for restricting cross-assembly internal access.

Author: TomFinley

src/Microsoft.ML.Core/BestFriendAttribute.cs

@@ -0,0 +1,33 @@
+// 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;
+
+namespace Microsoft.ML
+{
+    /// <summary>
+    /// Intended to be applied to types and members marked as internal to indicate that friend access of this
+    /// internal item is OK from another assembly. This restriction applies only to assemblies that declare the
+    /// <see cref="WantsToBeBestFriendsAttribute"/> assembly level attribute. Note that this attribute is not
+    /// transferrable: an internal member with this attribute does not somehow make a containing internal type
+    /// accessible. Conversely, neither does marking an internal type make any unmarked internal members accessible.
+    /// </summary>
+    [BestFriend]
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Field | AttributeTargets.Property | AttributeTargets.Constructor
+        | AttributeTargets.Method | AttributeTargets.Interface | AttributeTargets.Enum | AttributeTargets.Delegate, AllowMultiple = false, Inherited = false)]
+    internal sealed class BestFriendAttribute : Attribute
+    {
+    }
+
+    /// <summary>
+    /// This is an assembly level attribute to signal that friend accesses on this assembly should be checked
+    /// for usage of <see cref="BestFriendAttribute"/>. If this attribute is missing, normal access rules for
+    /// friends should apply.
+    /// </summary>
+    [BestFriend]
+    [AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false, Inherited = false)]
+    internal sealed class WantsToBeBestFriendsAttribute : Attribute
+    {
+    }
+}

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

@@ -0,0 +1,97 @@
+// 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 Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.ML.CodeAnalyzer.Tests.Helpers;
+using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.IO;
+using System.Linq;
+using System.Reflection;
+using Xunit;
+
+namespace Microsoft.ML.InternalCodeAnalyzer.Tests
+{
+    public sealed class BestFriendTest : DiagnosticVerifier<BestFriendAnalyzer>
+    {
+        // We do things in this somewhat odd way rather than just referencing the Core assembly directly,
+        // because we certainly want the best friend attribute itself to be internal, but the assembly
+        // we build dynamically as part of the test cannot be signed, and so cannot itself be a friend
+        // of the core assembly (even if we were in a mood to pollute the core assembly with friend
+        // declarations to enable this one test). We instead compile the same source, as part of this
+        // dummy assembly. The type name will be the same so the same analyzer will work.
+        private readonly Lazy<string> SourceAttribute = TestUtils.LazySource("BestFriendAttribute.cs");
+        private readonly Lazy<string> SourceDeclaration = TestUtils.LazySource("BestFriendDeclaration.cs");
+        private readonly Lazy<string> SourceUser = TestUtils.LazySource("BestFriendUser.cs");
+
+        [Fact]
+        public void BestFriend()
+        {
+            // The setup to this one is a bit more involved than many of the analyzer tests,
+            // because in this case we have to actually set up *two* assemblies, where the
+            // first considers the second a friend. But, setting up this dependency structure
+            // so that things actually compile to the point where the analyzer can actually do
+            // its work is rather involved.
+            Solution solution = null;
+            var projA = CreateProject("ProjectA", ref solution, SourceDeclaration.Value);
+            var projB = CreateProject("ProjectB", ref solution, SourceUser.Value);
+            solution = solution.AddProjectReference(projB.Id, new ProjectReference(projA.Id));
+
+            var analyzer = new BestFriendAnalyzer();
+
+            MetadataReference peRef;
+            var refs = new[] {
+                    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)
+                };
+            using (var ms = new MemoryStream())
+            {
+                // We also test whether private protected can be accessed, so we need C# 7.2 at least.
+                var parseOpts = new CSharpParseOptions(LanguageVersion.CSharp7_3);
+                var tree = CSharpSyntaxTree.ParseText(SourceDeclaration.Value, parseOpts);
+                var treeAttr = CSharpSyntaxTree.ParseText(SourceAttribute.Value, parseOpts);
+
+                var compOpts = new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary);
+                var innerComp = CSharpCompilation.Create(projA.Name, new[] { tree, treeAttr }, refs, compOpts);
+
+                var emitResult = innerComp.Emit(ms);
+                Assert.True(emitResult.Success, $"Compilation of {projA.Name} did not work. Diagnostics: {string.Join(" || ", emitResult.Diagnostics)}");
+
+                var peImage = ms.ToArray().ToImmutableArray();
+                peRef = MetadataReference.CreateFromImage(peImage);
+            }
+
+            var comp = projB.GetCompilationAsync().Result
+                .WithReferences(refs.Append(peRef).ToArray());
+            var compilationWithAnalyzers = comp.WithAnalyzers(ImmutableArray.Create((DiagnosticAnalyzer)analyzer));
+            var allDiags = compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync().Result;
+
+            var projectTrees = new HashSet<SyntaxTree>(projB.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(10, 31, "A"),
+                diag.CreateDiagnosticResult(11, 31, "A"),
+                diag.CreateDiagnosticResult(11, 33, "My"),
+                diag.CreateDiagnosticResult(14, 33, "Awhile"),
+                diag.CreateDiagnosticResult(15, 33, "And"),
+                diag.CreateDiagnosticResult(18, 13, "A"),
+                diag.CreateDiagnosticResult(18, 25, "A"),
+                diag.CreateDiagnosticResult(25, 13, "IA"),
+                diag.CreateDiagnosticResult(25, 23, "IA"),
+                diag.CreateDiagnosticResult(32, 38, ".ctor"),
+                diag.CreateDiagnosticResult(38, 38, ".ctor"),
+            };
+
+            VerifyDiagnosticResults(diags, analyzer, expected);
+        }
+    }
+}

test/Microsoft.ML.CodeAnalyzer.Tests/Helpers/DiagnosticVerifier.cs

@@ -88,7 +88,7 @@ private void VerifyDiagnostics(string[] sources, DiagnosticAnalyzer analyzer, pa
         /// <param name="actualResults">The Diagnostics found by the compiler after running the analyzer on the source code</param>
         /// <param name="analyzer">The analyzer that was being run on the sources</param>
         /// <param name="expectedResults">Diagnostic Results that should have appeared in the code</param>
-        private static void VerifyDiagnosticResults(IEnumerable<Diagnostic> actualResults, DiagnosticAnalyzer analyzer, params DiagnosticResult[] expectedResults)
+        protected static void VerifyDiagnosticResults(IEnumerable<Diagnostic> actualResults, DiagnosticAnalyzer analyzer, params DiagnosticResult[] expectedResults)
         {
             int expectedCount = expectedResults.Length;
             int actualCount = actualResults.Count();
@@ -265,7 +265,7 @@ private static string FormatDiagnostics(DiagnosticAnalyzer analyzer, params Diag
         private static readonly MetadataReference MLNetCoreReference = RefFromType<Runtime.IHostEnvironment>();
         private static readonly MetadataReference MLNetDataReference = RefFromType<Runtime.Model.ModelLoadContext>();
 
-        private static MetadataReference RefFromType<TType>()
+        protected static MetadataReference RefFromType<TType>()
             => MetadataReference.CreateFromFile(typeof(TType).Assembly.Location);
 
         internal const string DefaultFilePathPrefix = "Test";
@@ -292,40 +292,22 @@ private static Diagnostic[] GetSortedDiagnostics(string[] sources, DiagnosticAna
         /// <param name="analyzer">The analyzer to run on the documents</param>
         /// <param name="documents">The Documents that the analyzer will be run on</param>
         /// <returns>An IEnumerable of Diagnostics that surfaced in the source code, sorted by Location</returns>
-        protected static Diagnostic[] GetSortedDiagnosticsFromDocuments(DiagnosticAnalyzer analyzer, Document[] documents)
+        protected static Diagnostic[] GetSortedDiagnosticsFromDocuments(DiagnosticAnalyzer analyzer, IEnumerable<Document> documents)
         {
             var projects = new HashSet<Project>();
 
-            foreach (var document in documents)
-            {
-                projects.Add(document.Project);
-            }
+            projects.UnionWith(documents.Select(d => d.Project));
 
             var diagnostics = new List<Diagnostic>();
             foreach (var project in projects)
             {
                 var comp = project.GetCompilationAsync().Result;
                 var compilationWithAnalyzers = comp.WithAnalyzers(ImmutableArray.Create(analyzer));
                 var diags = compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync().Result;
-                foreach (var diag in diags)
-                {
-                    if (diag.Location == Location.None || diag.Location.IsInMetadata)
-                    {
-                        diagnostics.Add(diag);
-                    }
-                    else
-                    {
-                        for (int i = 0; i < documents.Length; i++)
-                        {
-                            var document = documents[i];
-                            var tree = document.GetSyntaxTreeAsync().Result;
-                            if (tree == diag.Location.SourceTree)
-                            {
-                                diagnostics.Add(diag);
-                            }
-                        }
-                    }
-                }
+                var projectTrees = new HashSet<SyntaxTree>(documents.Select(r => r.GetSyntaxTreeAsync().Result));
+
+                diagnostics.AddRange(diags.Where(d =>
+                d.Location == Location.None || d.Location.IsInMetadata || projectTrees.Contains(d.Location.SourceTree)));
             }
 
             var results = SortDiagnostics(diagnostics);
@@ -380,14 +362,27 @@ protected static Document CreateDocument(string source)
         /// <param name="sources">Classes in the form of strings</param>
         /// <returns>A Project created out of the Documents created from the source strings</returns>
         private static Project CreateProject(string[] sources)
+        {
+            Solution sol = null;
+            return CreateProject(TestProjectName, ref sol, sources);
+        }
+
+        /// <summary>
+        /// Create a project using the input strings as sources.
+        /// </summary>
+        /// <param name="sources">Classes in the form of strings</param>
+        /// <returns>A Project created out of the Documents created from the source strings</returns>
+        internal static Project CreateProject(string projectName, ref Solution solution, params string[] sources)
         {
             string fileNamePrefix = DefaultFilePathPrefix;
 
-            ProjectId projectId = ProjectId.CreateNewId(debugName: TestProjectName);
+            ProjectId projectId = ProjectId.CreateNewId(debugName: projectName);
+
+            if (solution == null)
+                solution = new AdhocWorkspace().CurrentSolution;
 
-            var solution = new AdhocWorkspace()
-                .CurrentSolution
-                .AddProject(projectId, TestProjectName, TestProjectName, LanguageNames.CSharp)
+            solution = solution
+                .AddProject(projectId, projectName, projectName, LanguageNames.CSharp)
                 .AddMetadataReference(projectId, CorlibReference)
                 .AddMetadataReference(projectId, StandardReference)
                 .AddMetadataReference(projectId, RuntimeReference)

test/Microsoft.ML.CodeAnalyzer.Tests/Helpers/TestUtils.cs

@@ -2,6 +2,7 @@
 // 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.IO;
 using System.Reflection;
 using System.Threading;
@@ -26,13 +27,25 @@ public static ref string EnsureSourceLoaded(ref string source, string resourceNa
         {
             if (source == null)
             {
-                string loadedSource;
+                string loadedSource = LoadSource(resourceName);
                 using (var stream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resourceName))
                 using (var reader = new StreamReader(stream))
                     loadedSource = reader.ReadToEnd();
                 Interlocked.CompareExchange(ref source, loadedSource, null);
             }
             return ref source;
         }
+
+        public static string LoadSource(string resourceName)
+        {
+            using (var stream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resourceName))
+            using (var reader = new StreamReader(stream))
+                return reader.ReadToEnd();
+        }
+
+        public static Lazy<string> LazySource(string resourceName)
+        {
+            return new Lazy<string>(() => LoadSource(resourceName), true);
+        }
     }
 }

test/Microsoft.ML.CodeAnalyzer.Tests/Microsoft.ML.CodeAnalyzer.Tests.csproj

@@ -7,6 +7,12 @@
     </EmbeddedResource>
   </ItemGroup>
 
+  <ItemGroup>
+    <EmbeddedResource Include="..\..\src\Microsoft.ML.Core\BestFriendAttribute.cs" Link="Resources/BestFriendAttribute.cs">
+      <LogicalName>%(Filename)%(Extension)</LogicalName>
+    </EmbeddedResource>
+  </ItemGroup>
+
   <ItemGroup>
     <ProjectReference Include="..\..\tools-local\Microsoft.ML.InternalCodeAnalyzer\Microsoft.ML.InternalCodeAnalyzer.csproj" />
     <ProjectReference Include="..\..\src\Microsoft.ML.Analyzer\Microsoft.ML.Analyzer.csproj" />

test/Microsoft.ML.CodeAnalyzer.Tests/Resources/BestFriendDeclaration.cs

@@ -0,0 +1,47 @@
+using System.Runtime.CompilerServices;
+using Microsoft.ML;
+
+[assembly: InternalsVisibleTo("ProjectB")]
+[assembly: WantsToBeBestFriends]
+
+namespace Bubba
+{
+    internal class A // Should fail.
+    {
+        public const int Hello = 2; // Fine by itself, but reference A.Hello will fail.
+        internal static int My { get; } = 2; // Should also fail on its own merits.
+    }
+
+    [BestFriend]
+    internal class B // Should succeed.
+    {
+        [BestFriend]
+        internal const string Friend = "Wave back when you wave hello."; // Should succeed.
+        public const string Stay = "Don't hold their nose and point at you."; // Should succeed.
+        internal const string Awhile = "Help you find your hat."; // Should Fail.
+
+        public B() { } // Should succeed.
+    }
+
+    public class C : IA
+    {
+        internal const int And = 2; // Should Fail.
+        [BestFriend]
+        internal const int Listen = 2;// Should succeed.
+
+        [BestFriend]
+        private protected C(int a) { } // Should succeed.
+        internal C(float a) { } // Should Fail.
+    }
+
+    public class D : IB
+    {
+        [BestFriend]
+        internal D(int a) { } // Should succeed.
+        private protected D(float a) { } // Should Fail.
+    }
+
+    internal interface IA { } // Should Fail.
+    [BestFriend]
+    internal interface IB { } // Should succeed.
+}

test/Microsoft.ML.CodeAnalyzer.Tests/Resources/BestFriendUser.cs

@@ -0,0 +1,41 @@
+using System;
+using Bubba;
+
+namespace McGee
+{
+    class YoureMyBestFriend
+    {
+        public void Foo()
+        {
+            Console.WriteLine(A.Hello);
+            Console.WriteLine(A.My);
+            Console.WriteLine(B.Friend);
+            Console.WriteLine(B.Stay);
+            Console.WriteLine(B.Awhile);
+            Console.WriteLine(C.And);
+            Console.WriteLine(C.Listen);
+
+            var a = new A();
+            var b = new B();
+            var c = new C(2);
+            c = new C(2.0f);
+            var d = new D(2);
+            d = new D(2.0f);
+
+            var da = (IA)c;
+            var db = (IB)d;
+        }
+
+        public class CDescend : C
+        {
+            public CDescend(int a) : base(a) { }
+            public CDescend(float a) : base(a) { }
+        }
+
+        public class DDescend : D
+        {
+            public DDescend(int a) : base(a) { }
+            public DDescend(float a) : base(a) { }
+        }
+    }
+}