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

Adjust IsMetadataVirtual assertion in MethodToClassRewriter #75066

Merged
merged 3 commits into from
Sep 17, 2024
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 @@ -658,12 +658,24 @@ internal virtual bool IsMetadataFinal
/// signature).
/// WARN WARN WARN: We won't have a final value for this until declaration
/// diagnostics have been computed for all <see cref="SourceMemberContainerTypeSymbol"/>s, so pass
/// ignoringInterfaceImplementationChanges: true if you need a value sooner
/// option: <see cref="IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges"/> if you need a value sooner
/// and aren't concerned about tweaks made to satisfy interface implementation
/// requirements.
/// NOTE: Not ignoring changes can only result in a value that is more true.
///
/// Use IsMetadataVirtualOption.ForceCompleteIfNeeded in DEBUG/assertion code
/// to get the final value.
/// </summary>
internal abstract bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false);
internal abstract bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None);

internal enum IsMetadataVirtualOption
{
None = 0,
IgnoreInterfaceImplementationChanges,
#if DEBUG
ForceCompleteIfNeeded
#endif
}

internal virtual bool ReturnValueIsMarshalledExplicitly
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChang
return false;
}

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,11 @@ public override BoundNode VisitCall(BoundCall node)
var rewrittenArguments = (ImmutableArray<BoundExpression>)this.VisitList(node.Arguments);
var rewrittenType = this.VisitType(node.Type);

Debug.Assert(rewrittenMethodSymbol.IsMetadataVirtual() == node.Method.IsMetadataVirtual());
#if DEBUG
// Calling IsMetadataVirtual with intent of getting a fully accurate result requires the symbol to be fully completed first
Debug.Assert(rewrittenMethodSymbol.IsMetadataVirtual(MethodSymbol.IsMetadataVirtualOption.ForceCompleteIfNeeded)
== node.Method.IsMetadataVirtual(MethodSymbol.IsMetadataVirtualOption.ForceCompleteIfNeeded));
#endif

// If the original receiver was a base access and it was rewritten,
// change the method to point to the wrapper method
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public override bool IsOverride
get { return false; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public override bool IsOverride
get { return true; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public override bool IsOverride
get { return true; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public override bool IsOverride
get { return false; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public override bool IsOverride
get { return true; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ public override bool IsVararg
public override ImmutableHashSet<string> ReturnNotNullIfParameterNotNull => ImmutableHashSet<string>.Empty;
public override FlowAnalysisAnnotations FlowAnalysisAnnotations => FlowAnalysisAnnotations.None;
internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None) => false;
internal sealed override UnmanagedCallersOnlyAttributeData? GetUnmanagedCallersOnlyAttributeData(bool forceComplete) => null;

internal override bool GenerateDebugInfo => throw ExceptionUtilities.Unreachable();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ public override int Arity

public override bool IsStatic => HasFlag(MethodAttributes.Static);

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => HasFlag(MethodAttributes.Virtual);
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None) => HasFlag(MethodAttributes.Virtual);

internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => HasFlag(MethodAttributes.NewSlot);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public static bool IsRuntimeFinalizer(this MethodSymbol method, bool skipFirstMe
// IsMetadataVirtualIgnoringInterfaceImplementationChanges. This also has the advantage of making
// this method safe to call before declaration diagnostics have been computed.
if ((object)method == null || method.Name != WellKnownMemberNames.DestructorName ||
method.ParameterCount != 0 || method.Arity != 0 || !method.IsMetadataVirtual(ignoreInterfaceImplementationChanges: true))
method.ParameterCount != 0 || method.Arity != 0 || !method.IsMetadataVirtual(MethodSymbol.IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges))
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1016,7 +1016,7 @@ internal static MethodSymbol GetFirstRuntimeOverriddenMethodIgnoringNewSlot(this
// be computed, then it is important for us to pass ignoreInterfaceImplementationChanges: true
// (see MethodSymbol.IsMetadataVirtual for details).
// Since we are only concerned with overrides (of class methods), interface implementations can be ignored.
const bool ignoreInterfaceImplementationChanges = true;
const MethodSymbol.IsMetadataVirtualOption ignoreInterfaceImplementationChanges = MethodSymbol.IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges;

wasAmbiguous = false;
if (!method.IsMetadataVirtual(ignoreInterfaceImplementationChanges) || method.IsStatic)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ internal sealed override bool HasAsyncMethodBuilderAttribute(out TypeSymbol buil

internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) { throw ExceptionUtilities.Unreachable(); }

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) { throw ExceptionUtilities.Unreachable(); }
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None) { throw ExceptionUtilities.Unreachable(); }

internal override bool IsMetadataFinal
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ protected override void NoteAttributesComplete(bool forReturnType) { }

internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None) => false;

internal override int CalculateLocalSyntaxOffset(int localPosition, SyntaxTree localTree)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ internal override OneOrMany<SyntaxList<AttributeListSyntax>> GetReturnTypeAttrib
return OneOrMany.Create(default(SyntaxList<AttributeListSyntax>));
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1838,7 +1838,7 @@ private void CheckInterfaceUnification(BindingDiagnosticBag diagnostics)
needSynthesizedImplementation = false;
}
}
else if (implementingMethod.IsMetadataVirtual(ignoreInterfaceImplementationChanges: true))
else if (implementingMethod.IsMetadataVirtual(MethodSymbol.IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges))
{
// If the signatures match and the implementation method is definitely virtual, then we're set.
needSynthesizedImplementation = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -554,13 +554,20 @@ internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChang
// in NamedTypeSymbolAdapter.cs).
return this.IsOverride ?
this.RequiresExplicitOverride(out _) :
!this.IsStatic && this.IsMetadataVirtual(ignoreInterfaceImplementationChanges);
!this.IsStatic && this.IsMetadataVirtual(ignoreInterfaceImplementationChanges ? IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges : IsMetadataVirtualOption.None);
}

// TODO (tomat): sealed?
internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return this.flags.IsMetadataVirtual(ignoreInterfaceImplementationChanges);
#if DEBUG
if (option == IsMetadataVirtualOption.ForceCompleteIfNeeded && !this.flags.IsMetadataVirtualLocked)
{
this.ContainingSymbol.ForceComplete(locationOpt: null, filter: null, cancellationToken: CancellationToken.None);
}
#endif

return this.flags.IsMetadataVirtual(ignoreInterfaceImplementationChanges: option == IsMetadataVirtualOption.IgnoreInterfaceImplementationChanges);
}

internal void EnsureMetadataVirtual()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChang
return true;
}

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ internal sealed override bool RequiresSecurityObject
get { return _interfaceMethod.RequiresSecurityObject; }
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return !IsStatic;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChang
return false;
}

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChang
return false;
}

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ internal override System.Reflection.MethodImplAttributes ImplementationAttribute
}
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ internal sealed override bool IsMetadataNewSlot(bool ignoreInterfaceImplementati
return false;
}

internal sealed override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal sealed override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ public override bool IsImplicitlyDeclared
}
}

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false)
internal override bool IsMetadataVirtual(IsMetadataVirtualOption option = IsMetadataVirtualOption.None)
{
return UnderlyingMethod.IsMetadataVirtual(ignoreInterfaceImplementationChanges);
return UnderlyingMethod.IsMetadataVirtual(option);
}

internal override bool IsMetadataFinal
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8585,5 +8585,86 @@ async Task<int> Do()
comp.VerifyEmitDiagnostics();
CompileAndVerify(comp, expectedOutput: ExpectedOutput("42"), verify: Verification.FailsPEVerify);
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/73563")]
public void IsMetadataVirtual_01()
{
var src1 = @"
using System.Collections.Generic;
using System.Threading;

public struct S : IAsyncEnumerable<int>
{
public IAsyncEnumerator<int> GetAsyncEnumerator(CancellationToken token = default) => throw null;

void M()
{
GetAsyncEnumerator();
}
}
";

var comp1 = CreateCompilation(src1, targetFramework: TargetFramework.Net80);

var src2 = @"
using System.Threading.Tasks;

class C
{
static async Task Main()
{
await foreach (var i in new S())
{
}
}
}
";
var comp2 = CreateCompilation(src2, references: [comp1.ToMetadataReference()], targetFramework: TargetFramework.Net80);
comp2.VerifyEmitDiagnostics(); // Indirectly calling IsMetadataVirtual on S.GetAsyncEnumerator (a read which causes the lock to be set)
comp1.VerifyEmitDiagnostics(); // Would call EnsureMetadataVirtual on S.GetAsyncEnumerator and would therefore assert if S was not already ForceCompleted
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/73563")]
public void IsMetadataVirtual_02()
{
var src1 = @"
using System;
using System.Threading.Tasks;

public struct S2 : IAsyncDisposable
{
public ValueTask DisposeAsync()
{
return ValueTask.CompletedTask;
}

void M()
{
DisposeAsync();
}
}
";

var comp1 = CreateCompilation(src1, targetFramework: TargetFramework.Net80);

var src2 = @"
class C
{
static async System.Threading.Tasks.Task Main()
{
await using (new S2())
{
}

await using (var s = new S2())
{
}
}
}
";
var comp2 = CreateCompilation(src2, references: [comp1.ToMetadataReference()], targetFramework: TargetFramework.Net80);
comp2.VerifyEmitDiagnostics(); // Indirectly calling IsMetadataVirtual on S.DisposeAsync (a read which causes the lock to be set)
comp1.VerifyEmitDiagnostics(); // Would call EnsureMetadataVirtual on S.DisposeAsync and would therefore assert if S was not already ForceCompleted
}
}
}
Loading
Loading