Skip to content
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
2 changes: 1 addition & 1 deletion src/ILInspector.JsExportSurface/JsonUnionWireRules.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ internal static IEnumerable<ApiMember> CaseConstructors(ApiType type) =>
&& !member.IsStatic
&& member.Accessibility is null or "public"
&& member.SignatureModel is { Parameters.Count: 1 } signature
&& signature.Parameters[0].Modifier is not ("ref" or "in" or "out"));
&& signature.Parameters[0].Modifier is not ("ref" or "in" or "out" or "ref readonly"));

internal static JsExportUnion Describe(
ApiType type,
Expand Down
12 changes: 2 additions & 10 deletions src/ILInspector.Metadata/ApiSurfaceExtractor.MethodSignatures.cs
Original file line number Diff line number Diff line change
Expand Up @@ -444,16 +444,8 @@ private static (string? name, bool isParams, string? refKind, bool hasDefault, o
handle,
beforeRetain: beforeRetain,
beforeMaterialize: beforeMaterialize);
// An interop-marshalled `ref` parameter sets both In and Out, so
// neither flag alone identifies a C# `out`/`in`. Spelling such a
// parameter `out` breaks definite assignment in the body.
bool isOut = (param.Attributes & System.Reflection.ParameterAttributes.Out) != 0;
bool isIn = (param.Attributes & System.Reflection.ParameterAttributes.In) != 0;
string? refKind = isOut && !isIn
? "out"
: isIn && !isOut
? "in"
: null;
// Classified for every parameter row; consumed only when the signature type is by-reference.
string? refKind = ParameterByRefKinds.Classify(reader, param, beforeMaterialize).CSharpModifier();

bool hasDefault = (param.Attributes & System.Reflection.ParameterAttributes.HasDefault) != 0;
object? defaultValue = null;
Expand Down
9 changes: 2 additions & 7 deletions src/ILInspector.Metadata/MetadataDeclarationQuery.cs
Original file line number Diff line number Diff line change
Expand Up @@ -858,13 +858,8 @@ static ParameterInfo GetParameterInfo(MetadataReader reader, ParameterHandleColl
var attributes = parameter.GetCustomAttributes();
var isParams = AttributeReader.HasAttribute(reader, attributes, "System.ParamArrayAttribute")
|| AttributeReader.HasAttribute(reader, attributes, KnownAttributeNames.ParamCollectionAttribute);
var isOut = (parameter.Attributes & ParameterAttributes.Out) != 0;
var isIn = (parameter.Attributes & ParameterAttributes.In) != 0;
var refKind = isOut && !isIn
? "out"
: isIn && !isOut
? "in"
: null;
// Classified for every parameter row; consumed only when the signature type is by-reference.
var refKind = ParameterByRefKinds.Classify(reader, parameter).CSharpModifier();
var defaultParameter = (parameter.Attributes & ParameterAttributes.HasDefault) != 0
? parameter
: (Parameter?)null;
Expand Down
77 changes: 77 additions & 0 deletions src/ILInspector.MetadataPrimitives/ParameterByRefKind.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
using System.Reflection;
using System.Reflection.Metadata;

namespace ILInspector.Metadata;

/// <summary>The C# passing mode of a parameter whose signature type is by-reference.</summary>
public enum ParameterByRefKind
{
/// <summary><c>ref</c>: a mutable reference, including interop <c>[In] ref</c> and <c>[In, Out] ref</c>.</summary>
Ref,

/// <summary><c>out</c>: the <c>Out</c> flag without <c>In</c>.</summary>
Out,

/// <summary><c>in</c>: marked with <c>IsReadOnlyAttribute</c>.</summary>
In,

/// <summary><c>ref readonly</c>: marked with <c>RequiresLocationAttribute</c>.</summary>
RefReadOnly,
}

/// <summary>
/// The one classification of a by-reference parameter's C# passing mode from metadata.
/// The raw <c>In</c> and <c>Out</c> parameter flags are interop marshalling directions,
/// not C# modifiers: <c>[In] ref</c> and <c>[In, Out] ref</c> are mutable <c>ref</c>
/// parameters. C# <c>in</c> and <c>ref readonly</c> are identified only by their
/// compiler attributes (both compilers also set the <c>In</c> flag).
/// </summary>
/// <remarks>
/// Precedence follows Roslyn's metadata reader: <c>Out</c> without <c>In</c> is
/// <c>out</c> before either attribute is consulted, then <c>IsReadOnlyAttribute</c>
/// is <c>in</c>, then <c>RequiresLocationAttribute</c> is <c>ref readonly</c>, and
/// everything else is <c>ref</c>.
/// </remarks>
public static class ParameterByRefKinds
{
const string IsReadOnlyAttribute = "System.Runtime.CompilerServices.IsReadOnlyAttribute";
const string RequiresLocationAttribute = "System.Runtime.CompilerServices.RequiresLocationAttribute";

/// <summary>
/// Classifies <paramref name="parameter"/>, whose signature type the caller has
/// already established is by-reference.
/// </summary>
public static ParameterByRefKind Classify(
MetadataReader reader,
Parameter parameter,
Action<int>? beforeMaterialize = null)
{
var flags = parameter.Attributes & (ParameterAttributes.In | ParameterAttributes.Out);
if (flags == ParameterAttributes.Out)
return ParameterByRefKind.Out;

bool requiresLocation = false;
foreach (var handle in parameter.GetCustomAttributes())
{
string? name = AttributeDecoder.GetAttributeTypeName(
reader,
reader.GetCustomAttribute(handle).Constructor,
beforeMaterialize);
if (name == IsReadOnlyAttribute)
return ParameterByRefKind.In;
requiresLocation |= name == RequiresLocationAttribute;
}

return requiresLocation ? ParameterByRefKind.RefReadOnly : ParameterByRefKind.Ref;
}

/// <summary>The C# declaration modifier for <paramref name="kind"/>.</summary>
public static string CSharpModifier(this ParameterByRefKind kind)
=> kind switch
{
ParameterByRefKind.Out => "out",
ParameterByRefKind.In => "in",
ParameterByRefKind.RefReadOnly => "ref readonly",
_ => "ref",
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -1334,15 +1334,15 @@ public void AbbreviatedMemberDeclaration_PreservesParameterModifiers()
{
Name = "M",
Kind = "method",
Signature = "void M(ref int value, out string text, in long source, params byte[] bytes)"
Signature = "void M(ref int value, out string text, in long source, ref readonly System.Guid id, params byte[] bytes)"
};

var declaration = CSharpDeclarationWriter.RenderMemberDeclaration(
type,
member,
new CSharpDeclarationOptions { AbbreviateSignature = true });

Assert.Equal("public void M(ref int, out string, in long, params byte[])", declaration);
Assert.Equal("public void M(ref int, out string, in long, ref readonly System.Guid, params byte[])", declaration);
}

[Fact]
Expand Down
20 changes: 20 additions & 0 deletions tests/ILInspector.JsExportSurface.Tests/JsonUnionWireTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,26 @@ public void Build_DoesNotDropAnUnreadableConstructorFromTheCaseSet()
Assert.Null(union.IncludesNull);
}

/// <remarks>
/// Case constructors are by-value; every by-ref passing mode the API surface spells,
/// including <c>ref readonly</c>, stays out of the case set.
/// </remarks>
[Theory]
[InlineData("ref")]
[InlineData("in")]
[InlineData("out")]
[InlineData("ref readonly")]
public void CaseConstructors_ExcludeEveryByRefPassingMode(string modifier)
{
ApiSurface api = Extract(nameof(UnionExports.GetScalar));
ApiType type = Assert.Single(api.Types, type => type.Name == nameof(ScalarUnion));
ApiMember[] constructors = JsonUnionWireRules.CaseConstructors(type).ToArray();
ApiMember byRef = constructors[0];
byRef.SignatureModel!.Parameters[0].Modifier = modifier;

Assert.Equal(constructors[1..], JsonUnionWireRules.CaseConstructors(type));
}

[Theory]
[InlineData(nameof(UnionExports.GetOrdinary))]
[InlineData(nameof(UnionExports.GetPlain))]
Expand Down
24 changes: 24 additions & 0 deletions tests/ILInspector.Metadata.Tests/ApiSignatureModelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,22 @@ public void MethodSignatureModel_SuppressesCompilerReservedRefReadonlyParameterA
Assert.Empty(member.SignatureModel.Parameters[0].Attributes);
}

/// <remarks>
/// The API surface spells by-ref passing from compiler attributes, not the raw
/// interop <c>In</c> flag (#3537), so <c>[In] ref</c> stays a mutable <c>ref</c>.
/// </remarks>
[Fact]
public void MethodSignatureModel_SpellsByRefParameterPassingFromCompilerAttributes()
{
var member = GetMember(nameof(ApiSignatureFixtures), nameof(ApiSignatureFixtures.MethodWithByRefPassing));

Assert.NotNull(member.SignatureModel);
Assert.Equal(
["ref", "ref", "ref", "out", "in", "ref readonly"],
member.SignatureModel.Parameters.Select(parameter => parameter.Modifier));
Assert.Contains("ref int interopIn, ref int interopInOut, out int result, in int readOnly, ref readonly int location", member.Signature);
}

[Fact]
public void MethodSignatureModel_EscapesCharArguments()
{
Expand Down Expand Up @@ -836,6 +852,14 @@ public void MethodWithRefReadonlyParameter(ref readonly int value)
{
}

public void MethodWithByRefPassing(
ref int plain,
[System.Runtime.InteropServices.In] ref int interopIn,
[System.Runtime.InteropServices.In, System.Runtime.InteropServices.Out] ref int interopInOut,
out int result,
in int readOnly,
ref readonly int location) => result = plain + interopIn + interopInOut + readOnly + location;

public void MethodWithQuoteCharAttribute([ParameterCharMarker('\'')] string value)
{
}
Expand Down
115 changes: 115 additions & 0 deletions tests/ILInspector.Metadata.Tests/MetadataDeclarationQueryTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.Reflection;
using System.Reflection.Emit;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;
using System.Runtime.InteropServices;
using ILInspector.Metadata;
Expand Down Expand Up @@ -41,6 +42,112 @@ public void MethodDeclaration_ExposesAccessibilityModifiersParametersAndReturnAt
Assert.Equal("2", declaration.Signature.Parameters[1].DefaultValueText);
}

/// <remarks>
/// The raw <c>In</c>/<c>Out</c> parameter flags are interop marshalling directions:
/// <c>[In] ref</c> and <c>[In, Out] ref</c> are mutable <c>ref</c> parameters (#3537).
/// C# <c>in</c> and <c>ref readonly</c> are identified by their compiler attributes.
/// </remarks>
[Theory]
[InlineData(0, "ref")]
[InlineData(1, "ref")]
[InlineData(2, "ref")]
[InlineData(3, "out")]
[InlineData(4, "in")]
[InlineData(5, "ref readonly")]
public void MethodDeclaration_SpellsByRefParameterPassingFromCompilerAttributes(int index, string modifier)
{
var type = GetTypeDefinition(typeof(MetadataDeclarationQueryFixtures));
var method = GetMethod(type, nameof(MetadataDeclarationQueryFixtures.ByRefPassing));

var declaration = MetadataDeclarationQuery.GetMethod(Reader, type, method);

Assert.Equal(modifier, declaration.Signature.Parameters[index].Modifier);
Assert.Equal("int", declaration.Signature.Parameters[index].Type);
}

/// <remarks>
/// Roslyn reads <c>Out</c> without <c>In</c> as <c>out</c> before it consults
/// <c>IsReadOnlyAttribute</c> or <c>RequiresLocationAttribute</c>. The compiler never
/// emits that combination, so the fixture's <c>in</c> and <c>ref readonly</c> rows are
/// patched to carry only the <c>Out</c> flag.
/// </remarks>
[Theory]
[InlineData(4)]
[InlineData(5)]
public void MethodDeclaration_SpellsOutOnlyFlagAsOutBeforeCompilerAttributes(int index)
{
byte[] image = File.ReadAllBytes(typeof(MetadataDeclarationQueryTests).Assembly.Location);
var type = GetTypeDefinition(typeof(MetadataDeclarationQueryFixtures));
var method = GetMethod(type, nameof(MetadataDeclarationQueryFixtures.ByRefPassing));
var parameterHandle = method.GetParameters()
.Single(handle => Reader.GetParameter(handle).SequenceNumber == index + 1);
int row = MetadataTokens.GetRowNumber(parameterHandle);
int flagsOffset = PeReader.PEHeaders.MetadataStartOffset
+ Reader.GetTableMetadataOffset(TableIndex.Param)
+ (row - 1) * Reader.GetTableRowSize(TableIndex.Param);
System.Buffers.Binary.BinaryPrimitives.WriteUInt16LittleEndian(
image.AsSpan(flagsOffset),
(ushort)ParameterAttributes.Out);

using var patched = new PEReader(new MemoryStream(image));
var reader = patched.GetMetadataReader();
var (patchedType, patchedMethod) = FindMethod(
reader,
typeof(MetadataDeclarationQueryFixtures).Namespace!,
nameof(MetadataDeclarationQueryFixtures),
nameof(MetadataDeclarationQueryFixtures.ByRefPassing));

var declaration = MetadataDeclarationQuery.GetMethod(reader, patchedType, patchedMethod);

Assert.Equal("out", declaration.Signature.Parameters[index].Modifier);
Assert.Equal("ref", declaration.Signature.Parameters[0].Modifier);
}

/// <remarks>
/// The motivating real asset (#3537): the runtime declares
/// <c>FindConnectionPoint([In] ref Guid riid, out IConnectionPoint ppCP)</c>, which was
/// rendered as <c>in System.Guid riid</c>, a readonly reference callers cannot assign through.
/// </remarks>
[Fact]
public void MethodDeclaration_SpellsCoreLibInteropInRefParameterAsRef()
{
using var peReader = new PEReader(File.OpenRead(
typeof(System.Runtime.InteropServices.ComTypes.IConnectionPointContainer).Assembly.Location));
var reader = peReader.GetMetadataReader();
var (type, method) = FindMethod(
reader,
"System.Runtime.InteropServices.ComTypes",
"IConnectionPointContainer",
"FindConnectionPoint");

var declaration = MetadataDeclarationQuery.GetMethod(reader, type, method);

Assert.Equal("ref", declaration.Signature.Parameters[0].Modifier);
Assert.Equal("out", declaration.Signature.Parameters[1].Modifier);
}

static (TypeDefinition Type, MethodDefinition Method) FindMethod(
MetadataReader reader,
string @namespace,
string typeName,
string methodName)
{
foreach (var typeHandle in reader.TypeDefinitions)
{
var type = reader.GetTypeDefinition(typeHandle);
if (reader.GetString(type.Namespace) != @namespace || reader.GetString(type.Name) != typeName)
continue;
foreach (var methodHandle in type.GetMethods())
{
var method = reader.GetMethodDefinition(methodHandle);
if (reader.GetString(method.Name) == methodName)
return (type, method);
}
}

throw new InvalidOperationException($"Method '{@namespace}.{typeName}.{methodName}' was not found.");
}

[Fact]
public void InitOnlySetter_PreservesReturnModifierDistinction()
{
Expand Down Expand Up @@ -615,6 +722,14 @@ public class MetadataDeclarationQueryFixtures

public decimal DecimalDefault(decimal amount = 5m) => amount;

public void ByRefPassing(
ref int plain,
[In] ref int interopIn,
[In, Out] ref int interopInOut,
out int result,
in int readOnly,
ref readonly int location) => result = plain + interopIn + interopInOut + readOnly + location;

public int Count() => _count;

public int @class() => 0;
Expand Down