roslyn icon indicating copy to clipboard operation
roslyn copied to clipboard

Decimal default parameter value is not propagated to the synthesized delegate

Open AlekseyTs opened this issue 1 year ago • 7 comments

class C
{
    void Test1()
    {
        var x = (decimal y = (decimal)1.1) => {};
    }

    void Test2(decimal y = (decimal)1.1)
    {
    }
}

Observed:

.class private auto ansi sealed '<>f__AnonymousDelegate0'
    extends [System.Runtime]System.MulticastDelegate
{
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
        01 00 00 00
    )
    // Methods
    .method public hidebysig specialname rtspecialname 
        instance void .ctor (
            object 'object',
            native int 'method'
        ) runtime managed 
    {
    } // end of method '<>f__AnonymousDelegate0'::.ctor

    .method public hidebysig newslot virtual 
        instance void Invoke (
            [opt] valuetype [System.Runtime]System.Decimal arg
        ) runtime managed 
    {
    } // end of method '<>f__AnonymousDelegate0'::Invoke

} // end of class <>f__AnonymousDelegate0

Note, there is no DecimalConstantAttribute on the optional parameter.

It is present for a regular declaration:

    .method private hidebysig 
        instance void Test2 (
            [opt] valuetype [System.Runtime]System.Decimal y
        ) cil managed 
    {
        .param [1]
            .custom instance void [System.Runtime]System.Runtime.CompilerServices.DecimalConstantAttribute::.ctor(uint8, uint8, uint32, uint32, uint32) = (
                01 00 01 00 00 00 00 00 00 00 00 00 0b 00 00 00
                00 00
            )
        // Method begins at RVA 0x2085
        // Code size 1 (0x1)
        .maxstack 8

        IL_0000: ret
    } // end of method C::Test2

AlekseyTs avatar Dec 02 '22 19:12 AlekseyTs

@jjonescz FYI

AlekseyTs avatar Dec 02 '22 19:12 AlekseyTs

Local functions behave the same way (the default value is not propagated; see SharpLab). Should they be fixed, too?

jjonescz avatar Dec 05 '22 10:12 jjonescz

Local functions behave the same way (the default value is not propagated; see ... . Should they be fixed, too?

What behavior you are referring to? Could you be more specific, because there is a lot of IL there and I am not sure what I should be looking at? Also, it is fine to refer to external resources, but consider putting information (repro, observed, expected) directly into the issue anyway. If external resource goes down for one reason or another, what is left for the reader?

I thought, that for synthesized delegates we specifically want default values to be propagated both from lambdas and method groups. It is very likely that method group scenarios have similar problem with decimal values (local functions or not).

Whether an implementation method generated for a local function should have default values propagated from declaration is an interesting question. One might say that those synthesized methods cannot be consumed by users directly. So, what scenario we are going to enable if we propagate defaults? Perhaps debugging scenarios? I do not know at the moment. But I think this is not related to synthesized delegates.

AlekseyTs avatar Dec 05 '22 14:12 AlekseyTs

What behavior you are referring to?

I was referring to the fact that the implementation method of a local function doesn't have DecimalConstantAttribute, although it has other default values propagated.

void f1(decimal x = 1.1m) { }
void f2(int x = 1) { }
    .method assembly hidebysig static 
        void '<<Main>$>g__f1|0_0' (
            [opt] valuetype [System.Runtime]System.Decimal x
        ) cil managed 
    {
        .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
            01 00 00 00
        )
        // Method begins at RVA 0x2076
        // Code size 2 (0x2)
        .maxstack 8

        IL_0000: nop
        IL_0001: ret
    } // end of method Program::'<<Main>$>g__f1|0_0'

    .method assembly hidebysig static 
        void '<<Main>$>g__f2|0_1' (
            [opt] int32 x
        ) cil managed 
    {
        .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
            01 00 00 00
        )
        .param [1] = int32(1)
        // Method begins at RVA 0x2076
        // Code size 2 (0x2)
        .maxstack 8

        IL_0000: nop
        IL_0001: ret
    } // end of method Program::'<<Main>$>g__f2|0_1'

Whether an implementation method generated for a local function should have default values propagated from declaration is an interesting question.

The propagation already happens for non-decimal constants as demonstrated above. Shouldn't it be more consistent?

jjonescz avatar Dec 05 '22 15:12 jjonescz

The propagation already happens for non-decimal constants as demonstrated above. Shouldn't it be more consistent?

In general, consistency is good. However, it would be good to understand was there a specific intent behind propagating defaults in this case, or was it just an unintended consequence of some unrelated change.

AlekseyTs avatar Dec 05 '22 15:12 AlekseyTs

@jjonescz

The propagation already happens for non-decimal constants as demonstrated above. Shouldn't it be more consistent?

In general, consistency is good. However, it would be good to understand was there a specific intent behind propagating defaults in this case, or was it just an unintended consequence of some unrelated change.

It looks like propagation of default values for local functions is intentional, see #53402. Consider fixing this issue as well. Otherwise, let's open a separate issue to follow up.

AlekseyTs avatar Dec 06 '22 16:12 AlekseyTs

Consider fixing this issue as well.

I have included the fix in the PR.

jjonescz avatar Dec 13 '22 11:12 jjonescz