Accept member IDs without a signature in --navigateto - #4063
Merged
Merged
Conversation
christophwille
left a comment
Member
There was a problem hiding this comment.
Review summary (high-effort, recall-biased)
Both changes are sound: the third pass only runs after both exact-match passes fail and only when no '(' / '~' is present, so every existing full-ID resolution is untouched, and the UI fallback correctly keeps 'none' as handled. Callers checked: both FindEntity overloads share FindMemberInType, so the ilspycmd -m claim holds; DecompilerTextView.ResolveDocReference (cref clicks) picks the fallback up too, which is a benefit.
Findings (one minor resolver bug, two nits), inline:
- HasSignature ignores the generic-arity marker, so a short form that does state an arity drifts to a same-named method of a different arity. Repro on the existing Acme fixture below.
- Test doc comment uses 'licence' (en-GB) - CLAUDE.md asks for en-US.
- HasSignature re-parses what GetApproximateMemberName already parsed; the nameFilter != null guard on the new line is redundant.
No findings on the AssemblyTreeModel change or the UI tests.
siegfriedpammer
force-pushed
the
fix/navigateto-short-form
branch
2 times, most recently
from
August 28, 2026 16:41
c4ead78 to
100a131
Compare
Typing "M:System.Linq.Enumerable.Where" at a command line is a reasonable thing to do, and it found nothing: resolution compares the whole id string, so a form without the parameter list only ever matched a member that genuinely takes none. Spelling the signature out is no answer, because it means knowing the overload count before asking. The same goes for a generic arity - and the exact spelling, Dictionary`2, does not even survive an unquoted bash prompt, where a backtick starts command substitution. None of that makes the short form legal. Measured against Roslyn: its own DocumentationCommentId resolver accepts no abbreviation at all, and the compiler never emits one - a cref is a different grammar, which the compiler binds and rewrites into a full id, warning CS0419 and picking one member when the cref is ambiguous. A prefixed cref is copied through unvalidated, so an id in a documentation file can be anything a human typed. So the id grammar stays exact and IdStringProvider stays with it, which is what lets cref-following trust its answer. The tolerance belongs to the callers that serve people typing, and lives in DocumentationIdSearch as a ladder that loosens one thing at a time: the exact id, then the id without its parameter list, then without generic arities. Stating a detail wrongly still finds nothing; only leaving one out asks for any. A rung may match several members and all of them are returned, because which to present is the caller's decision and hiding the rest would hide that the id was ambiguous. ilspycmd shows every member of the group, headed by a comment naming the ambiguity, and accepts the shapes people actually type: no prefix, a shortened namespace, and arity written the cref or C# way. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
An ID that resolves to no member left the tree on an empty selection with no indication of what happened, because supplying --navigateto also suppresses the single-assembly selection that opening a file otherwise makes. Only a target that actually resolved should claim the selection; "none" still counts as handled, since the VS add-in uses it to deliberately leave the tree empty. The target arrives from a command line, so it goes through the omission-tolerant search rather than exact resolution, and that can name several members. All of them are selected. Landing on one would hide that there was a choice, and falling back to the declaring type would bury the group in a large type's decompilation - Enumerable.Where would decompile some two hundred members to show four. The tree already multi-selects, so the overloads appear together at the level the ID was pointing at. Assisted-by: Claude:claude-opus-5[1m]:Claude Code
siegfriedpammer
force-pushed
the
fix/navigateto-short-form
branch
from
August 29, 2026 09:59
14f443e to
ead3ee7
Compare
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.
Found while trying to open a member from the command line:
--navigateto "M:Class148.DefaultCheckCopyed"found nothing, because the id left the parameter list off.
Why a ladder, and why not in the resolver
The obvious fix is to make
IdStringProvidertolerate a missing parameter list. I did that first, andit was wrong. Measuring against Roslyn (probe sources in the linked analysis):
DocumentationCommentIdresolver accepts no abbreviation at all. Parameter listomitted, type arity omitted, method arity omitted, wrong arity, C# syntax, missing namespace,
indexer without parameters - every one resolves to nothing. The id grammar is exact.
crefis a different grammar: the compiler binds itand rewrites it into a full id, warning
CS0419and picking one member when the cref names several.cref="M:Widget.M1``9"lands in the XML verbatim,so an id in a documentation file is only as trustworthy as the human who typed it.
So accepting abbreviations is a convenience for people typing, not a correctness fix, and it does
not belong in a resolver whose other consumer is cref-following - where strictness is exactly what
keeps a doc link honest.
The evidence that this matters is not hypothetical. An exhaustive round-trip over every id Roslyn
generates for the test fixture (rather than a hand-picked list) caught the in-resolver version
mis-resolving an id Roslyn itself writes into documentation XML:
A record struct's implicit parameterless constructor never reaches metadata, so the fallback drifted to
the primary constructor - a wrong answer where the previous behaviour was no answer. Keeping the
resolver strict dissolves that, and dissolves @christophwille's stated-but-wrong-arity finding with it,
rather than patching either.
IdStringProvideris therefore unchanged by this PR. The tolerance lives in the newDocumentationIdSearch, a ladder that loosens one thing at a time and stops at the first rung thatmatches anything:
Two rules hold across the ladder. Leaving a detail out asks for any; stating one wrongly asks for
something that does not exist - so
Dictionary9.Add` still finds nothing. And a rung may matchseveral members, and all of them are returned, because which to present is the caller's decision and
returning one would hide that the id was ambiguous.
Why arity too
Because it is the same problem one level up - you cannot write
Dictionary`2without alreadyknowing the answer - and because the exact spelling is hostile to the place it gets typed:
A backtick inside double quotes is command substitution, so the correct id kills the shell before
ilspycmd runs. The cref spelling
{TKey,TValue}is accepted too, and it survives quoting.What the front ends do
Everything below now works, against
System.Collections.dll:Without a prefix,
A.Bgenuinely reads as either a nested type or a member; both are searched and bothreported rather than one being guessed at.
ilspycmd -mdecompiles every member of the group, headed by a comment naming the ambiguity.Making the user re-run with a full signature would defeat the point of accepting the short form.
--ilast -mlikewise dumps each body instead of silently taking the first.to the declaring type would bury the group in a large type's decompilation -
Enumerable.Wherewoulddecompile some two hundred members to show four. The tree already multi-selects.
Note this deliberately diverges from Roslyn, which warns and picks one. That is right for an authoring
tool, which must end up with a single id in the XML; a navigation tool can show all of them.
An unresolvable target left the tree empty
Supplying
--navigatetoalso suppresses the single-assembly selection that opening a file otherwisemakes, so an ID that resolved to nothing left the tree on an empty selection with no indication of what
had happened.
NavigateOnLaunchAsyncnow reports whether it navigated, and the caller fills in theselection when it did not. The
nonesentinel still counts as handled, since the VS add-in uses it todeliberately leave the tree empty.
Tests
Written red-first.
IdStringProviderTestskeeps the resolver honest -FindEntity_ResolvesOnlyWhatTheIdExactlyNamespins eleven negatives, including every abbreviation - andFindEntity_RoundTripsEveryIdRoslynGeneratesdrives a round-trip over the whole Roslyn id map, which iswhat caught the record-struct case. The ladder has its own cases for each rung, the prefix and namespace
omissions, bracketed arity, and nesting three levels deep with arities dropped at any level.
Two fixture facts worth recording, because both contradicted my expectations:
M:Acme.Widget.#ctoris that constructor's own exact id, so rung 1 wins.op_CheckedExplicit), so it is not part of theunchecked operator's group.
ICSharpCode.Decompiler.Tests3539 / 0 failed / 45 skipped;ILSpy.Tests1243 / 0 / 3;ICSharpCode.ILSpyCmd.Tests33 / 0 / 0.Review
@christophwille - your arity finding is a large part of why this got restructured; thank you. All three
points are addressed, though two by deletion rather than by fix: the code carrying the redundant
nameFilterguard and the duplicatedHasSignatureparsing no longer exists, and thelicencespelling went with the test that contained it. One note in fairness: the
nameFilter != nullatIdStringProvider.cs:954is master's own pre-existing line and part of your nit arguably applies to ittoo, but this PR no longer touches that method, so I have left it alone rather than adding unrelated
churn.
Out of scope, but worth recording:
ilspycmd --typehas long acceptedDictionaryvia its ownarity-stripping ladder and reports candidates when several match, while
-mwas strict. Thatinconsistency is what a user actually trips over, and this PR removes it only on the
-mside.Written by an AI agent (Claude) on Siegfried's behalf.