Extensions: propagate return-targeted and local function parameter attributes #80070
Conversation
jcouv
added
Area-Compilers
Feature - Extension Everything
jcouv
force-pushed
the
extensions-return
branch
from
b3ee966 to
dde1b78
Compare
| /// for symbols from source or derived from source symbols. | ||
| /// Returns <see cref="ConstantValue.NotAvailable"/> when not applicable. | ||
| /// </summary> | ||
| internal abstract ConstantValue DefaultValueFromAttributes { get; } |
There was a problem hiding this comment.
I think this should return ConstantValue? (i.e., nullable) since ConstantValue.NotAvailable is null. Btw, I'm not sure why ConstantValue.NotAvailable is not marked as nullable (and it's surprising that there is no nullable warning at the declaration site, but that seems to be a known issue - #50580). #Resolved
| return _originalMethod.GetAttributes(); | ||
| } | ||
|
|
||
| public override ImmutableArray<CSharpAttributeData> GetReturnTypeAttributes() |
There was a problem hiding this comment.
| public override ImmutableArray<CSharpAttributeData> GetReturnTypeAttributes() | |
| public sealed override ImmutableArray<CSharpAttributeData> GetReturnTypeAttributes() | |
| ``` #Resolved |
| get { return null; } | ||
| } | ||
|
|
||
| internal override ConstantValue DefaultValueFromAttributes |
There was a problem hiding this comment.
| internal override ConstantValue DefaultValueFromAttributes | |
| internal sealed override ConstantValue DefaultValueFromAttributes | |
| ``` #Resolved |
| } | ||
| } | ||
|
|
||
| internal override ConstantValue DefaultValueFromAttributes => ConstantValue.NotAvailable; |
There was a problem hiding this comment.
| internal override ConstantValue DefaultValueFromAttributes => ConstantValue.NotAvailable; | |
| internal sealed override ConstantValue DefaultValueFromAttributes => ConstantValue.NotAvailable; | |
| ``` #Resolved |
| get { return _underlyingParameter.ExplicitDefaultConstantValue; } | ||
| } | ||
|
|
||
| internal override ConstantValue DefaultValueFromAttributes |
There was a problem hiding this comment.
| internal override ConstantValue DefaultValueFromAttributes | |
| internal sealed override ConstantValue DefaultValueFromAttributes | |
| ``` #Resolved |
| var synthesizedParam = SynthesizedParameterSymbol.Create(localFunction, param.TypeWithAnnotations, ordinal: 0, RefKind.Out, param.Name, baseParameterForAttributes: (SourceComplexParameterSymbolBase)param); | ||
| Assert.False(synthesizedParam.IsMetadataIn); | ||
| Assert.True(synthesizedParam.IsMetadataOut); | ||
| } |
There was a problem hiding this comment.
Seems like this deleted block of code was important to the test - at least from the name BaseParameterWithDifferentRefKind and the linked issue. #Resolved
There was a problem hiding this comment.
Removed the test as we never need to synthesize a parameter where the refkind and the base parameter's refkind disagree.
The linked issue describes the scenario covered by InOutAttributes.
Tagging @RikkiGibson to confirm
|
@RikkiGibson You'd be a good reviewer for this one, as you worked in this area before (propagating attributes on local functions, in #39830). Please take a look |
|
|
||
| 42.M(); | ||
| 42.M2(); | ||
| E.M(42); |
There was a problem hiding this comment.
Nit: I know you didn't make this test source in this test, but consider refactoring to just set a preprocessor symbol through options and avoid repeating the same source twice, that we then have to keep in sync. #Resolved
| } | ||
|
|
||
| [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/80017")] | ||
| public void PropagateAttributes_21(bool withPreserve) |
There was a problem hiding this comment.
Why is this test in this file? It's unrelated to extensions? #Resolved
There was a problem hiding this comment.
It's meant to establish baseline for the test above. I'll remove move it
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
Done review pass. I'd like that local function test moved to CodeGenLocalFunctionsTests please.
4 participants
The change in
SynthesizedParameterSymbolBaseaffects return-targeted attributes.The change in
SynthesizedMethodBaseSymbolis for local function parameter attributes. It only supported propagating attributes from aSourceComplexParameterSymbolBase, but for a local function inside an extension we're dealing with aRewrittenMethodParameterSymbolwhich wraps one.This required generalizing
SynthesizedComplexParameterSymbolto accept aParameterSymbolinstead of just aSourceComplexParameterSymbolBasewhich caused the rest of the changes (movingDefaultValueFromAttributesup the symbol hierarchy, tweaks toSynthesizedComplexParameterSymbol.IsMetadataIn/Out).Test
PropagateAttributes_20illustrates both of the changes. Before, we were not properly emitting attributesAandCon the closure method.The change in
CodeGenLocalFunctionTests.BaseParameterWithDifferentRefKindresults from tightening expectations inSynthesizedComplexParameterSymbol(added assertion in constructor).Fixes #80017 (return-targeted attributes not properly emitted)
Relates to test plan #76130
Possible interaction with #79880
Relates to #73920