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

Src gen: use GetBestTypeByMetadataName instead of GetTypeByMetadataName #59092

Merged
merged 11 commits into from
Sep 29, 2021
4 changes: 2 additions & 2 deletions src/libraries/Common/src/Roslyn/GetBestTypeByMetadataName.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@

using Microsoft.CodeAnalysis;

namespace System.Text.Json.Reflection
namespace Microsoft.CodeAnalysis.DotnetRuntime.Extensions
{
internal static partial class RoslynExtensions
internal static class RoslynExtensions
{
// Copied from: https://github.com/dotnet/roslyn/blob/main/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/CompilationExtensions.cs
/// <summary>
Expand Down
27 changes: 19 additions & 8 deletions src/libraries/Common/tests/SourceGenerators/RoslynTestUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ public static Project CreateTestProject(IEnumerable<Assembly>? references, bool
}

return new AdhocWorkspace()
.AddSolution(SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Create()))
.AddProject("Test", "test.dll", "C#")
.WithMetadataReferences(refs)
.WithCompilationOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary).WithNullableContextOptions(NullableContextOptions.Enable));
.AddSolution(SolutionInfo.Create(SolutionId.CreateNewId(), VersionStamp.Create()))
.AddProject("Test", "test.dll", "C#")
.WithMetadataReferences(refs)
.WithCompilationOptions(new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary).WithNullableContextOptions(NullableContextOptions.Enable));
}

public static Task CommitChanges(this Project proj, params string[] ignorables)
Expand Down Expand Up @@ -152,13 +152,24 @@ public static TextSpan MakeSpan(string text, int spanNum)
CancellationToken cancellationToken = default)
{
Project proj = CreateTestProject(references, includeBaseReferences);

proj = proj.WithDocuments(sources);

Assert.True(proj.Solution.Workspace.TryApplyChanges(proj.Solution));

Compilation? comp = await proj!.GetCompilationAsync(CancellationToken.None).ConfigureAwait(false);
return RunGenerator(comp!, generator, cancellationToken);
}

/// <summary>
/// Runs a Roslyn generator given a Compilation.
/// </summary>
public static (ImmutableArray<Diagnostic>, ImmutableArray<GeneratedSourceResult>) RunGenerator(
Compilation compilation,
#if ROSLYN4_0_OR_GREATER
IIncrementalGenerator generator,
#else
ISourceGenerator generator,
#endif
CancellationToken cancellationToken = default)
{
#if ROSLYN4_0_OR_GREATER
// workaround https://github.com/dotnet/roslyn/pull/55866. We can remove "LangVersion=Preview" when we get a Roslyn build with that change.
CSharpParseOptions options = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview);
Expand All @@ -167,7 +178,7 @@ public static TextSpan MakeSpan(string text, int spanNum)
CSharpGeneratorDriver cgd = CSharpGeneratorDriver.Create(new[] { generator });
#endif

GeneratorDriver gd = cgd.RunGenerators(comp!, cancellationToken);
GeneratorDriver gd = cgd.RunGenerators(compilation, cancellationToken);

GeneratorDriverRunResult r = gd.GetRunResult();
return (r.Results[0].Diagnostics, r.Results[0].GeneratedSources);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;

namespace Microsoft.Extensions.Logging.Generators
{
Expand Down Expand Up @@ -65,28 +66,28 @@ internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) =>
/// </summary>
public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSyntax> classes)
{
INamedTypeSymbol loggerMessageAttribute = _compilation.GetTypeByMetadataName(LoggerMessageAttribute);
INamedTypeSymbol loggerMessageAttribute = _compilation.GetBestTypeByMetadataName(LoggerMessageAttribute);
if (loggerMessageAttribute == null)
{
// nothing to do if this type isn't available
return Array.Empty<LoggerClass>();
}

INamedTypeSymbol loggerSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.ILogger");
INamedTypeSymbol loggerSymbol = _compilation.GetBestTypeByMetadataName("Microsoft.Extensions.Logging.ILogger");
if (loggerSymbol == null)
{
// nothing to do if this type isn't available
return Array.Empty<LoggerClass>();
}

INamedTypeSymbol logLevelSymbol = _compilation.GetTypeByMetadataName("Microsoft.Extensions.Logging.LogLevel");
INamedTypeSymbol logLevelSymbol = _compilation.GetBestTypeByMetadataName("Microsoft.Extensions.Logging.LogLevel");
if (logLevelSymbol == null)
{
// nothing to do if this type isn't available
return Array.Empty<LoggerClass>();
}

INamedTypeSymbol exceptionSymbol = _compilation.GetTypeByMetadataName("System.Exception");
INamedTypeSymbol exceptionSymbol = _compilation.GetBestTypeByMetadataName("System.Exception");
if (exceptionSymbol == null)
{
Diag(DiagnosticDescriptors.MissingRequiredType, null, "System.Exception");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,8 @@
<PackageReference Include="Microsoft.DotNet.Build.Tasks.Packaging" Version="$(MicrosoftDotNetBuildTasksPackagingVersion)" PrivateAssets="all" />
</ItemGroup>

<ItemGroup>
<Compile Include="$(CommonPath)\Roslyn\GetBestTypeByMetadataName.cs" Link="Common\Roslyn\GetBestTypeByMetadataName.cs" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.IO;
using System.Reflection;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;

namespace Microsoft.Extensions.Logging.Generators.Tests
{
public class CompilationHelper
{
public static Compilation CreateCompilation(
string source,
MetadataReference[]? additionalReferences = null,
string assemblyName = "TestAssembly")
{
string corelib = Assembly.GetAssembly(typeof(object))!.Location;
string runtimeDir = Path.GetDirectoryName(corelib)!;

var refs = new List<MetadataReference>();
refs.Add(MetadataReference.CreateFromFile(corelib));
refs.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "netstandard.dll")));
refs.Add(MetadataReference.CreateFromFile(Path.Combine(runtimeDir, "System.Runtime.dll")));
refs.Add(MetadataReference.CreateFromFile(typeof(ILogger).Assembly.Location));
refs.Add(MetadataReference.CreateFromFile(typeof(LoggerMessageAttribute).Assembly.Location));

if (additionalReferences != null)
{
foreach (MetadataReference reference in additionalReferences)
{
refs.Add(reference);
}
}

CSharpParseOptions options = CSharpParseOptions.Default;

#if ROSLYN4_0_OR_GREATER
// workaround https://github.com/dotnet/roslyn/pull/55866. We can remove "LangVersion=Preview" when we get a Roslyn build with that change.
options = options.WithLanguageVersion(LanguageVersion.Preview);
#endif

return CSharpCompilation.Create(
assemblyName,
syntaxTrees: new[] { CSharpSyntaxTree.ParseText(source, options) },
references: refs.ToArray(),
options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)
);
}

public static byte[] CreateAssemblyImage(Compilation compilation)
{
MemoryStream ms = new MemoryStream();
var emitResult = compilation.Emit(ms);
if (!emitResult.Success)
{
throw new InvalidOperationException();
}
return ms.ToArray();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -679,6 +680,55 @@ static partial class C
Assert.Empty(diagnostics); // should fail quietly on broken code
}

[Fact]
internal void MultipleTypeDefinitions()
{
// Adding a dependency to an assembly that has internal definitions of public types
// should not result in a collision and break generation.
// Verify usage of the extension GetBestTypeByMetadataName(this Compilation) instead of Compilation.GetTypeByMetadataName().
var referencedSource = @"
namespace Microsoft.Extensions.Logging
{
internal class LoggerMessageAttribute { }
}
namespace Microsoft.Extensions.Logging
{
internal interface ILogger { }
internal enum LogLevel { }
}";

// Compile the referenced assembly first.
Compilation referencedCompilation = CompilationHelper.CreateCompilation(referencedSource);

// Obtain the image of the referenced assembly.
byte[] referencedImage = CompilationHelper.CreateAssemblyImage(referencedCompilation);

// Generate the code
string source = @"
namespace Test
{
using Microsoft.Extensions.Logging;

partial class C
{
[LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = ""M1"")]
static partial void M1(ILogger logger);
}
}";

MetadataReference[] additionalReferences = { MetadataReference.CreateFromImage(referencedImage) };
Compilation compilation = CompilationHelper.CreateCompilation(source, additionalReferences);
LoggerMessageGenerator generator = new LoggerMessageGenerator();

(ImmutableArray<Diagnostic> diagnostics, ImmutableArray<GeneratedSourceResult> generatedSources) =
RoslynTestUtils.RunGenerator(compilation, generator);

// Make sure compilation was successful.
Assert.Empty(diagnostics);
Assert.Equal(1, generatedSources.Length);
Assert.Equal(21, generatedSources[0].SourceText.Lines.Count);
}

private static async Task<IReadOnlyList<Diagnostic>> RunGenerator(
string code,
bool wrap = true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@
using System.Security.Cryptography;
using System.Text;
using System.Threading;

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;

namespace Generators
{
Expand All @@ -31,14 +31,14 @@ public Parser(Compilation compilation, Action<Diagnostic> reportDiagnostic, Canc

public EventSourceClass[] GetEventSourceClasses(List<ClassDeclarationSyntax> classDeclarations)
{
INamedTypeSymbol? autogenerateAttribute = _compilation.GetTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute");
INamedTypeSymbol? autogenerateAttribute = _compilation.GetBestTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute");
if (autogenerateAttribute is null)
{
// No EventSourceAutoGenerateAttribute
return Array.Empty<EventSourceClass>();
}

INamedTypeSymbol? eventSourceAttribute = _compilation.GetTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAttribute");
INamedTypeSymbol? eventSourceAttribute = _compilation.GetBestTypeByMetadataName("System.Diagnostics.Tracing.EventSourceAttribute");
if (eventSourceAttribute is null)
{
// No EventSourceAttribute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<Compile Include="$(LibrariesProjectRoot)\System.Private.CoreLib\generators\EventSourceGenerator.cs" />
<Compile Include="$(LibrariesProjectRoot)\System.Private.CoreLib\generators\EventSourceGenerator.Emitter.cs" />
<Compile Include="$(LibrariesProjectRoot)\System.Private.CoreLib\generators\EventSourceGenerator.Parser.cs" />
<Compile Include="$(CommonPath)\Roslyn\GetBestTypeByMetadataName.cs" Link="Common\Roslyn\GetBestTypeByMetadataName.cs" />
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\UnconditionalSuppressMessageAttribute.cs" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;
using Microsoft.CodeAnalysis.Text;

namespace System.Text.Json.SourceGeneration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Reflection;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;

namespace System.Text.Json.Reflection
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

namespace System.Text.Json.Reflection
{
internal static partial class RoslynExtensions
internal static class RoslynExtensions
layomia marked this conversation as resolved.
Show resolved Hide resolved
{
public static Type AsType(this ITypeSymbol typeSymbol, MetadataLoadContextInternal metadataLoadContext)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ public void TestMultipleDefinitions()
{
// Adding a dependency to an assembly that has internal definitions of public types
// should not result in a collision and break generation.
// This verifies the usage of GetBestTypeByMetadataName() instead of GetTypeByMetadataName().
// Verify usage of the extension GetBestTypeByMetadataName(this Compilation) instead of Compilation.GetTypeByMetadataName().
var referencedSource = @"
namespace System.Text.Json.Serialization
{
Expand All @@ -487,16 +487,7 @@ internal class JsonSourceGenerationOptionsAttribute { }
Compilation referencedCompilation = CompilationHelper.CreateCompilation(referencedSource);

// Obtain the image of the referenced assembly.
byte[] referencedImage;
using (MemoryStream ms = new MemoryStream())
{
var emitResult = referencedCompilation.Emit(ms);
if (!emitResult.Success)
{
throw new InvalidOperationException();
}
referencedImage = ms.ToArray();
}
byte[] referencedImage = CompilationHelper.CreateAssemblyImage(referencedCompilation);

// Generate the code
string source = @"
Expand Down
12 changes: 12 additions & 0 deletions src/libraries/tests.proj
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@
<TestTrimming Condition="'$(TestTrimming)' == ''">false</TestTrimming>
</PropertyGroup>

<ItemGroup Condition="'$(TargetsMobile)' == 'true' and '$(TargetOS)' != 'Browser'">
<!-- Microsoft.CodeAnalysis.* assemblies missing in the virtual file system for Browser and the bundle for the mobile platforms -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetOS)' == 'Browser' and '$(BuildAOTTestsOnHelix)' == 'true' and '$(RunDisabledWasmTests)' != 'true' and '$(RunAOTCompilation)' == 'true'">
Copy link
Member

@lewing lewing Sep 21, 2021

Choose a reason for hiding this comment

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

you shouldn't need a new group for this, the exclusions 4 lines above should be enough but it looks like the paths are incorrect? I see

./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj
./Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous section (Condition="'$(TargetsMobile)' == 'true' and '$(TargetOS)' != 'Browser'") also excludes the same tests.

I obtained the conditions from #54833 which is per #51961 (but for System.Text.Json).

The paths appear correct to me and are consistent with other references to them.

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the path start with Microsoft.Extensions.Logging.Abstractions\tests not Microsoft.Extensions.Logging\tests ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes thanks. Missed that.

@eerhardt it looks like the test exclusion added in 9035d94 has the wrong path. I left it as-is in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

since it's pointing at the wrong path and passing those old exclusions can probably be removed. I'll do it in a separate pr

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW Eric fixed that in #59608

<!-- Exceeds VM resources in CI on compilation: https://github.com/dotnet/runtime/issues/51961 -->
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging.Abstractions\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn3.11.Tests.csproj" />
<ProjectExclusions Include="$(MSBuildThisFileDirectory)Microsoft.Extensions.Logging.Abstractions\tests\Microsoft.Extensions.Logging.Generators.Tests\Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj" />
</ItemGroup>

<!-- Projects that don't support code coverage measurement. -->
<ItemGroup Condition="'$(Coverage)' == 'true'">
<ProjectExclusions Include="$(CommonTestPath)Common.Tests.csproj" />
Expand Down