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

Generate different DocumentIds for duplicate generators #64729

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
Expand Up @@ -346,7 +346,7 @@ public async ValueTask RefreshFileAsync(CancellationToken cancellationToken)
else
{
// The file isn't there anymore; do we still have the generator at all?
if (project.AnalyzerReferences.Any(a => a.GetGenerators(project.Language).Any(static (g, self) => SourceGeneratorIdentity.GetGeneratorAssemblyName(g) == self._documentIdentity.Generator.AssemblyName, this)))
if (project.AnalyzerReferences.Any(a => a.FullPath == _documentIdentity.Generator.AssemblyPath))
Copy link
Member

Choose a reason for hiding this comment

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

is == ok on filepaths on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally it can be iffy, although in this case the AssemblyPath came from the analzyer reference's path in the first place. I'm going to leave this as is, since at some point this code needs to migrate to potentially run in VS for Mac too, where such a case insensitive comparison would actually be problematic, and no reason to add the extra overhead of the insensitive comparison if not needed.

{
windowFrameMessageToShow = string.Format(ServicesVSResources.The_generator_0_that_generated_this_file_has_stopped_generating_this_file, GeneratorDisplayName);
windowFrameImageMonikerToShow = KnownMonikers.StatusError;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public SourceGeneratorItem(ProjectId projectId, ISourceGenerator generator, Anal
: base(name: SourceGeneratorIdentity.GetGeneratorTypeName(generator))
{
ProjectId = projectId;
Identity = new SourceGeneratorIdentity(generator);
Identity = new SourceGeneratorIdentity(generator, analyzerReference);
AnalyzerReference = analyzerReference;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Text;
using Microsoft.CodeAnalysis.Host;

namespace Microsoft.CodeAnalysis.SourceGeneratorTelemetry
{
internal interface ISourceGeneratorTelemetryCollectorWorkspaceService : IWorkspaceService
{
void CollectRunResult(GeneratorDriverRunResult driverRunResult, GeneratorDriverTimingInfo driverTimingInfo);
void CollectRunResult(GeneratorDriverRunResult driverRunResult, GeneratorDriverTimingInfo driverTimingInfo, ProjectState project);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Internal.Log;
using Roslyn.Utilities;

Expand All @@ -14,15 +17,22 @@ internal class SourceGeneratorTelemetryCollectorWorkspaceService : ISourceGenera
{
private sealed record GeneratorTelemetryKey
{
public GeneratorTelemetryKey(ISourceGenerator generator)
// TODO: mark with SetsRequiredMembers when we have the attributes in place
// [SetsRequiredMembers]
public GeneratorTelemetryKey(ISourceGenerator generator, AnalyzerReference analyzerReference)
{
Identity = new SourceGeneratorIdentity(generator);
FileVersion = FileVersionInfo.GetVersionInfo(generator.GetGeneratorType().Assembly.Location).FileVersion ?? "(null)";
Identity = new SourceGeneratorIdentity(generator, analyzerReference);
FileVersion = "(null)";

if (Identity.AssemblyPath != null)
{
FileVersion = FileVersionInfo.GetVersionInfo(Identity.AssemblyPath).FileVersion;
}
}

// TODO: mark these 'required' when we have the attributes in place
public SourceGeneratorIdentity Identity { get; }
public string FileVersion { get; }
public SourceGeneratorIdentity Identity { get; init; }
public string FileVersion { get; init; }
}

/// <summary>
Expand All @@ -34,19 +44,19 @@ public GeneratorTelemetryKey(ISourceGenerator generator)
private readonly StatisticLogAggregator<GeneratorTelemetryKey> _elapsedTimeByGenerator = new();
private readonly StatisticLogAggregator<GeneratorTelemetryKey> _producedFilesByGenerator = new();

private GeneratorTelemetryKey GetTelemetryKey(ISourceGenerator generator)
=> _generatorTelemetryKeys.GetValue(generator, static g => new GeneratorTelemetryKey(g));
private GeneratorTelemetryKey GetTelemetryKey(ISourceGenerator generator, ProjectState project)
=> _generatorTelemetryKeys.GetValue(generator, g => new GeneratorTelemetryKey(g, project.GetAnalyzerReferenceForGenerator(g)));

public void CollectRunResult(GeneratorDriverRunResult driverRunResult, GeneratorDriverTimingInfo driverTimingInfo)
public void CollectRunResult(GeneratorDriverRunResult driverRunResult, GeneratorDriverTimingInfo driverTimingInfo, ProjectState project)
{
foreach (var generatorTime in driverTimingInfo.GeneratorTimes)
{
_elapsedTimeByGenerator.AddDataPoint(GetTelemetryKey(generatorTime.Generator), generatorTime.ElapsedTime);
_elapsedTimeByGenerator.AddDataPoint(GetTelemetryKey(generatorTime.Generator, project), generatorTime.ElapsedTime);
}

foreach (var generatorResult in driverRunResult.Results)
{
_producedFilesByGenerator.AddDataPoint(GetTelemetryKey(generatorResult.Generator), generatorResult.GeneratedSources.Length);
_producedFilesByGenerator.AddDataPoint(GetTelemetryKey(generatorResult.Generator, project), generatorResult.GeneratedSources.Length);
}
}

Expand Down
34 changes: 26 additions & 8 deletions src/Workspaces/Core/Portable/Workspace/Solution/ProjectState.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ internal partial class ProjectState
private AnalyzerOptions? _lazyAnalyzerOptions;

/// <summary>
/// Backing field for <see cref="SourceGenerators"/>; this is a default ImmutableArray if it hasn't been computed yet.
/// The list of source generators and the analyzer reference they came from.
/// </summary>
private ImmutableArray<ISourceGenerator> _lazySourceGenerators;
private ImmutableDictionary<ISourceGenerator, AnalyzerReference>? _lazySourceGenerators;

private ProjectState(
ProjectInfo projectInfo,
Expand Down Expand Up @@ -796,20 +796,38 @@ public ProjectState WithAnalyzerReferences(IEnumerable<AnalyzerReference> analyz
return With(projectInfo: ProjectInfo.WithAnalyzerReferences(analyzerReferences).WithVersion(Version.GetNewerVersion()));
}

public ImmutableArray<ISourceGenerator> SourceGenerators
[MemberNotNull(nameof(_lazySourceGenerators))]
private void EnsureSourceGeneratorsInitialized()
{
get
if (_lazySourceGenerators == null)
{
if (_lazySourceGenerators.IsDefault)
var builder = ImmutableDictionary.CreateBuilder<ISourceGenerator, AnalyzerReference>();

foreach (var analyzerReference in AnalyzerReferences)
{
var generators = AnalyzerReferences.SelectMany(a => a.GetGenerators(this.Language)).ToImmutableArray();
ImmutableInterlocked.InterlockedInitialize(ref _lazySourceGenerators, generators);
foreach (var generator in analyzerReference.GetGenerators(Language))
builder.Add(generator, analyzerReference);
}

return _lazySourceGenerators;
Interlocked.CompareExchange(ref _lazySourceGenerators, builder.ToImmutable(), comparand: null);
}
}

public IEnumerable<ISourceGenerator> SourceGenerators
{
get
{
EnsureSourceGeneratorsInitialized();
return _lazySourceGenerators.Keys;
}
}

public AnalyzerReference GetAnalyzerReferenceForGenerator(ISourceGenerator generator)
{
EnsureSourceGeneratorsInitialized();
return _lazySourceGenerators[generator];
}

public ProjectState AddDocuments(ImmutableArray<DocumentState> documents)
{
Debug.Assert(!documents.Any(d => DocumentStates.Contains(d.Id)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ public ReplaceGeneratorDriverAction(GeneratorDriver oldGeneratorDriver, ProjectS
var generatorDriver = _oldGeneratorDriver.ReplaceAdditionalTexts(_newProjectState.AdditionalDocumentStates.SelectAsArray(static documentState => documentState.AdditionalText))
.WithUpdatedParseOptions(_newProjectState.ParseOptions!)
.WithUpdatedAnalyzerConfigOptions(_newProjectState.AnalyzerOptions.AnalyzerConfigOptionsProvider)
.ReplaceGenerators(_newProjectState.SourceGenerators);
.ReplaceGenerators(_newProjectState.SourceGenerators.ToImmutableArray());

return generatorDriver;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ private async Task<CompilationInfo> FinalizeCompilationAsync(

generatorInfo = generatorInfo.WithDriver(compilationFactory.CreateGeneratorDriver(
this.ProjectState.ParseOptions!,
ProjectState.SourceGenerators,
ProjectState.SourceGenerators.ToImmutableArray(),
this.ProjectState.AnalyzerOptions.AnalyzerConfigOptionsProvider,
additionalTexts));
}
Expand Down Expand Up @@ -880,7 +880,7 @@ private async Task<CompilationInfo> FinalizeCompilationAsync(

generatorInfo = generatorInfo.WithDriver(generatorInfo.Driver!.RunGenerators(compilationToRunGeneratorsOn, cancellationToken));

solution.Services.GetService<ISourceGeneratorTelemetryCollectorWorkspaceService>()?.CollectRunResult(generatorInfo.Driver!.GetRunResult(), generatorInfo.Driver!.GetTimingInfo());
solution.Services.GetService<ISourceGeneratorTelemetryCollectorWorkspaceService>()?.CollectRunResult(generatorInfo.Driver!.GetRunResult(), generatorInfo.Driver!.GetTimingInfo(), ProjectState);

var runResult = generatorInfo.Driver!.GetRunResult();

Expand All @@ -907,11 +907,14 @@ private async Task<CompilationInfo> FinalizeCompilationAsync(
continue;
}

var generatorAnalyzerReference = this.ProjectState.GetAnalyzerReferenceForGenerator(generatorResult.Generator);

foreach (var generatedSource in generatorResult.GeneratedSources)
{
var existing = FindExistingGeneratedDocumentState(
generatorInfo.Documents,
generatorResult.Generator,
generatorAnalyzerReference,
generatedSource.HintName);

if (existing != null)
Expand All @@ -933,7 +936,8 @@ private async Task<CompilationInfo> FinalizeCompilationAsync(
ProjectState.Id,
generatedSource.HintName,
generatorResult.Generator,
generatedSource.SyntaxTree.FilePath);
generatedSource.SyntaxTree.FilePath,
generatorAnalyzerReference);

generatedDocumentsBuilder.Add(
SourceGeneratedDocumentState.Create(
Expand Down Expand Up @@ -1002,9 +1006,10 @@ private async Task<CompilationInfo> FinalizeCompilationAsync(
static SourceGeneratedDocumentState? FindExistingGeneratedDocumentState(
TextDocumentStates<SourceGeneratedDocumentState> states,
ISourceGenerator generator,
AnalyzerReference analyzerReference,
string hintName)
{
var generatorIdentity = new SourceGeneratorIdentity(generator);
var generatorIdentity = new SourceGeneratorIdentity(generator, analyzerReference);

foreach (var (_, state) in states.States)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.Text;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Roslyn.Utilities;
Expand All @@ -21,23 +22,28 @@ internal readonly record struct SourceGeneratedDocumentIdentity
{
public bool ShouldReuseInSerialization => true;

public static SourceGeneratedDocumentIdentity Generate(ProjectId projectId, string hintName, ISourceGenerator generator, string filePath)
public static SourceGeneratedDocumentIdentity Generate(ProjectId projectId, string hintName, ISourceGenerator generator, string filePath, AnalyzerReference analyzerReference)
{
// We want the DocumentId generated for a generated output to be stable between Compilations; this is so features that track
// a document by DocumentId can find it after some change has happened that requires generators to run again.
// To achieve this we'll just do a crytographic hash of the generator name and hint name; the choice of a cryptographic hash
// as opposed to a more generic string hash is we actually want to ensure we don't have collisions.
var generatorIdentity = new SourceGeneratorIdentity(generator);
var generatorIdentity = new SourceGeneratorIdentity(generator, analyzerReference);

// Combine the strings together; we'll use Encoding.Unicode since that'll match the underlying format; this can be made much
// faster once we're on .NET Core since we could directly treat the strings as ReadOnlySpan<char>.
var projectIdBytes = projectId.Id.ToByteArray();
using var _ = ArrayBuilder<byte>.GetInstance(capacity: (generatorIdentity.AssemblyName.Length + 1 + generatorIdentity.TypeName.Length + 1 + hintName.Length) * 2 + projectIdBytes.Length, out var hashInput);

// The assembly path should exist in any normal scenario; the hashing of the name only would apply if the user loaded a
// dynamic assembly they produced at runtime and passed us that via a custom AnalyzerReference.
var assemblyNameToHash = generatorIdentity.AssemblyPath ?? generatorIdentity.AssemblyName;

using var _ = ArrayBuilder<byte>.GetInstance(capacity: (assemblyNameToHash.Length + 1 + generatorIdentity.TypeName.Length + 1 + hintName.Length) * 2 + projectIdBytes.Length, out var hashInput);
hashInput.AddRange(projectIdBytes);

// Add a null to separate the generator name and hint name; since this is effectively a joining of UTF-16 bytes
// we'll use a UTF-16 null just to make sure there's absolutely no risk of collision.
hashInput.AddRange(Encoding.Unicode.GetBytes(generatorIdentity.AssemblyName));
hashInput.AddRange(Encoding.Unicode.GetBytes(assemblyNameToHash));
hashInput.AddRange(0, 0);
hashInput.AddRange(Encoding.Unicode.GetBytes(generatorIdentity.TypeName));
hashInput.AddRange(0, 0);
Expand All @@ -61,6 +67,7 @@ public void WriteTo(ObjectWriter writer)

writer.WriteString(HintName);
writer.WriteString(Generator.AssemblyName);
writer.WriteString(Generator.AssemblyPath);
writer.WriteString(Generator.AssemblyVersion.ToString());
writer.WriteString(Generator.TypeName);
writer.WriteString(FilePath);
Expand All @@ -72,14 +79,21 @@ internal static SourceGeneratedDocumentIdentity ReadFrom(ObjectReader reader)

var hintName = reader.ReadString();
var generatorAssemblyName = reader.ReadString();
var generatorAssemblyPath = reader.ReadString();
var generatorAssemblyVersion = Version.Parse(reader.ReadString());
var generatorTypeName = reader.ReadString();
var filePath = reader.ReadString();

return new SourceGeneratedDocumentIdentity(
documentId,
hintName,
new SourceGeneratorIdentity(generatorAssemblyName, generatorAssemblyVersion, generatorTypeName),
new SourceGeneratorIdentity
{
AssemblyName = generatorAssemblyName,
AssemblyPath = generatorAssemblyPath,
AssemblyVersion = generatorAssemblyVersion,
TypeName = generatorTypeName
},
filePath);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,38 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Text;
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;

namespace Microsoft.CodeAnalysis
{
internal record struct SourceGeneratorIdentity(string AssemblyName, Version AssemblyVersion, string TypeName)
/// <remarks>
/// Assembly path is used as a part of a generator identity to deal with the case that the user accidentally installed the same
/// generator twice from two different paths, or actually has two different generators that just happened to use the same name.
/// In the wild we've seen cases where a user has a broken project or build that results in the same generator being added twice;
/// we aren't going to try to deduplicate those anywhere since currently the compiler does't do any deduplication either:
/// you'll simply get duplicate outputs which might collide and cause compile errors. If https://github.com/dotnet/roslyn/issues/56619
/// is addressed, we can potentially match the compiler behavior by taking a different approach here.
/// </remarks>
internal record struct SourceGeneratorIdentity
{
public SourceGeneratorIdentity(ISourceGenerator generator)
: this(GetGeneratorAssemblyName(generator), generator.GetGeneratorType().Assembly.GetName().Version!, GetGeneratorTypeName(generator))
{
}
// TODO: mark these 'required' when we have the attributes in place
public string AssemblyName { get; init; }
public string? AssemblyPath { get; init; }
public Version AssemblyVersion { get; init; }
public string TypeName { get; init; }

public static string GetGeneratorAssemblyName(ISourceGenerator generator)
// TODO: mark with SetsRequiredMembers when we have the attributes in place
// [SetsRequiredMembers]
public SourceGeneratorIdentity(ISourceGenerator generator, AnalyzerReference analyzerReference)
{
return generator.GetGeneratorType().Assembly.GetName().Name!;
var generatorType = generator.GetGeneratorType();
var assembly = generatorType.Assembly;
var assemblyName = assembly.GetName();
AssemblyName = assemblyName.Name!;
AssemblyPath = analyzerReference.FullPath;
AssemblyVersion = assemblyName.Version!;
TypeName = generatorType.FullName!;
}

public static string GetGeneratorTypeName(ISourceGenerator generator)
Expand Down
Loading