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.

Sign up for GitHub

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

Jump to bottom

Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for EventSourceGeneration). #71662

Merged
merged 8 commits into from
Jul 12, 2022
Merged

Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for EventSourceGeneration). #71662

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
498b5e6
Polyfill the incremental generator ForAttributeWithMetadataName from …
CyrusNajmabadi Jul 5, 2022
6cade39
Move common code to shared location
CyrusNajmabadi Jul 5, 2022
f8b212b
Polyfill the incremental generator ForAttributeWithMetadataName from …
CyrusNajmabadi Jul 5, 2022
2ed84a2
Update src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.…
CyrusNajmabadi Jul 7, 2022
b2847be
Revert
CyrusNajmabadi Jul 12, 2022
e09862d
Merge branch 'eventSource' of https://github.com/CyrusNajmabadi/runti…
CyrusNajmabadi Jul 12, 2022
4ce9f76
Simplify
CyrusNajmabadi Jul 12, 2022
c2a0af8
Merge remote-tracking branch 'upstream/main' into eventSource
CyrusNajmabadi Jul 12, 2022
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
141 changes: 59 additions & 82 deletions src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,119 +4,96 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
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
{
public partial class EventSourceGenerator
{
private static bool IsSyntaxTargetForGeneration(SyntaxNode node, CancellationToken cancellationToken) =>
node is ClassDeclarationSyntax { AttributeLists.Count: > 0 };

private static EventSourceClass? GetSemanticTargetForGeneration(GeneratorSyntaxContext context, CancellationToken cancellationToken)
private static EventSourceClass? GetSemanticTargetForGeneration(GeneratorAttributeSyntaxContext context, CancellationToken cancellationToken)
{
const string EventSourceAutoGenerateAttribute = "System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute";
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't need to check for EventSourceAutoGenerateAttribute. we wouldn't have gotten here unless we had that attribute on the class we're examining.

const string EventSourceAttribute = "System.Diagnostics.Tracing.EventSourceAttribute";

var classDef = (ClassDeclarationSyntax)context.Node;
SemanticModel sm = context.SemanticModel;
var classDef = (ClassDeclarationSyntax)context.TargetNode;
NamespaceDeclarationSyntax? ns = classDef.Parent as NamespaceDeclarationSyntax;
if (ns is null)
{
if (classDef.Parent is not CompilationUnitSyntax)
{
// since this generator doesn't know how to generate a nested type...
return null;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

moved this check up. there was no point doing it in a loop since it would always be the same result. and if it always caused us to continue, we'd never set eventSourceClass. so we'd terminate the loop and would return null. so this is effectively the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

note: this logic is wrong in that it can't handle: namespace n1 { namespace n2 { class Whatever { } } }. However, that bug existed before. I didn't change it.

A better check would likely be to do if (classDef.Parent is TypeDeclarationSyntax)


EventSourceClass? eventSourceClass = null;
string? nspace = null;

bool autoGenerate = false;
foreach (AttributeListSyntax cal in classDef.AttributeLists)
foreach (AttributeData attribute in context.TargetSymbol.GetAttributes())
{
foreach (AttributeSyntax ca in cal.Attributes)
if (attribute.AttributeClass?.Name != "EventSourceAttribute" ||
Copy link
Member Author

Choose a reason for hiding this comment

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

in general, try to do the basic name check first. it's much faster and allocation-light versus ToDisplayString

attribute.AttributeClass.ToDisplayString() != EventSourceAttribute)
{
if (sm.GetSymbolInfo(ca, cancellationToken).Symbol is not IMethodSymbol caSymbol)
{
// badly formed attribute definition, or not the right attribute
continue;
}
continue;
}

string attributeFullName = caSymbol.ContainingType.ToDisplayString();
nspace ??= ConstructNamespace(ns);

if (attributeFullName.Equals(EventSourceAutoGenerateAttribute, StringComparison.Ordinal))
{
autoGenerate = true;
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

we required having EventSourceAutoGenerateAttribute before. otherwise, autogenerate would not be true, and we'd bail out below.

}
string className = classDef.Identifier.ValueText;
string name = className;
string guid = "";

if (!attributeFullName.Equals(EventSourceAttribute, StringComparison.Ordinal))
{
continue;
}
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 code below would only execute if we had EventSourceAttribute, so that's what we maintain below.

ImmutableArray<KeyValuePair<string, TypedConstant>> args = attribute.NamedArguments;
foreach (KeyValuePair<string, TypedConstant> arg in args)
{
string argName = arg.Key;
string value = arg.Value.Value?.ToString();

string nspace = string.Empty;
NamespaceDeclarationSyntax? ns = classDef.Parent as NamespaceDeclarationSyntax;
if (ns is null)
{
if (classDef.Parent is not CompilationUnitSyntax)
{
// since this generator doesn't know how to generate a nested type...
continue;
}
}
else
switch (argName)
{
nspace = ns.Name.ToString();
while (true)
{
ns = ns.Parent as NamespaceDeclarationSyntax;
if (ns == null)
{
break;
}

nspace = $"{ns.Name}.{nspace}";
}
case "Guid":
guid = value;
break;
case "Name":
name = value;
break;
}
}

string className = classDef.Identifier.ToString();
string name = className;
string guid = "";
if (!Guid.TryParse(guid, out Guid result))
{
result = GenerateGuidFromName(name.ToUpperInvariant());
}

SeparatedSyntaxList<AttributeArgumentSyntax>? args = ca.ArgumentList?.Arguments;
if (args is not null)
{
foreach (AttributeArgumentSyntax arg in args)
{
string argName = arg.NameEquals!.Name.Identifier.ToString();
string value = sm.GetConstantValue(arg.Expression, cancellationToken).ToString();

switch (argName)
{
case "Guid":
guid = value;
break;
case "Name":
name = value;
break;
}
}
}
eventSourceClass = new EventSourceClass(nspace, className, name, result);
continue;
}

if (!Guid.TryParse(guid, out Guid result))
{
result = GenerateGuidFromName(name.ToUpperInvariant());
}
return eventSourceClass;
}

eventSourceClass = new EventSourceClass(nspace, className, name, result);
continue;
}
}
private static string? ConstructNamespace(NamespaceDeclarationSyntax? ns)
{
if (ns is null)
return string.Empty;

if (!autoGenerate)
string nspace = ns.Name.ToString();
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

note. this is not correct. if you have namespace N1/*comment*/. N2, then the ToString will include hte extraneous comments and whitespace. However, this was the logic from before, so i'm keeping it.

while (true)
{
return null;
ns = ns.Parent as NamespaceDeclarationSyntax;
if (ns == null)
{
break;
}

nspace = $"{ns.Name}.{nspace}";
}

return eventSourceClass;
return nspace;
}

// From System.Private.CoreLib
Expand Down
10 changes: 8 additions & 2 deletions src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions;

namespace Generators
{
Expand Down Expand Up @@ -35,9 +36,14 @@ public partial class EventSourceGenerator : IIncrementalGenerator

public void Initialize(IncrementalGeneratorInitializationContext context)
{
const string EventSourceAutoGenerateAttribute = "System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute";

IncrementalValuesProvider<EventSourceClass> eventSourceClasses =
context.SyntaxProvider
.CreateSyntaxProvider(IsSyntaxTargetForGeneration, GetSemanticTargetForGeneration)
context.SyntaxProvider.ForAttributeWithMetadataName(
context,
EventSourceAutoGenerateAttribute,
(node, _) => node is ClassDeclarationSyntax,
GetSemanticTargetForGeneration)
.Where(x => x is not null);

context.RegisterSourceOutput(eventSourceClasses, EmitSourceFile);
Expand Down
15 changes: 15 additions & 0 deletions src/libraries/System.Private.CoreLib/gen/System.Private.CoreLib.Generators.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,21 @@
<Compile Include="$(CoreLibSharedDir)\System\Runtime\CompilerServices\IsExternalInit.cs" Link="Common\System\Runtime\CompilerServices\IsExternalInit.cs" />
</ItemGroup>

<ItemGroup>
<Compile Include="$(CommonPath)Roslyn\GetBestTypeByMetadataName.cs" Link="Common\Roslyn\GetBestTypeByMetadataName.cs" />
<Compile Include="$(CommonPath)Roslyn\Hash.cs" Link="Common\Roslyn\Hash.cs" />
<Compile Include="$(CommonPath)Roslyn\ISyntaxHelper.cs" Link="Common\Roslyn\ISyntaxHelper.cs" />
<Compile Include="$(CommonPath)Roslyn\CSharpSyntaxHelper.cs" Link="Common\Roslyn\CSharpSyntaxHelper.cs" />
<Compile Include="$(CommonPath)Roslyn\GlobalAliases.cs" Link="Common\Roslyn\GlobalAliases.cs" />
<Compile Include="$(CommonPath)Roslyn\SyntaxNodeGrouping.cs" Link="Common\Roslyn\SyntaxNodeGrouping.cs" />
<Compile Include="$(CommonPath)Roslyn\SyntaxValueProvider.ImmutableArrayValueComparer.cs" Link="Common\Roslyn\SyntaxValueProvider.ImmutableArrayValueComparer.cs" />
<Compile Include="$(CommonPath)Roslyn\SyntaxValueProvider_ForAttributeWithMetadataName.cs" Link="Common\Roslyn\SyntaxValueProvider_ForAttributeWithMetadataName.cs" />
<Compile Include="$(CommonPath)Roslyn\SyntaxValueProvider_ForAttributeWithSimpleName.cs" Link="Common\Roslyn\SyntaxValueProvider_ForAttributeWithSimpleName.cs" />

<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.cs" Link="Production\ValueListBuilder.cs" />
<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.Pop.cs" Link="Production\ValueListBuilder.Pop.cs" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" PrivateAssets="all" Version="$(MicrosoftCodeAnalysisCSharpVersion)" />
</ItemGroup>
Expand Down
0 ...llections/Generic/ValueListBuilder.Pop.cs → ...llections/Generic/ValueListBuilder.Pop.cs
View file
Open in desktop
File renamed without changes.
4 changes: 2 additions & 2 deletions ...aries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,13 +358,13 @@ private static IncrementalStubGenerationContext CalculateStubInformation(
// Since the char type in an array will not be part of the P/Invoke signature, we can
// use the regular blittable marshaller in all cases.
new CharMarshallingGeneratorFactory(generatorFactory, useBlittableMarshallerForUtf16: true),
new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, Scenario.ElementIn, Scenario.ElementRef, Scenario.ElementOut));
new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, MarshalMode.ElementIn, MarshalMode.ElementRef, MarshalMode.ElementOut));
// We don't need to include the later generator factories for collection elements
// as the later generator factories only apply to parameters.
generatorFactory = new AttributedMarshallingModelGeneratorFactory(
generatorFactory,
elementFactory,
new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, Scenario.ManagedToUnmanagedIn, Scenario.ManagedToUnmanagedRef, Scenario.ManagedToUnmanagedOut));
new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, MarshalMode.ManagedToUnmanagedIn, MarshalMode.ManagedToUnmanagedRef, MarshalMode.ManagedToUnmanagedOut));
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not clear on why these changes are necessary in this PR. Is this PR renaming Scenario to MarshalMode or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

These were bad merges as i was undoing the changes. The files are now untouched. Note: this will be squashed when PR is merged, so there won't be random cruft like this in your repo history. However, i don't like squashing as the PR is in progress as that can make it harder to see what's going on. If you want though i can pre-squash.

Copy link
Member

Choose a reason for hiding this comment

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

If you want though i can pre-squash.

Nope.


generatorFactory = new ByValueContentsMarshalKindValidator(generatorFactory);
}
Expand Down
2 changes: 1 addition & 1 deletion ...raries/System.Text.RegularExpressions/gen/System.Text.RegularExpressions.Generator.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
<Compile Include="$(CommonPath)System\HexConverter.cs" Link="Production\HexConverter.cs" />
<Compile Include="$(CommonPath)System\Text\ValueStringBuilder.cs" Link="Production\ValueStringBuilder.cs" />
<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.cs" Link="Production\ValueListBuilder.cs" />
<Compile Include="..\src\System\Collections\Generic\ValueListBuilder.Pop.cs" Link="Production\ValueListBuilder.Pop.cs" />
<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.Pop.cs" Link="Production\ValueListBuilder.Pop.cs" />
<Compile Include="..\src\System\Threading\StackHelper.cs" Link="Production\StackHelper.cs" />
<Compile Include="..\src\System\Text\RegularExpressions\RegexCaseEquivalences.Data.cs" Link="Production\RegexCaseEquivalences.Data.cs" />
<Compile Include="..\src\System\Text\RegularExpressions\RegexCaseEquivalences.cs" Link="Production\RegexCaseEquivalences.cs" />
Expand Down