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

Non-copyable analysis for fields and auto-properties #4798

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Feb 7, 2021

  • Do not allow a non-copyable field to be defined in a copyable type
  • Do not allow a non-copyable auto-property to be defined (the getter in this case always copies the backing field value)

@sharwell sharwell requested a review from a team as a code owner February 7, 2021 22:15
public override void VisitAlias(IAliasSymbol symbol)
{
base.VisitAlias(symbol);
}
Copy link
Member

Choose a reason for hiding this comment

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

i don't quite get the purpose of the overrides. is it just to be explicit that this is the behavior you want? (if so, that's fine. just trying to understand).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this analyzer overrides everything.


public override void VisitProperty(IPropertySymbol symbol)
{
// Auto-properties cannot have non-copyable types
Copy link
Member

Choose a reason for hiding this comment

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

probably worth commenting why?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user-facing description of the rule includes the explanation. The getter for an auto-property always copies the backing field, which is not allowed for a non-copyable type.

Copy link
Member Author

@sharwell sharwell Feb 8, 2021

Choose a reason for hiding this comment

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

➡️ I updated the code to include more complete descriptions. The user facing messages still have descriptions as well.

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #4798 (14703b1) into master (e2f66f0) will decrease coverage by 0.01%.
The diff coverage is 84.89%.

@@            Coverage Diff             @@
##           master    #4798      +/-   ##
==========================================
- Coverage   95.69%   95.68%   -0.02%     
==========================================
  Files        1197     1197              
  Lines      271784   272061     +277     
  Branches    16433    16439       +6     
==========================================
+ Hits       260082   260309     +227     
- Misses       9599     9623      +24     
- Partials     2103     2129      +26     

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