Implement IAttributeOperation by Youssef1313 · Pull Request #59369 · dotnet/roslyn

Youssef1313 · 2022-02-08T15:07:27Z

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

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

Youssef1313 · 2022-02-10T12:36:06Z

@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.

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs

333fred

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

Youssef1313 · 2022-07-21T07:29:16Z

@333fred I added more tests.

333fred

LGTM (commit 72). @dotnet/roslyn-compiler for a second review.

RikkiGibson

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

RikkiGibson · 2022-07-21T19:43:34Z

src/Compilers/CSharp/Portable/Compilation/AttributeSemanticModel.cs

@@ -38,7 +41,7 @@ private AttributeSemanticModel(
        public static AttributeSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, AttributeSyntax syntax, NamedTypeSymbol attributeType, AliasSymbol aliasOpt, Symbol? attributeTarget, Binder rootBinder, ImmutableDictionary<Symbol, Symbol> parentRemappedSymbolsOpt)


Was this addressed by #62137?

RikkiGibson · 2022-07-22T19:09:41Z

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

+        }
+
+        [Fact]
+        public void TestAttributeCallerInfoSemanticModel_Method_Speculative2()


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?

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

Works for me. Thanks

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IAttributeOperation.cs

…s_IAttributeOperation.cs

RikkiGibson · 2022-07-22T20:26:09Z

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

JoeRobich

The changes under src/Analyzers look good to me.

333fred · 2022-08-16T17:34:45Z

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

333fred · 2022-08-16T22:16:08Z

Thanks @Youssef1313!

* 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.

This reverts commit d89769b.

Revert "Implement IAttributeOperation (#59369)"

…rs in attributes Fixes an issue introduced with dotnet/roslyn#59369, as the AvoidZeroLengthAllocations analyzer did not see these params parameters before.

Initial implementation of IAttributeOperation

eb4fdf1

Youssef1313 requested a review from a team as a code owner February 8, 2022 15:07

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Feb 8, 2022

Update IOperationTests_IAttributeOperation.cs

218fbbf

RikkiGibson reviewed Feb 8, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits February 9, 2022 16:31

Fix implementation

52e8782

Fix freeing

1611fca

Youssef1313 commented Feb 9, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs Outdated Show resolved Hide resolved

Youssef1313 changed the title ~~Initial implementation of IAttributeOperation~~ Implement of IAttributeOperation Feb 9, 2022

Youssef1313 changed the title ~~Implement of IAttributeOperation~~ Implement IAttributeOperation Feb 9, 2022

Youssef1313 added 3 commits February 9, 2022 17:18

Add caller info test

c074598

Produce InvalidOperation for null attribute constructor

75bcbda

Fix crash

c184951

Youssef1313 commented Feb 9, 2022

View reviewed changes

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

Youssef1313 added 2 commits February 9, 2022 19:51

Apply suggestions from code review

395f0cd

Fix implementation

878080b

Youssef1313 commented Feb 10, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs Outdated Show resolved Hide resolved

Youssef1313 commented Feb 10, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs Outdated Show resolved Hide resolved

Youssef1313 added 4 commits February 10, 2022 14:38

Add assertion back

3af6851

null suppressions

aeebffd

Match existing hasErrors logic

28aa869

Fix failures

a7834df

Youssef1313 commented Feb 10, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Attributes.cs Outdated Show resolved Hide resolved

RikkiGibson self-assigned this Feb 10, 2022

RikkiGibson added Area-Compilers Feature - IOperation IOperation labels Feb 10, 2022

Youssef1313 added 3 commits February 10, 2022 23:41

Fix test

443b239

Attempt to fix failing test

dfdf708

Null suppressions

322d436

333fred reviewed Jul 20, 2022

View reviewed changes

Youssef1313 added 2 commits July 21, 2022 08:13

Merge branch 'main' into iattribute-operation

5a406a0

Add more tests

76fb6f9

333fred approved these changes Jul 21, 2022

View reviewed changes

RikkiGibson approved these changes Jul 22, 2022

View reviewed changes

Youssef1313 commented Jul 22, 2022

View reviewed changes

src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IAttributeOperation.cs Outdated Show resolved Hide resolved

Update src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTest…

dd72277

…s_IAttributeOperation.cs

RikkiGibson approved these changes Jul 22, 2022

View reviewed changes

333fred mentioned this pull request Aug 12, 2022

Building Corelib in runtime triggers several asserts #63348

Closed

JoeRobich approved these changes Aug 15, 2022

View reviewed changes

Merge branch 'main' into iattribute-operation

d87efc2

333fred merged commit d89769b into dotnet:main Aug 16, 2022

ghost added this to the Next milestone Aug 16, 2022

Youssef1313 deleted the iattribute-operation branch August 17, 2022 03:39

333fred mentioned this pull request Aug 17, 2022

Do not warn on zero-length allocations in unspecified params parameters in attributes dotnet/roslyn-analyzers#6114

Merged

dibarbet added a commit to dibarbet/roslyn that referenced this pull request Aug 19, 2022

Revert "Implement IAttributeOperation (dotnet#59369)"

7f59718

This reverts commit d89769b.

JoeRobich added a commit that referenced this pull request Aug 19, 2022

Merge pull request #63485 from dibarbet/revert_iop

ae56638

Revert "Implement IAttributeOperation (#59369)"

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022

333fred added a commit that referenced this pull request Oct 4, 2022

Revert "Revert "Implement IAttributeOperation (#59369)""

216a200

333fred added a commit that referenced this pull request Oct 12, 2022

Revert "Revert "Implement IAttributeOperation (#59369)"" (#64488)

e34e164

ryzngard mentioned this pull request Oct 19, 2022

Fix Color Contrast For STE Close Button #64834

Merged

rainersigwald mentioned this pull request Mar 6, 2023

[Broken Build]: Updating MSBuild to version 17.5 makes CA1507 appear #67195

Closed

		@@ -38,7 +41,7 @@ private AttributeSemanticModel(
		public static AttributeSemanticModel Create(SyntaxTreeSemanticModel containingSemanticModel, AttributeSyntax syntax, NamedTypeSymbol attributeType, AliasSymbol aliasOpt, Symbol? attributeTarget, Binder rootBinder, ImmutableDictionary<Symbol, Symbol> parentRemappedSymbolsOpt)

Conversation

Youssef1313 commented Feb 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Youssef1313 commented Feb 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Jul 21, 2022

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 21, 2022

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RikkiGibson commented Jul 22, 2022

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

333fred commented Aug 16, 2022

Uh oh!

333fred commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants