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

Fix crash when compiling internal attribute constructor #42192

Merged
merged 3 commits into from
Mar 16, 2020
Merged

Fix crash when compiling internal attribute constructor #42192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a812dc
Fix crash when compiling internal attribute constructor
khyperia Mar 5, 2020
f832237
PR feedback
khyperia Mar 6, 2020
0650a59
Add new rule to refout.md
khyperia Mar 16, 2020
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
7 changes: 5 additions & 2 deletions src/Compilers/CSharp/Portable/Emitter/Model/NamedTypeSymbolAdapter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,10 +661,13 @@ internal virtual bool IsMetadataSealed
{
CheckDefinitionInvariant();

// All constructors in attributes should be emitted
bool isAttribute = DeclaringCompilation.IsAttributeType(this);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

bool isAttribute = DeclaringCompilation.IsAttributeType(this); [](start = 11, length = 63)

We probably should avoid calculating this when nothing is going to be filtered out anyway. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's the best code pattern to do something like this? I feel like Lazy with a lambda is too heavyweight, and a bool hasComputedIsAttribute; bool isAttribute; is kind of gross (so I want confirmation before doing that). (Or did you mean something else by that? Not computing if context.IncludePrivateMembers is true or something like that?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or did you mean something else by that? Not computing if context.IncludePrivateMembers is true or something like that?)

Yes, something like that. Do not calculate if ShouldInclude doesn't filter anything out.

In reply to: 388878100 [](ancestors = 388878100)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also rename the local to something like alwaysIncludeConstructors it will cause less confusion once its value is conditionally calculated. The type doesn't stop to be an attribute no matter what ShouldInclude does.

In reply to: 389016262 [](ancestors = 389016262,388878100)


foreach (var method in this.GetMethodsToEmit())
{
Debug.Assert((object)method != null);
if (method.ShouldInclude(context))
if ((isAttribute && method.MethodKind == MethodKind.Constructor) || method.ShouldInclude(context))
{
yield return method;
}
Expand All @@ -676,7 +679,7 @@ internal virtual bool IsMetadataSealed
{
foreach (var m in generated)
{
if (m.ShouldInclude(context))
if ((isAttribute && m.IsConstructor) || m.ShouldInclude(context))
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IsConstructor [](start = 42, length = 13)

Can we use the same way to perform the constructor check here and above? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surprisingly, no - in the "normal" (not generated) method case, method is a MethodSymbol, but in the generated method case, m is a Cci.IMethodDefinition. It doesn't seem like there's a common API between them to check if it's a constructor.

{
yield return m;
}
Expand Down
70 changes: 70 additions & 0 deletions src/Compilers/CSharp/Test/Emit/Emit/CompilationEmitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,76 @@ public event System.Action PrivateRemover { add { } private remove { } }
);
}

[Fact]
[WorkItem(38444, "https://github.com/dotnet/roslyn/issues/38444")]
public void EmitRefAssembly_InternalAttributeConstructor()
{
CSharpCompilation comp = CreateCompilation(@"
using System;
internal class SomeAttribute : Attribute
{
internal SomeAttribute()
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

internal SomeAttribute() [](start = 4, length = 24)

Please add a regular internal method and verify that it is not emitted. #Closed

{
}
}
[Some]
public class C
{
}
");

using (var output = new MemoryStream())
using (var metadataOutput = new MemoryStream())
{
EmitResult emitResult = comp.Emit(output, metadataPEStream: metadataOutput,
options: new EmitOptions(includePrivateMembers: false));
emitResult.Diagnostics.Verify();
Assert.True(emitResult.Success);

VerifyMethods(output, "C", new[] { "C..ctor()" });
VerifyMethods(metadataOutput, "C", new[] { "C..ctor()" });
VerifyMethods(output, "SomeAttribute", new[] { "SomeAttribute..ctor()" });
VerifyMethods(metadataOutput, "SomeAttribute", new[] { "SomeAttribute..ctor()" });
}
}

[Fact]
[WorkItem(38444, "https://github.com/dotnet/roslyn/issues/38444")]
public void EmitRefAssembly_InternalAttributeConstructor_DoesntIncludeMethods()
{
CSharpCompilation comp = CreateCompilation(@"
using System;
internal class SomeAttribute : Attribute
{
internal SomeAttribute()
{
}

internal void F()
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

internal void F() [](start = 4, length = 17)

Please also add a test with a static constructor. #Closed

{
}
}
[Some]
public class C
{
}
");

using (var output = new MemoryStream())
using (var metadataOutput = new MemoryStream())
{
EmitResult emitResult = comp.Emit(output, metadataPEStream: metadataOutput,
options: new EmitOptions(includePrivateMembers: false));
emitResult.Diagnostics.Verify();
Assert.True(emitResult.Success);

VerifyMethods(output, "C", new[] { "C..ctor()" });
VerifyMethods(metadataOutput, "C", new[] { "C..ctor()" });
VerifyMethods(output, "SomeAttribute", new[] { "SomeAttribute..ctor()", "void SomeAttribute.F()" });
VerifyMethods(metadataOutput, "SomeAttribute", new[] { "SomeAttribute..ctor()" });
}
}

private static void VerifyMethods(MemoryStream stream, string containingType, string[] expectedMethods)
{
stream.Position = 0;
Expand Down