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

Support foreach over parameters in DoNotCopyValue #4374

Merged
merged 3 commits into from
Oct 28, 2020

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Oct 27, 2020

Builds on #4368

@sharwell sharwell requested a review from a team as a code owner October 27, 2020 23:50
// No special requirements
break;

case RefKind.RefReadOnly when operation.Syntax is CommonForEachStatementSyntax syntax && operation.SemanticModel.GetForEachStatementInfo(syntax).GetEnumeratorMethod is { IsReadOnly: true }:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting too difficult to manage - IMO this analyzer is not sustainable unless rewritten as a flow-based analyzer. I'll let you decide how to take this forward.

Copy link
Member Author

@sharwell sharwell Oct 28, 2020

Choose a reason for hiding this comment

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

IMO this analyzer is not sustainable unless rewritten as a flow-based analyzer.

Flow-based analysis does not help here. Every language construct needs to be allow-listed as it is encountered. The only real alternative would be new language support or IL analysis.

Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

At this point, I am not sure my reviews are valuable for this analyzer :-) - the analyzer feels way too complex and syntax/language construct dependent to me.

@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #4374 into master will decrease coverage by 0.00%.
The diff coverage is 81.98%.

@@            Coverage Diff             @@
##           master    #4374      +/-   ##
==========================================
- Coverage   95.79%   95.79%   -0.01%     
==========================================
  Files        1170     1172       +2     
  Lines      266722   266801      +79     
  Branches    16063    16069       +6     
==========================================
+ Hits       255511   255574      +63     
- Misses       9155     9165      +10     
- Partials     2056     2062       +6     

@sharwell sharwell merged commit 129a02d into dotnet:master Oct 28, 2020
@sharwell sharwell deleted the foreach-readonly branch October 28, 2020 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants