Fix #3282: leave out trailing optional arguments in indexer accesses

[siegfriedpammer]

Written by Claude (agent), posted on behalf of @siegfriedpammer.

Fixes #3282.
Fixes #3060 - the same assert reached from a different assembly. Verified against the reporter's err131.dll: ilspycmd err131.dll -m 0x060001AF hits CheckNoNamedOrOptionalArguments on master and decompiles to list[num] on this branch, where master's Release build wrote list[num, false].

HandleAccessorCall had no way to express an omitted argument, so CallBuilder asserted that no optional argument had been detected before it got there. Any assembly that indexes through an indexer with an optional parameter hits that assert in a Debug build; Release builds silently wrote the defaults back out.

Let an indexer access leave out trailing optional arguments

Accessor calls now go through the same ArgumentList helpers as an ordinary call. Two things had to reach them: the assigned value of a setter is the last argument of the call but not an argument of the access - the standard adds it only for the invocation of the accessor (§12.6.2.1) - so it neither ends the run of optional arguments nor is written out with them; and the argument names have to stop wherever the argument list does.

Whether the shortened access still binds to the same member is left to IsUnambiguousAccess. If it does not, the omitted arguments are written out again before any cast is tried, since restoring them cannot change what the access means. A type declaring both this[int] and this[int, int = 10] therefore keeps both arguments; the fixture pins that.

Write named arguments for indexer accesses

C# allows named arguments in an element access, but NamedArgumentTransform refused to introduce one for any accessor, so an access whose arguments the compiler reordered came out as a temporary variable assigned before the access. Only indexers gain this: a property access has no argument list, an operator cannot take names either, and a setter's last argument stays unnamed on the right-hand side.

Tests

Indexer cases in OptionalArguments (get, set, compound assignment, increment, struct receiver, object initializer, and the overload that must keep its argument), in OptionalArgumentsDisabled (with the setting off the arguments stay explicit), and in NamedArguments.

Not covered

Still written out explicitly, each for its own reason: params indexers, [Optional] without a constant and [DateTimeConstant]-style defaults (nothing in the signature to compare against), default(T) at a value-type instantiation, and omitting a middle optional argument - the last of which plain calls do not do either (M(0, z: 9) decompiles to M(0, 1, 9)).

Interaction with #3972

Checked: all three commits of #3972 cherry-pick onto this branch without conflict, the combined tree builds, and its full decompiler suite is green (3467 tests, 0 failures), including c[1] += 5 on an indexer, which goes through both changes.

[christophwille]

Review summary

The optional-argument part works for the fixtures added, but opening indexer accessors to the named-argument machinery (NamedArgumentTransform + HandleAccessorCall now consuming GetArgumentNames()/GetArgumentExpressions()) re-exposes several assumptions elsewhere that were only safe because accessor calls never carried names. All findings below were reproduced against a build of this branch with small probe assemblies; master decompiles the same probes correctly.

Default settings, plain Roslyn output (crash or uncompilable code):

1. NamedArgumentTransform: an indexer setter inside a BlockKind.CallInlineAssign block gets replaced by a CallWithNamedArgs block -> Block.CheckInvariant assert (Debug) / MatchInlineAssignBlock() returned false (Release).
2. HandleAccessorCall: when every indexer argument is an omitted trailing optional, zero arguments remain and the code falls into the property branches, emitting this.Item = 5 / initializedObject.Item.
3. HandleAccessorCall: CastArguments pairs argument-order arguments with declaration-order method.Parameters; with reordered named indexer arguments each argument is cast to the wrong parameter's type.
4. HandleAccessorCall: AddNamesToPrimitiveValues now applies to every indexer access (dictionary[true] -> dictionary[key: true]) and, unlike GetRequiredTransformationsForCall, the retry loop never tries turning it off before casting -> ((Base)d)[flag: true] on overridden indexers. Untested default-output change.

Need AggressiveInlining (or an aggressive context: catch-when, ctor initializer, expression tree):
5. CanExtendNamedArgument still names an indexer setter's value argument; CallWithNamedArgs then reorders it away from the last position, which BuildArgumentList/HandleAccessorCall rely on -> this[value: Get(0), y: Get(1)] = Get(2);.
6. An indexer getter that is the Target of a CompoundAssignmentInstruction can now become a CallWithNamedArgs block -> CompoundAssignmentInstruction.CheckValidTarget assert.

Low severity:
7. IsSetterAccessorWrittenAsAssignment and Build()'s routing condition can disagree after params expansion (contrived, not producible from C#).

Common root cause for 1, 5, 6: accessor calls can now be wrapped in CallWithNamedArgs blocks, but several IL consumers (CallInlineAssign invariant, CompoundAssignmentInstruction.CheckValidTarget, CanExtendNamedArgument) and the position-based "last argument is the value" logic in CallBuilder still assume they cannot. Guarding CanIntroduceNamedArgument against a call.Parent that is a CallInlineAssign block / compound-assignment target, plus identifying the setter value by parameter index rather than position, closes all three.

Minor cleanups (no separate comments): GetArgumentExpressions still does argumentNames.Take(argumentCount) after GetArgumentNames already truncates (dead); lastNameableArgument is an exclusive end (misnamed); the setter predicate is duplicated in three places (Build, IsSetterAccessorWrittenAsAssignment, NamedArgumentTransform).

Details and repro snippets are in the inline comments.

[christophwille]

Re-review (head d5c17cd)

All seven findings of the previous review (id 5002832803) are fixed at this head; each was re-run against a Debug build of the branch with the original probe snippets:

1. Indexer setter inside a CallInlineAssign block -> CanIntroduceNamedArgument now stops on that parent; Check(this[y: Get(1), x: Get(2)] = Get(3)) decompiles to int y = Get(1); int x = (this[Get(2), y] = Get(3)); Check(x);.
2. All-optional indexer -> FirstOptionalArgumentIndex is bumped to 1 in HandleAccessorCall; this[10] = 5; this[10] += 5; Console.WriteLine(this[10]); new Probe4 { [10] = 3 } all come back as written.
3. CastArguments now takes ExpectedParameters (argument order); the this[o: ..., i: ...] overload pair casts the right argument.
4. AddNamesToPrimitiveValues is off for accessor calls; dictionary[true], this[true] and d[true] on an overriding indexer are unchanged.
5. CanExtendNamedArgument stops at NameableArgumentCount; int s = Get(0); this[y: Get(1), x: Get(2)] = s; keeps the value last (also with AggressiveInlining=true).
6. Compound-assignment target guard in CanIntroduceNamedArgument; this[y: Get(1), x: Get(2)] += 5 and ++ are fine in both inlining modes.
7. params setter: IsWrittenAsMemberAccess + !isSetter guard and the ParamsPropertySetter fixture.

Also probed without regressions: OptionalArguments=false, NamedArguments=false, AlwaysQualifyMemberReferences=true, AggressiveInlining=true, null-conditional, deconstruction targets, ref-returning indexers (assignment, ++, ref local), struct receivers (field, ref parameter, array element), interface indexers with optional parameters, base[x], generic indexers, string/enum/nullable defaults, lambdas, object/collection initializers, in parameters, and the constructor path that now shares the ladder.

One new regression (inline comment): OmittedArgumentsAreDefaultsOf refuses any argument list that has an ArgumentToParameterMap, so plain method calls that combine named arguments with omitted trailing optional arguments now write the defaults back out. M(b: Get(2), a: Get(1)) for void M(int a, int b, int c = 3) decompiles to M(b: Get(2), a: Get(1), c: 3) where master gives M(b: Get(2), a: Get(1)). Compilable, but a step back for ordinary calls, and not covered by the NamedArguments/OptionalArguments fixtures (which is why the suite stays green).

Low-severity observation (inline comment): a named indexer access on a receiver whose static type overrides the indexer with different parameter names now comes out as a target cast, ((Base2)d)[y: Get(1), x: Get(2)], where master emitted int y = Get(1); d[Get(2), y]. Correct, and consistent with what the method path already does, so no action required unless you want to keep the temporary in that case.

All findings were reproduced against a Debug build of this branch (ilspycmd, default settings unless stated) and cross-checked against master.

[christophwille]

Regression (plain method calls, default settings): named arguments and omitted trailing optional arguments no longer combine.

This early return false fires for every argument list that has an ArgumentToParameterMap, and the caller then sets FirstOptionalArgumentIndex = -1, so the defaults are written back out whenever a call also has a reordered named argument. master dropped them (its IsUnambiguousCall truncated the names to firstOptionalArgumentIndex and let overload resolution decide).

public static int Get(int x) => x;
public void M(int a, int b, int c = 3) { }
public void N(int x, int y = 10, int z = 20) { }
public void Call()
{
	M(b: Get(2), a: Get(1));
	N(y: Get(1), x: Get(2));
	N(z: Get(1), x: Get(2));
}

PR build:

M(b: Get(2), a: Get(1), c: 3);
N(y: Get(1), x: Get(2), z: 20);
N(z: Get(1), x: Get(2), y: 10);

master:

M(b: Get(2), a: Get(1));
N(y: Get(1), x: Get(2));
N(z: Get(1), x: Get(2));

The comment above is not quite right: with names in play, an omitted argument does not have to be a trailing parameter at all (N(z: Get(1), x: Get(2)) legitimately omits the middle y), it only has to be a trailing argument whose parameter is optional in the member found. That is exactly what this loop can check by mapping the argument index through the map, e.g.

var map = argumentList.ArgumentToParameterMap;
int offset = map == null ? 0 : map.Count - argumentList.Length; // skips the 'this' slot
for (int i = omittedFrom; i < argumentCount; i++)
{
	int p = map == null ? i : map[offset + i];
	if (p < 0 || p >= parameters.Count || !IsOptionalArgument(parameters[p], argumentList.Arguments[i]))
		return false;
}

and then drop the ArgumentToParameterMap != null bail-out. A fixture line for M(b: Get(2), a: Get(1)) in NamedArguments.cs or OptionalArguments.cs would pin it; none of the existing ones exercise the combination.

[christophwille]

Low severity / output quality: the names come from the indexer the IL names, which for a virtual call is the base declaration. When the receiver's static type overrides it with different parameter names, IsUnambiguousAccess fails on the names and the ladder ends in a target cast.

public class Base2 { public virtual int this[int x, int y] { get => x; set { } } }
public class Derived2 : Base2 { public override int this[int a, int b] { get => a; set { } } }
...
public void Call(Derived2 d) { Console.WriteLine(d[b: Get(1), a: Get(2)]); }

PR build: Console.WriteLine(((Base2)d)[y: Get(1), x: Get(2)]); -- master: int y = Get(1); Console.WriteLine(d[Get(2), y]);

Both compile and mean the same thing, and the method path on master already produces ((Base2)d).M(y: Get(1), x: Get(2)), so this is consistent rather than wrong. Mentioning it only because the previous round flagged the ((Base)d)[flag: true] cast for primitive names; here the names are genuinely needed, so a cast is the honest fallback. No change required.

[dgrunwald]

"Let an indexer access leave out arguments and name them" -- do we really need this feature?
It took a lot of complexity to get there (too much to review in one evening).
Please don't let AI transform the CallBuilder into something no one understands anymore.

[siegfriedpammer]

The main complexity comes from the fact that I asked the LLM to refactor the disambiguation algorithm into something we can reuse. At least that was my intent for a later refactoring, currently it seems there is a lot of duplication going on.

Honestly, I think CallBuilder has way too many responsibilities already. Originally we extracted the "call" specific things out of ExpressionBuilder and it seems that now CallBuilder suffers from the same disease:

• Calls to ordinary methods
• Calls to ctors
• Dictionary initializers???
• string.Format parsing???
• Calls to operators
• Calls to indexers
• Range support???
• Delegate construction???

all the items marked ??? should be discussed and moved somewhere else, IMHO. At least we should think about it.

And there is of course the big machinery that repeatedly asks OR/MemberLookup: Would this call still resolve to the method called in IL? ... And, what about now? ... Now? ... Still not? That is something we should probably make reusable too... if it were reusable, adding optional args support for indexers would be almost free and not this monster of a change.

That aside: A cheap fix would be to just remove the assertion?