Skip to content

CallerArgumentExpression: Warning for self-referential#54129

Merged
333fred merged 3 commits intodotnet:features/caller-argument-expressionfrom
Youssef1313:fix-prototypes
Jun 17, 2021
Merged

CallerArgumentExpression: Warning for self-referential#54129
333fred merged 3 commits intodotnet:features/caller-argument-expressionfrom
Youssef1313:fix-prototypes

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 requested a review from a team as a code owner June 16, 2021 04:33
@ghost ghost added the Area-Compilers label Jun 16, 2021
@Youssef1313
Copy link
Copy Markdown
Member Author

CI is stuck. Closing & reopening.

@Youssef1313 Youssef1313 reopened this Jun 16, 2021
<value>The CallerArgumentExpressionAttribute applied to parameter '{0}' will have no effect because it's self-referential.</value>
</data>
<data name="WRN_CallerArgumentExpressionAttributeSelfReferential_Title" xml:space="preserve">
<value>The CallerArgumentExpressionAttribute is applied with a self-referential parameter.</value>
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 16, 2021

Choose a reason for hiding this comment

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

The CallerArgumentExpressionAttribute is applied with a self-referential parameter.

We usually use the original message with removed placeholders. I think this will work well in this case as well: "The CallerArgumentExpressionAttribute applied to parameter will have no effect because it's self-referential." #Closed


var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe);
compilation.VerifyDiagnostics();
compilation.VerifyDiagnostics(
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 16, 2021

Choose a reason for hiding this comment

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

.VerifyDiagnostics(

Consider calling this on the result returned by CompileAndVerify instead. #Closed


var compilation = CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, options: TestOptions.ReleaseExe, parseOptions: TestOptions.Regular9);
compilation.VerifyDiagnostics();
compilation.VerifyDiagnostics(
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 16, 2021

Choose a reason for hiding this comment

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

VerifyDiagnostics

Same comment #Closed

// PROTOTYPE(caller-expr): Should this have a warning?
[ConditionalFact(typeof(CoreClrOnly))]
public void TestArgumentExpressionIsReferingToItself()
public void TestArgumentExpressionIsSelfReferential()
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 16, 2021

Choose a reason for hiding this comment

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

TestArgumentExpressionIsSelfReferential

Do we have a test covering consumption of similar attribute from metadata? Perhaps we can compile similar class to image reference, confirm the attribute is there and test the same scenario against that image. #Closed

@AlekseyTs
Copy link
Copy Markdown
Contributor

Done with the review pass (commit 1)

@Youssef1313
Copy link
Copy Markdown
Member Author

@AlekseyTs Thanks for review. I addressed your feedback.

@AlekseyTs AlekseyTs added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jun 17, 2021
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@AlekseyTs
Copy link
Copy Markdown
Contributor

@dotnet/roslyn-compiler, @333fred Please review, need a second review for a Community PR.

Copy link
Copy Markdown
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 3)

@333fred 333fred merged commit c979f20 into dotnet:features/caller-argument-expression Jun 17, 2021
@333fred
Copy link
Copy Markdown
Member

333fred commented Jun 17, 2021

Thanks @Youssef1313!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature - CallerArgumentExpressionAttribute

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants