Fix #3824: fold a hoisted argument null-guard into the chained ctor call

[sailro]

Fixes #3824.

When a constructor initializer argument contains argument-validation that throws
(e.g. arg ?? throw new ArgumentNullException(...)) and the argument is used by
more than one parameter, the compiler hoists the null-check in front of the
chained constructor call. MoveConstructorInitializer only inspected the first
body statement, so the hoisted guard defeated it and the chained call was left as
an illegal in-body this..ctor(...) / base..ctor(...) statement (a parse error).

Fix

When locating the chained call, skip leading argument null-guards
(if (param == null) throw ...;) and fold each one back into the first argument
that uses the parameter, as param ?? throw .... If a guard cannot be folded (its
parameter is not used by any argument), the constructor is left unchanged.

Before:

public PatternToken(string value)
{
    if (value == null)
    {
        throw new ArgumentNullException("value");
    }
    this..ctor(AsciiCategory.None, value.Length, value.Length, "value"); // parse error
    Value = value;
}

After:

public PatternToken(string value)
    : this(AsciiCategory.None, (value ?? throw new ArgumentNullException("value")).Length, value.Length, "value")
{
    Value = value;
}

Test

Pretty/ConstructorInitializers gains a #if CS70 case where the null-check is
hoisted because the parameter is used by more than one argument; it round-trips to
the folded initializer form.

Real-world occurrence: IbanNet PatternToken(string) (net462).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[siegfriedpammer]

Thanks for bringing attention to this issue and for taking the time to prepare a fix — much appreciated!

After digging into it, we concluded that this needs a more involved implementation at the ILAst level, as part of the null-coalescing transform. The main advantage of doing it there is the existing transform infrastructure and the precise semantic model: variables know all of their uses and the control flow is explicit, so the transform can reliably verify it only fires on the exact compiler-generated pattern. That information is reachable from the C# AST too, but only through an extra level of indirection, which makes such checks considerably more awkward there.

I'm therefore closing this PR as superseded by #3852, which implements the fix that way. Thanks again for the contribution!