Skip to content

Support caller info and nullability attributes in lambdas#64823

Closed
jjonescz wants to merge 1 commit intodotnet:features/lambda-default-parametersfrom
jjonescz:ldp-caller-info-attributes
Closed

Support caller info and nullability attributes in lambdas#64823
jjonescz wants to merge 1 commit intodotnet:features/lambda-default-parametersfrom
jjonescz:ldp-caller-info-attributes

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented Oct 19, 2022

Adds support for [CallerMemberName]+friends and [AllowNull]+friends in lambdas by forwarding them from a lambda declaration into its synthesized delegate type. In fact, with the proposed implementation, all well-known attributes forwarded by SynthesizedComplexParameterSymbol are supported.

@ghost ghost added the Area-Compilers label Oct 19, 2022
@jjonescz jjonescz changed the title Copy caller info and nullability attributes from lambdas Support caller info and nullability attributes in lambdas Oct 20, 2022
@jjonescz jjonescz marked this pull request as ready for review October 20, 2022 08:03
@jjonescz jjonescz requested a review from a team as a code owner October 20, 2022 08:03
@333fred
Copy link
Copy Markdown
Member

333fred commented Oct 21, 2022

@cston wasn't it intentional that these weren't forwarded?

ImmutableArray<TypeWithAnnotations> parameterTypes,
ImmutableArray<ConstantValue?> parameterDefaultValues)
ImmutableArray<ConstantValue?> parameterDefaultValues,
ImmutableArray<SourceComplexParameterSymbolBase> parameterSymbolsForAttributes)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameterSymbolsForAttributes

Could we pass method symbol instead of the last 7 parameters and have this function extract necessary information directly from it?


public readonly ConstantValue? DefaultValue;

public readonly SourceComplexParameterSymbolBase? ParameterSymbolForAttributes;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParameterSymbolForAttributes

Could DefaultValue be extracted from this symbol as well?

}
""";
CompileAndVerify(source, expectedOutput: "file::member:0");
CompileAndVerify(source, expectedOutput: "::Main:9");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CompileAndVerify(source, expectedOutput: "::Main:9");

Are we testing that attributes are actually present/missing in metadata (on the lambda method and the delegate invoke)?

@AlekseyTs
Copy link
Copy Markdown
Contributor

        SourceComplexParameterSymbolBase? baseParameterForAttributes)

Can we get defaultValue from it as well?


Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs:295 in 8b663c3. [](commit_id = 8b663c3, deletion_comment = False)

@cston
Copy link
Copy Markdown
Contributor

cston commented Oct 24, 2022

        // Use System.Action<...> or System.Func<...> if possible.

It looks like we're ignoring attributes when deciding whether to synthesize a delegate type or use Action<...> or Func<...>. Is that intentional?


Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:9038 in 8b663c3. [](commit_id = 8b663c3, deletion_comment = False)

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Oct 25, 2022

forwarding them from a lambda declaration into its synthesized delegate type

I think the current behavior is intentional. We don't generally forward attributes (although we did an exception for [UnscopedRef] recently).
Let's spec this out and discuss

@jjonescz jjonescz closed this Oct 25, 2022
@jjonescz jjonescz deleted the ldp-caller-info-attributes branch October 25, 2022 18:17
@jjonescz jjonescz restored the ldp-caller-info-attributes branch October 25, 2022 18:17
@jjonescz jjonescz deleted the ldp-caller-info-attributes branch October 26, 2022 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants