Skip to content
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.

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

New constant expected analyzer #5766

Merged
merged 32 commits into from
Nov 16, 2022

Conversation

wzchua
Copy link
Contributor

@wzchua wzchua commented Dec 13, 2021

Resolves dotnet/runtime#33771
This is still a WIP and I'm using CA1860 and CA1861 as placeholder.
I believe the logic covers most of the general cases.

It is still not fully clear to me the rules for applying this attribute to a generic parameter.
I have added logic to handle some of it but not sure if this is how the team would like them.

  • Add more tests

@buyaa-n @tannergooding

@wzchua wzchua force-pushed the new-analyzer/constant-expected branch from 9ee7606 to c97c002 Compare December 13, 2021 11:21
@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #5766 (266fba8) into main (5182a8a) will increase coverage by 0.00%.
The diff coverage is 98.15%.

❗ Current head 266fba8 differs from pull request most recent head ca89072. Consider uploading reports for the commit ca89072 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##             main    #5766     +/-   ##
=========================================
  Coverage   96.04%   96.05%             
=========================================
  Files        1360     1364      +4     
  Lines      312225   313686   +1461     
  Branches    10047    10125     +78     
=========================================
+ Hits       299890   301324   +1434     
- Misses       9928     9940     +12     
- Partials     2407     2422     +15     

@wzchua wzchua force-pushed the new-analyzer/constant-expected branch 2 times, most recently from 3d61e3f to 02c9847 Compare December 13, 2021 14:58
@wzchua wzchua force-pushed the new-analyzer/constant-expected branch from 38ac251 to 9cf0495 Compare January 8, 2022 04:30
@deeprobin
Copy link
Contributor

@wzchua Are you still working on this?

@wzchua
Copy link
Contributor Author

wzchua commented Feb 23, 2022

Yea, it is effectively done but I'm sure I'm still missing something.

Also, I'm not sure if the team wants the vb analyzer

@deeprobin
Copy link
Contributor

Can you please check if the analyzer works also on VB?

dotnet/runtime#33771 (comment)
And we need some VB Tests and then its imo ready to review

@wzchua
Copy link
Contributor Author

wzchua commented Feb 25, 2022

It won't work, it depends on language specific syntax since there is no IOperation for attribute

@wzchua
Copy link
Contributor Author

wzchua commented Feb 25, 2022

I'll do a rebase and mark it for review later in the day

@wzchua wzchua force-pushed the new-analyzer/constant-expected branch from 84d6e4d to 5f8922f Compare February 25, 2022 11:36
@wzchua
Copy link
Contributor Author

wzchua commented Feb 25, 2022

The test is still missing a reference assembly with the attribute

@wzchua wzchua marked this pull request as ready for review February 25, 2022 11:41
@wzchua wzchua requested a review from a team as a code owner February 25, 2022 11:41
Co-authored-by: Robin Lindner <robin.lindner1@t-online.de>
Copy link
Contributor

@deeprobin deeprobin left a comment

Choose a reason for hiding this comment

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

LGTM

@tannergooding
Copy link
Member

This isn't something I have the relevant expertise to review. It only impacts some of the APIs in my area as things that might use the analyzer.

CC> @buyaa-n

@wzchua
Copy link
Contributor Author

wzchua commented Mar 26, 2022

@tannergooding. From the test, you can view the behaviour of the analyzer. Let me know if the behaviour does not match what is expected.

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 2, 2022

Thanks @wzchua, looks a few comments still not addressed yet, otherwise looks good

@wzchua
Copy link
Contributor Author

wzchua commented Aug 27, 2022

I have resolved the remaining comments

@wzchua wzchua force-pushed the new-analyzer/constant-expected branch from 5369cc6 to 4a752f7 Compare August 27, 2022 07:51
wzchua added 4 commits August 27, 2022 19:00
ConstantExpectedContext captures the INamedTypeSymbol and wrap methods related to checking with the ConstantExpected attribute
@wzchua wzchua force-pushed the new-analyzer/constant-expected branch from 4a752f7 to 865974c Compare August 27, 2022 11:00
Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution @wzchua, I would simplify the perf test a bit

@buyaa-n
Copy link
Contributor

buyaa-n commented Sep 13, 2022

FYI: @mavasani @jmarolf it is safe to merge this analyzer in 7.0 as it would not cause any warnings because the ConstantExpected attribute is not yet applied to any APIs in runtime. And we have no plan to apply any attribute within 7.0. If its OK with you and have you have done reviewing we can merge the analyzer

@tannergooding
Copy link
Member

What's the status of this and is it ready to merge?

@buyaa-n
Copy link
Contributor

buyaa-n commented Oct 3, 2022

What's the status of this and is it ready to merge?

This is ready (except it got new merge conflicts), it can be merged into 7.0 if @jmarolf @mavasani OK with merging (It would not cause any warning as no any API attributed with ConstantExpectedAttribute in 7.0). Else I will merge it when the main branch will become for 8.0

@buyaa-n buyaa-n enabled auto-merge (squash) November 16, 2022 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass constants to parameters marked as [ConstantExpected]
5 participants