Skip to content

Implement IAttributeOperation #59369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
333fred merged 74 commits into from
Aug 16, 2022
Merged

Implement IAttributeOperation #59369

333fred merged 74 commits into from
Aug 16, 2022

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Feb 8, 2022

Fixes #18198
Fixes #53618

Not yet ready to merge, but wanted to get initial feedback.

FYI @CyrusNajmabadi

@Youssef1313 Youssef1313 requested a review from a team as a code owner February 8, 2022 15:07
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 8, 2022
RikkiGibson
RikkiGibson reviewed Feb 8, 2022
View reviewed changes
Youssef1313
Youssef1313 commented Feb 9, 2022
View reviewed changes
@Youssef1313 Youssef1313 changed the title Initial implementation of IAttributeOperation Implement of IAttributeOperation Feb 9, 2022
@Youssef1313 Youssef1313 changed the title Implement of IAttributeOperation Implement IAttributeOperation Feb 9, 2022
Youssef1313
Youssef1313 commented Feb 9, 2022
View reviewed changes
@Youssef1313
Copy link
Member Author

Youssef1313 commented Feb 10, 2022

@RikkiGibson Can you take another look? I haven't yet added tests for params. But want to know if the general approach is good/correct.

Youssef1313
Youssef1313 commented Feb 10, 2022
View reviewed changes
Youssef1313
Youssef1313 commented Feb 10, 2022
View reviewed changes
Youssef1313
Youssef1313 commented Feb 10, 2022
View reviewed changes
@RikkiGibson RikkiGibson self-assigned this Feb 10, 2022
333fred
333fred reviewed Jul 20, 2022
View reviewed changes
Copy link
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.

Overall this is looking very good. I think it just needs a few more tests.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 21, 2022

@333fred I added more tests.

333fred
333fred approved these changes Jul 21, 2022
View reviewed changes
Copy link
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 72). @dotnet/roslyn-compiler for a second review.

RikkiGibson
RikkiGibson approved these changes Jul 22, 2022
View reviewed changes
Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM with very minor nits. Let us know if you plan on changing anything in response to the new comments.

src/Compilers/CSharp/Portable/Compilation/AttributeSemanticModel.cs
/// <summary>
/// Creates an AttributeSemanticModel that allows asking semantic questions about an attribute node.
/// </summary>
public static AttributeSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, AttributeSyntax syntax, NamedTypeSymbol attributeType, AliasSymbol aliasOpt, Symbol? attributeTarget, Binder rootBinder, ImmutableDictionary<Symbol, Symbol> parentRemappedSymbolsOpt)
Copy link
Member

@RikkiGibson RikkiGibson Jul 21, 2022

Choose a reason for hiding this comment

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

Was this addressed by #62137?

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests.cs
}

[Fact]
public void TestAttributeCallerInfoSemanticModel_Method_Speculative2()
Copy link
Member

@RikkiGibson RikkiGibson Jul 22, 2022

Choose a reason for hiding this comment

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

I didn't understand what this was testing versus the original. Does it matter that Method_Speculative uses a string literal and Method_Speculative2 uses a constant-valued interpolated string?

Copy link
Member Author

@Youssef1313 Youssef1313 Jul 22, 2022

Choose a reason for hiding this comment

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

I don't recall why did I add this test :(
I think it doesn't harm anyway.

Copy link
Member

@RikkiGibson RikkiGibson Jul 22, 2022

Choose a reason for hiding this comment

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

Works for me. Thanks

@RikkiGibson
Copy link
Member

RikkiGibson commented Jul 22, 2022

Tagging @dotnet/roslyn-ide for review of changes under src/Analyzers/

@333fred 333fred mentioned this pull request Aug 12, 2022
Closed
JoeRobich
JoeRobich approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

The changes under src/Analyzers look good to me.

@333fred
Copy link
Member

333fred commented Aug 16, 2022

@Youssef1313 can you resolve the conflict? Then we can get this merged :)

@333fred 333fred merged commit d89769b into dotnet:main Aug 16, 2022
@333fred
Copy link
Member

333fred commented Aug 16, 2022

Thanks @Youssef1313!

@ghost ghost added this to the Next milestone Aug 16, 2022
333fred added a commit that referenced this pull request Aug 17, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into deref-unused
f1e1da5 
* upstream/main:
  Fix compilation of exe RefFieldTests (#63431)
  Implement IAttributeOperation (#59369)
  Restore nullable warnings for unset required members from base types (#63286)
  Remove portion of incremental-analyzer-infrastructure that has no usage.
@Youssef1313 Youssef1313 deleted the iattribute-operation branch August 17, 2022 03:39
@333fred 333fred mentioned this pull request Aug 17, 2022
Merged
dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 19, 2022
@dibarbet
Revert "Implement IAttributeOperation (dotnet#59369)"
7f59718 
This reverts commit d89769b.
JoeRobich added a commit that referenced this pull request Aug 19, 2022
@JoeRobich
Merge pull request #63485 from dibarbet/revert_iop
ae56638 
Revert "Implement IAttributeOperation (#59369)"
333fred added a commit to dotnet/roslyn-analyzers that referenced this pull request Aug 29, 2022
@333fred
Do not warn on zero-length allocations in unspecified params paramete…
7768502 
…rs in attributes

Fixes an issue introduced with dotnet/roslyn#59369, as the AvoidZeroLengthAllocations analyzer did not see these params parameters before.
@dibarbet dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022
333fred added a commit that referenced this pull request Oct 4, 2022
@333fred
Revert "Revert "Implement IAttributeOperation (#59369)""
216a200
333fred added a commit that referenced this pull request Oct 12, 2022
@333fred
Revert "Revert "Implement IAttributeOperation (#59369)"" (#64488)
e34e164
@ryzngard ryzngard mentioned this pull request Oct 19, 2022
Merged
@rainersigwald rainersigwald mentioned this pull request Mar 6, 2023
Closed
ViktorHofer pushed a commit to dotnet/sdk that referenced this pull request Aug 15, 2025
@333fred
Do not warn on zero-length allocations in unspecified params paramete…
62eb4e9 
…rs in attributes

Fixes an issue introduced with dotnet/roslyn#59369, as the AvoidZeroLengthAllocations analyzer did not see these params parameters before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoeRobich JoeRobich JoeRobich approved these changes

@333fred 333fred 333fred approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@AlekseyTs AlekseyTs Awaiting requested review from AlekseyTs

@jcouv jcouv Awaiting requested review from jcouv

Assignees

@RikkiGibson RikkiGibson

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New IOperation: IAttributeOperation IOperation API for attributes

8 participants

@Youssef1313 @AlekseyTs @333fred @RikkiGibson @CyrusNajmabadi @JoeRobich @jcouv @dibarbet