diff --git a/src/ILInspector.JsExportSurface/JsonUnionWireRules.cs b/src/ILInspector.JsExportSurface/JsonUnionWireRules.cs index 57519d2231..fb62627ba4 100644 --- a/src/ILInspector.JsExportSurface/JsonUnionWireRules.cs +++ b/src/ILInspector.JsExportSurface/JsonUnionWireRules.cs @@ -11,7 +11,7 @@ internal static IEnumerable 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, diff --git a/src/ILInspector.Metadata/ApiSurfaceExtractor.MethodSignatures.cs b/src/ILInspector.Metadata/ApiSurfaceExtractor.MethodSignatures.cs index a68a5d5256..626e882591 100644 --- a/src/ILInspector.Metadata/ApiSurfaceExtractor.MethodSignatures.cs +++ b/src/ILInspector.Metadata/ApiSurfaceExtractor.MethodSignatures.cs @@ -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; diff --git a/src/ILInspector.Metadata/MetadataDeclarationQuery.cs b/src/ILInspector.Metadata/MetadataDeclarationQuery.cs index 45c9c81120..7c3c729daf 100644 --- a/src/ILInspector.Metadata/MetadataDeclarationQuery.cs +++ b/src/ILInspector.Metadata/MetadataDeclarationQuery.cs @@ -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; diff --git a/src/ILInspector.MetadataPrimitives/ParameterByRefKind.cs b/src/ILInspector.MetadataPrimitives/ParameterByRefKind.cs new file mode 100644 index 0000000000..4aec14660b --- /dev/null +++ b/src/ILInspector.MetadataPrimitives/ParameterByRefKind.cs @@ -0,0 +1,77 @@ +using System.Reflection; +using System.Reflection.Metadata; + +namespace ILInspector.Metadata; + +/// The C# passing mode of a parameter whose signature type is by-reference. +public enum ParameterByRefKind +{ + /// ref: a mutable reference, including interop [In] ref and [In, Out] ref. + Ref, + + /// out: the Out flag without In. + Out, + + /// in: marked with IsReadOnlyAttribute. + In, + + /// ref readonly: marked with RequiresLocationAttribute. + RefReadOnly, +} + +/// +/// The one classification of a by-reference parameter's C# passing mode from metadata. +/// The raw In and Out parameter flags are interop marshalling directions, +/// not C# modifiers: [In] ref and [In, Out] ref are mutable ref +/// parameters. C# in and ref readonly are identified only by their +/// compiler attributes (both compilers also set the In flag). +/// +/// +/// Precedence follows Roslyn's metadata reader: Out without In is +/// out before either attribute is consulted, then IsReadOnlyAttribute +/// is in, then RequiresLocationAttribute is ref readonly, and +/// everything else is ref. +/// +public static class ParameterByRefKinds +{ + const string IsReadOnlyAttribute = "System.Runtime.CompilerServices.IsReadOnlyAttribute"; + const string RequiresLocationAttribute = "System.Runtime.CompilerServices.RequiresLocationAttribute"; + + /// + /// Classifies , whose signature type the caller has + /// already established is by-reference. + /// + public static ParameterByRefKind Classify( + MetadataReader reader, + Parameter parameter, + Action? 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; + } + + /// The C# declaration modifier for . + public static string CSharpModifier(this ParameterByRefKind kind) + => kind switch + { + ParameterByRefKind.Out => "out", + ParameterByRefKind.In => "in", + ParameterByRefKind.RefReadOnly => "ref readonly", + _ => "ref", + }; +} diff --git a/tests/ILInspector.CSharp.Tests/CSharpDeclarationWriterTests.cs b/tests/ILInspector.CSharp.Tests/CSharpDeclarationWriterTests.cs index 7d2af9273d..adb101602e 100644 --- a/tests/ILInspector.CSharp.Tests/CSharpDeclarationWriterTests.cs +++ b/tests/ILInspector.CSharp.Tests/CSharpDeclarationWriterTests.cs @@ -1334,7 +1334,7 @@ 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( @@ -1342,7 +1342,7 @@ public void AbbreviatedMemberDeclaration_PreservesParameterModifiers() 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] diff --git a/tests/ILInspector.JsExportSurface.Tests/JsonUnionWireTests.cs b/tests/ILInspector.JsExportSurface.Tests/JsonUnionWireTests.cs index 22ff248ddd..5300b14014 100644 --- a/tests/ILInspector.JsExportSurface.Tests/JsonUnionWireTests.cs +++ b/tests/ILInspector.JsExportSurface.Tests/JsonUnionWireTests.cs @@ -167,6 +167,26 @@ public void Build_DoesNotDropAnUnreadableConstructorFromTheCaseSet() Assert.Null(union.IncludesNull); } + /// + /// Case constructors are by-value; every by-ref passing mode the API surface spells, + /// including ref readonly, stays out of the case set. + /// + [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))] diff --git a/tests/ILInspector.Metadata.Tests/ApiSignatureModelTests.cs b/tests/ILInspector.Metadata.Tests/ApiSignatureModelTests.cs index 0ee6382072..44f6a40bd9 100644 --- a/tests/ILInspector.Metadata.Tests/ApiSignatureModelTests.cs +++ b/tests/ILInspector.Metadata.Tests/ApiSignatureModelTests.cs @@ -370,6 +370,22 @@ public void MethodSignatureModel_SuppressesCompilerReservedRefReadonlyParameterA Assert.Empty(member.SignatureModel.Parameters[0].Attributes); } + /// + /// The API surface spells by-ref passing from compiler attributes, not the raw + /// interop In flag (#3537), so [In] ref stays a mutable ref. + /// + [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() { @@ -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) { } diff --git a/tests/ILInspector.Metadata.Tests/MetadataDeclarationQueryTests.cs b/tests/ILInspector.Metadata.Tests/MetadataDeclarationQueryTests.cs index 842fd24daf..ab0417db59 100644 --- a/tests/ILInspector.Metadata.Tests/MetadataDeclarationQueryTests.cs +++ b/tests/ILInspector.Metadata.Tests/MetadataDeclarationQueryTests.cs @@ -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; @@ -41,6 +42,112 @@ public void MethodDeclaration_ExposesAccessibilityModifiersParametersAndReturnAt Assert.Equal("2", declaration.Signature.Parameters[1].DefaultValueText); } + /// + /// The raw In/Out parameter flags are interop marshalling directions: + /// [In] ref and [In, Out] ref are mutable ref parameters (#3537). + /// C# in and ref readonly are identified by their compiler attributes. + /// + [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); + } + + /// + /// Roslyn reads Out without In as out before it consults + /// IsReadOnlyAttribute or RequiresLocationAttribute. The compiler never + /// emits that combination, so the fixture's in and ref readonly rows are + /// patched to carry only the Out flag. + /// + [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); + } + + /// + /// The motivating real asset (#3537): the runtime declares + /// FindConnectionPoint([In] ref Guid riid, out IConnectionPoint ppCP), which was + /// rendered as in System.Guid riid, a readonly reference callers cannot assign through. + /// + [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() { @@ -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;