Repository navigation
Spell by-ref parameters from compiler attributes, not the interop In flag - #10033
Merged
Merged
Conversation
…flag The raw In/Out parameter flags are interop marshalling directions, not C# modifiers. Both Metadata declaration paths spelled any In-flagged by-ref parameter as `in`, so the runtime's FindConnectionPoint([In] ref Guid riid) rendered as `in System.Guid riid`, a readonly reference callers cannot assign through, and RTS delegate and virtual-method shells gained a false modreq(InAttribute). ParameterByRefKinds (MetadataPrimitives) is now the one classification: IsReadOnlyAttribute -> in, RequiresLocationAttribute -> ref readonly, Out without In -> out, otherwise ref. MetadataDeclarationQuery and ApiSurfaceExtractor both use it. Fixes #3537. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016JAYbAeLyaZdJSXjXNjGCP
…readonly union case constructors Round 1 review of #10033: - ParameterByRefKinds.Classify now reads Out without In as out before consulting IsReadOnlyAttribute or RequiresLocationAttribute, matching Roslyn's metadata reader. A patched-flag theory covers both attributes. - JsonUnionWireRules.CaseConstructors excludes "ref readonly" alongside ref, in and out, so a [Union] convention type with a ref readonly constructor keeps its by-value case set. - Caller comments now say the classification runs for every parameter row and is consumed only for by-ref signature types. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016JAYbAeLyaZdJSXjXNjGCP
Owner
Author
|
Review clean at head
Merge needs explicit authorization. Generated by Claude Code |
This was referenced Oct 13, 2026
richlander
added a commit
that referenced
this pull request
Oct 16, 2026
MethodDefinitionFacts.ReadParameterRefKinds now classifies each by-ref parameter through the shared Metadata classifier (#10033), so call-site spelling follows Roslyn's precedence: Out without In is out before either compiler attribute is consulted. in and ref readonly still map to ArgumentRefKind.In (spelled bare), and HasRefReadOnlyParameters is kept. The unused IrImporter copy (ReadParameterRefKinds, ClassifyByRefParameter, HasReadOnlyRefAttribute) is deleted. Tests cover compiled passing modes, Out-only rows patched onto in and ref readonly parameters, and the CoreLib FindConnectionPoint real asset. Part of #10164. Claude-Session: https://claude.ai/code/session_016JAYbAeLyaZdJSXjXNjGCP Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dotnet-inspectspells by-reference parameters in rendered declarations801e3de7e(round 2; base13890712e)Change
Conclusion: REVIEW.
Both Metadata declaration paths spelled any by-ref parameter that has the raw
Inflag (and notOut) as C#in. That flag is the interop marshalling direction, not the C# modifier.inis identified byIsReadOnlyAttribute.ref readonlyis identified byRequiresLocationAttribute.[In] refis an ordinary mutableref.So two kinds of parameter came out wrong:
[In] refparameters rendered asin. That is a readonly reference callers cannot assign through, so it changes the declaration's meaning.ref readonlyparameters rendered asin.Demo (production host: the CLI, real asset)
System.Runtime.InteropServices.ComTypes.IConnectionPointContainer.FindConnectionPointin System.Private.CoreLib. The runtime source declaresFindConnectionPoint([In] ref Guid riid, out IConnectionPoint ppCP).dotnet-inspect member System.Runtime.InteropServices.ComTypes.IConnectionPointContainer FindConnectionPoint --all --library <System.Private.CoreLib.dll> -v:dThe same spelling feeds the RTS shells.
InRefOnly([In] ref int x)and an abstract methodV([In] ref int x)were rebuilt within. Roslyn then emittedmodreq(InAttribute), which produced a falseOperandDifffor correct body text.ref.Design basis
Normative owner: Metadata parameter facts. The new
ParameterByRefKinds.Classifyinsrc/ILInspector.MetadataPrimitives/ParameterByRefKind.csis the one classification of a by-ref parameter's C# passing mode. Its remarks state the rule and its precedence.Claim: for a parameter whose signature type is by-reference, the declaration modifier follows the first rule that matches. This is Roslyn's metadata-reader precedence.
Outflag withoutInoutIsReadOnlyAttributepresentinRequiresLocationAttributepresentref readonly[In] refand[In, Out] ref)refparamsis unchanged.Baseline: C# compiler encoding. Roslyn emits
IsReadOnlyAttributeforin,RequiresLocationAttributeforref readonly, and sets theInflag on both. TheInandOutflags alone are ECMA-335 marshalling directions (II.23.1.13). Roslyn'sPEParameterSymbolchecksOut-only before either attribute.Analogous implementations: the Decompiler's call-site classifiers,
IrImporter.ClassifyByRefParameterandMethodDefinitionFacts.ClassifyByRefParameter. They apply the attribute rule but check the attributes beforeOut; the follow-up adoption takes the corrected order.What changed
ParameterByRefKinds(ILInspector.MetadataPrimitives, SRM-only) addsClassify(reader, parameter)andCSharpModifier().MetadataDeclarationQuery.GetParameterInfoandApiSurfaceExtractor.GetParameterInforeplace their duplicate raw-flag rules with the shared classifier. It runs for every parameter row; its result is used only when the signature type is by-ref, exactly as before.JsonUnionWireRules.CaseConstructors(JsExportSurface) excludesref readonlyalongsideref,inandout. Case constructors are by-value. Before this PR,ref readonlyconstructors arrived asinand were already excluded.Evidence
Gates (Release)
New tests:
MetadataDeclarationQueryTests.MethodDeclaration_SpellsByRefParameterPassingFromCompilerAttributesref,[In] ref,[In, Out] ref,out,inandref readonlyspell asref,ref,ref,out,inandref readonlyMetadataDeclarationQueryTests.MethodDeclaration_SpellsOutOnlyFlagAsOutBeforeCompilerAttributesParamrows carryingIsReadOnlyAttributeorRequiresLocationAttribute, patched toOut-only, spellout(Roslyn precedence)MetadataDeclarationQueryTests.MethodDeclaration_SpellsCoreLibInteropInRefParameterAsRefFindConnectionPointgivesrefandoutApiSignatureModelTests.MethodSignatureModel_SpellsByRefParameterPassingFromCompilerAttributesJsonUnionWireTests.CaseConstructors_ExcludeEveryByRefPassingModeref,in,outandref readonlyconstructors stay out of the union case setCSharpDeclarationWriterTests.AbbreviatedMemberDeclaration_PreservesParameterModifiersref readonlypasses through raw signaturesMutations, each caught:
RequiresLocationAttributeclassified asinOut-only (round 1 order)ref readonlydropped from the union filterSuites at the round 2 head:
ILInspector.Metadata.Tests4061/4061,ILInspector.JsExportSurface.Tests722/722,ILInspector.CSharp.Tests794/794,DotnetInspector.MetadataRendering.Tests174/174.DotnetInspector.Queries.Tests4101/4101 (round 1 head).DotnetInspect.Cli.Tests(29 methods, 35 cases) andDotnetInspector.Services.Tests(3 tests) fail at the round 1 head. I re-ran every one with main's two Metadata files swapped in. All the CLI failures and two of the Services failures fail the same way on main (local feed acquisition, routing, section pipelines, positional records, root-owned unreadable directories). The third Services failure,VersionPopulationOperationTimeoutRetainsEnumeratedVersions, is timing-dependent: it failed 2 of 3 runs on the branch and 1 of 3 on main.Build:
dotnet build dotnet-inspect.slnx -c Releasegives 0 warnings and 0 errors.Corpus exposure and baselines
An SRM probe counted the affected parameters in each corpus:
[In] ref→refref readonlyIMetadataImport.ResolveTypeRef)refonly) andcompile-cap 0compiles no declarations, so nothing can observe the spellingReal-world baseline (cap 4000 / fidelity 50,
rts-cutover), regenerated three times on the round 1 code:20670901b: identical to the committed file, rows and metrics.The round 2 change affects only
Out-only rows that also carry a compiler attribute. Roslyn never emits that shape, and no corpus assembly contains it. No baseline or pin changes.User-visible changes (release tracker #8151)
[In] refparameters now render asref, notin.ref readonlyparameters now render asref readonly, notin.Digest, a fingerprint of the canonical C# signature) change for those members. For example,FindConnectionPointgoes from6c6babd372to856a522237. ECMA member anchors come from metadata and are unchanged.Non-claims and follow-ups
IrImporter,MethodDefinitionFacts): they still keep their own copies of the rule, in attribute-first order. Adopting the shared classifier is the next step, kept separate so that one owner changes at a time. That adoption takes Roslyn's precedence and makesMethodDefinitionFactshandleref readonlytoo.[In]/[Out]pseudo-attributes: they are still not rendered, so the result isref System.Guid riid, not[In] ref System.Guid riid. The meaning is now correct and the call-site IL is identical; rendering the pseudo-attributes is a separate feature.[Out] ref: it is metadata-identical toout, and still spellsout, as before.Review rounds
983fed8fa): 2 accepted findings, both low severity, fixed in round 2.Out-only flag, so it disagreed with Roslyn and droppedouton hand-built metadata.JsonUnionWireRulesdidn't exclude"ref readonly"constructors.🤖 Generated with Claude Code
https://claude.ai/code/session_016JAYbAeLyaZdJSXjXNjGCP