Emit caller argument expression parameter name in correct casing#54642
Conversation
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 3). @dotnet/roslyn-compiler for a second review.
|
@jcouv Can you review please? Thanks! |
| Return MyBase.EarlyDecodeWellKnownAttribute(arguments) | ||
| End Function | ||
|
|
||
| Friend Overrides Iterator Function GetCustomAttributesToEmit(compilationState As ModuleCompilationState) As IEnumerable(Of VisualBasicAttributeData) |
There was a problem hiding this comment.
Has the scenario where CallerArgumentExpressionAttribute is applied multiple times been considered (this requires to define a non-standard attribute)? The below code seems to assume only one usage.
As an example, for AsyncMethodBuilderAttribute only the first application is consisdered (the others are ignored).
This method and CallerArgumentExpressionParameterIndex probably need to agree on behavior (and not crash ;-).
Such situation could be combined with an application of the attribute using a different constructor (breaks assertion below). #Closed
There was a problem hiding this comment.
Has the scenario where
CallerArgumentExpressionAttributeis applied multiple times been considered (this requires to define a non-standard attribute)? The below code seems to assume only one usage.
As an example, forAsyncMethodBuilderAttributeonly the first application is consisdered (the others are ignored).
This method andCallerArgumentExpressionParameterIndexprobably need to agree on behavior (and not crash ;-).
Applying multiple times is interesting. I'll add a test. Not sure if this scenario will need a warning?
Such situation could be combined with an application of the attribute using a different constructor (breaks assertion below).
I assumed this can't happen because the attribute description defines what constructor signature should it be:
But definitely this worth a test.
|
Merged/squashed. Thanks @Youssef1313 ! |
|
Just thought of something else given that @333fred said we'll be ready to merge this soon: We'll need to create a new language version for VB. This could be done in this feature branch since that'll be the only VB feature. FYI for @KathleenDollard |
|
@jcouv I believe I already created a new language version in a previous PR. |
Proposal: dotnet/csharplang#287
Test plan: #52745
@AlekseyTs @333fred for review