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.

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

Add code fix for MarkMembersStatic (CA1822) #1982

Merged
merged 3 commits into from Jan 8, 2019
Merged

Add code fix for MarkMembersStatic (CA1822) #1982

merged 3 commits into from Jan 8, 2019

Conversation

mavasani
Copy link
Member

@mavasani mavasani commented Jan 7, 2019

I borrowed the code from Fred's original PR #1116, and extended it to fix the references along with the declaration. We still conservatively bail out for cases where the replacement can potentially lead to semantic changes (such as say the instance receiver of method invocation itself is an invocation). However, these will be very rare cases and still cause explicit build break, so I think we should be fine. We can refine this behavior in future based on customer feedback.

Fixes #471
Fixes #1929

@mavasani
Add code fix for MarkMembersStatic (CA1822)
753114b 
I borrowed the code from Fred's original PR dotnet#1116, and extended it to
fix the references along with the declaration. We conservatively bail
out for cases where the replacement can potentially lead to semantic
changes - these will be very rare cases and still cause explicit build break, so I think we should be
fine.

Fixes dotnet#471
Fixes dotnet#1929
@mavasani mavasani requested review from heejaechang, 333fred and a team January 7, 2019 23:07
@mavasani
Copy link
Member Author

mavasani commented Jan 7, 2019

Tagging @Neme12

@Neme12
Copy link

Neme12 commented Jan 7, 2019 •
edited

Is it possible that this could in any way be shared with dotnet/roslyn#31838, and if so, should it? That fix would be different in that there shouldn't be any usages to worry about, so maybe it's OK to have completely separate implementations.

@mavasani
Copy link
Member Author

mavasani commented Jan 7, 2019

@Neme12 Unfortunately, not as the code is in different repos, and this one is based off a CAXXXX diagnostic generated in FxCop analyzers NuGet package. We can certainly borrow test/product code as appropriate, but sharing seems unlikely.

@Neme12
Copy link

Neme12 commented Jan 7, 2019

Yeah I was thinking the code fix in Roslyn could say it fixes CA1822 in addition to CS0708 through FixableDiagnosticIds but that's way too ugly.

@mavasani
Change MakeStaticAnalyzer to register the invocation operation action…
f0a1a6d 
… inside operation block start. Without this change, the operation block start and nested actions never seem to execute within live analysis in IDE (I will file a separate Roslyn bug for it)
genlu
genlu approved these changes Jan 8, 2019
View reviewed changes
Copy link
Member

@genlu genlu left a comment •
edited

Choose a reason for hiding this comment

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

LGTM. It'd be nice to have some cross language tests.

@mavasani mavasani mentioned this pull request Jan 8, 2019
Open
@mavasani mavasani merged commit 60535a6 into dotnet:master Jan 8, 2019
@mavasani mavasani deleted the MakeStatic branch January 8, 2019 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genlu genlu genlu approved these changes

@heejaechang heejaechang heejaechang left review comments

@Neme12 Neme12 Neme12 left review comments

@333fred 333fred Awaiting requested review from 333fred

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Code fix for CA1822 to make member static Port FxCop rule CA1822: MarkMembersAsStatic
4 participants
@mavasani @Neme12 @genlu @heejaechang