I'm not sure if there are other properties I should override, and what they'll affect.

I looked over the ParameterSymbol members and all that comes to mind is IsIDispatchConstant and IsIUnknownConstant.

I think that synthesized parameter symbols with non-null _baseParameterForAttributes are only produced for lowering/emit, so we don't expect to actually check the IsIDispatchConstant or IsIUnknownConstant members from the _baseParameterForAttributes.

I am not certain whether it makes more sense to:

1. Do nothing to override or verify IsIDispatchConstant or IsIUnknownConstant on SynthesizedComplexParameterSymbol (i.e. don't change the implementation in this PR)
2. Change IsIDispatchConstant and IsIUnknownConstant on SynthesizedParameterSymbolBase to throw ExceptionUtilities.Unreachable, since we don't expect to check those members on such parameters, because we don't expect to bind a default argument based on such a parameter, for example. (I'm not certain about this--if it results in new CI failures then we'll have learned something about the usage 😄)
3. Override IsIDispatchConstant and IsIUnknownConstant on SynthesizedComplexParameterSymbol to delegate to the _baseParameterForAttributes.

I'm leaning slightly toward (2).

@RikkiGibson Thanks. I took the second approach as you suggested.

Is there anything needed for nullable analysis here?

e.g, override FlowAnalysisAnnotation and things related to nullable analysis?

I had a few more overrides and assertions in mind but I felt like enumerating them was equal work to adding the implementations, so I went ahead and pushed a commit to your PR.

We should verify if this causes a behavior change in any other scenarios, and if so, do we want that behavior change. For example, could this cause parameter default value info to be emitted for some other kind of declaration that we don't want it emitted for.

@RikkiGibson Based on @jaredpar comment on the issue, a breaking change is okay here.

One possible code that's going to have a behavioral change:

using System;
public static class Program
{
    public static void Main()
    {
        void TestLocalAction(int a = 2)
        {
        }

        MapAction((Action<int>)TestMethodAction);
        MapAction((Action<int>)TestLocalAction);
    }

    public static void TestMethodAction(int a = 2)
    {
    }

    public static void MapAction(Delegate action)
    {
        Console.WriteLine(action.Method.GetParameters()[0].HasDefaultValue);
    }
}

I couldn't come up with a more "realistic" scenario.

I agree that it is good to "break" this scenario, I just want to make sure that we've enumerated the other places in the compiler where a SynthesizedParameterSymbol can be created with a non-null _baseParameterForAttributes, in case this might impact other kinds of symbols.

I'll try and sit down later this afternoon and make sure of it myself. I think local functions might be the only scenario where we set a non-null value for this field, although lambdas might do it too now (cc @cston), and we probably would want the same kind of behavior for lambdas as we want for local functions here.

edit: actually, I think optional parameters are disallowed in lambdas both before and after the lambda improvements changes we're doing in C# 10, so it might be moot to try and test that scenario.

I think the only place for _baseParameterForAttributes to be non-null is SynthesizedClosureMethod (either a lambda or a local function).

I think the only place for _baseParameterForAttributes to be non-null is SynthesizedClosureMethod (either a lambda or a local function).

It feels like we might want to add negative tests for other scenarios consuming SynthesizedParameterSymbol.Create.

@AlekseyTs Any concrete/specific test cases you have in mind? For example, top-level entry point and record copy ctor are using SynthesizedParameterSymbol.Create. How a test for them would look like? they're never synthesized with optional parameters.