Add code fix for CS0513 by Evangelink · Pull Request #40448 · dotnet/roslyn

Evangelink · 2019-12-17T15:17:03Z

Make enclosing class abstract when member does not have body.

Fix #31833

Make enclosing class abstract when member does not have body.

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

CyrusNajmabadi · 2019-12-17T23:19:22Z

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

+                {
+                    var enclosingClass = declaration.FirstAncestorOrSelf<TClassDeclarationSyntax>();
+                    editor.ReplaceNode(enclosingClass,
+                        (currentClassDeclaration, generator) => generator.WithModifiers(currentClassDeclaration, DeclarationModifiers.Abstract));


won't this blow away other modifiers? don't you need to 'union' this with the existing mods?

That was my thought at first but this seem to keep the existing modifiers and just apply the new one (with some validity check).

Yes, the current behavior seems strange to me for a With method. Ideally it should overwrite, not merge. It is unlikely we would change the behavior though as it would be a breaking change.

@mavasani Well it can simply be overwrite as we need to keep the other modifiers or am I missing something?

@mavasani so what do you want me to do here?

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

CyrusNajmabadi · 2019-12-17T23:20:17Z

hey @Evangelink Thanks for the PR!

CyrusNajmabadi · 2019-12-17T23:20:28Z

Missing VB impl and tests currently. Can you add those?

Evangelink · 2019-12-19T10:08:50Z

@CyrusNajmabadi I haven't added an impl for VB.NET because I couldn't find the equivalent error in Errors.vb looking for abstract or mustinherit. If you could point me to the error, I'll gladly provide the impl.

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs

CyrusNajmabadi · 2019-12-19T19:17:06Z

Just experiment in VB to see waht you get. For example, i tried this out:

And got htis error:

Severity	Code	Description	Project	File	Line	Suppression State
Error	BC31411	'C' must be declared 'MustInherit' because it contains methods declared 'MustOverride'

jcouv · 2020-01-03T18:16:58Z

@vatsalyaagrawal I removed the Area-Compilers label as I don't see any compiler file change. Let me know if I missed something. Thanks

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs

mavasani

Overall LGTM. Some minor suggestions and an open question on whether the abstract modifier should be added to all declarations of a partial type.

mavasani · 2020-06-10T11:50:18Z

@Evangelink can you please resolve conflicts and fix CI?

Evangelink · 2020-07-03T14:38:28Z

I have "fixed" the CI but I don't understand why so many cases do not work for vbnet (including the fixall), the implementation seems to match other codefix.

Evangelink · 2020-07-03T17:56:03Z

See #42814

Evangelink · 2020-07-06T14:28:59Z

@mavasani I don't understand what's going wrong with the tests/PR. I have copied the behavior/structure from the AbstractMakeMemberStaticCodeFixProvider (and sub-classes).

mavasani · 2020-07-06T14:57:22Z

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MakeClassAbstract
+{
+    public class MakeClassAbstractTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest


@Evangelink Can we instead just convert these tests to use the new test framework? Will that be too much work? That will avoid you having to add a public ctor for this code fixer and then suppressing RS0033.

If https://github.com/dotnet/roslyn/pull/40448/files#r450282581 addresses the CI, it is fine to skip changing the tests for this PR.

I have tried (if I got it right that the syntax is the same as for roslyn-analyzers) but I end-up with nuget errors: NuGet.Protocol.Core.Types.FatalProtocolException : Unable to load the service index for source https://api.nuget.org/v3/index.json.

I think it is fine to not do this as part of this PR. You may want to file a follow-up issue with your questions. Also suggest looking at the existing tests which are already moved to new framework, for example: https://github.com/dotnet/roslyn/blob/master/src/Analyzers/CSharp/Tests/RemoveUnnecessaryImports/RemoveUnnecessaryImportsTests.cs

...atures/VisualBasic/Portable/MakeClassAbstract/VisualBasicMakeClassAbstractCodeFixProvider.vb

Add code fix for CS0513

6c2912c

Make enclosing class abstract when member does not have body.

Evangelink requested a review from a team as a code owner December 17, 2019 15:17

Evangelink commented Dec 17, 2019

View reviewed changes

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

Evangelink commented Dec 17, 2019

View reviewed changes

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Dec 17, 2019

View reviewed changes

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

Address review comments

dd8d03e

Evangelink commented Dec 19, 2019

View reviewed changes

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs Show resolved Hide resolved

Remove un-needed temp variable

7a3efdd

Evangelink requested review from CyrusNajmabadi and removed request for a team December 19, 2019 16:09

CyrusNajmabadi reviewed Dec 19, 2019

View reviewed changes

src/Features/CSharp/Portable/MakeClassAbstract/CSharpMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

WIP

40aa0f5

vatsalyaagrawal added the Area-Compilers label Dec 23, 2019

jcouv removed the Area-Compilers label Jan 3, 2020

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. Feature Request labels Jan 4, 2020

jinujoseph assigned mavasani Jan 4, 2020

jinujoseph added this to the 16.5.P2 milestone Jan 4, 2020

mavasani reviewed Feb 3, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/MakeClassAbstract/MakeClassAbstractTests.cs Show resolved Hide resolved

mavasani reviewed Feb 3, 2020

View reviewed changes

src/Features/Core/Portable/MakeClassAbstract/AbstractMakeClassAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

mavasani approved these changes Feb 3, 2020

View reviewed changes

Evangelink added 2 commits February 13, 2020 14:33

Merge branch 'master' into codefix-CS0513

91ddd91

Address review comment

92d52fa

Evangelink mentioned this pull request Feb 13, 2020

SyntaxGenerator.Merge is not merging abstract and partial correctly #41654

Closed

Skip failing partial abstract test

bf11973

jinujoseph modified the milestones: 16.5.P2, 16.6 Feb 21, 2020

jinujoseph modified the milestones: 16.6, 16.7 Apr 19, 2020

Evangelink added 2 commits July 3, 2020 11:36

Merge branch 'master' into codefix-CS0513

55d3c81

Fix broken tests

f4b05f4

Evangelink requested a review from mavasani July 3, 2020 14:44

mavasani reviewed Jul 6, 2020

View reviewed changes

...atures/VisualBasic/Portable/MakeClassAbstract/VisualBasicMakeClassAbstractCodeFixProvider.vb Outdated Show resolved Hide resolved

Fix MEF export for vbnet

649e623

mavasani merged commit 195ddaf into dotnet:master Jul 6, 2020

ghost modified the milestones: 16.7, Next Jul 6, 2020

mavasani modified the milestones: Next, 16.8, 16.8.P1 Jul 6, 2020

Evangelink deleted the codefix-CS0513 branch July 6, 2020 21:36

Conversation

Evangelink commented Dec 17, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Dec 17, 2019

Uh oh!

CyrusNajmabadi commented Dec 17, 2019

Uh oh!

Evangelink commented Dec 19, 2019

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Dec 19, 2019

Uh oh!

jcouv commented Jan 3, 2020

Uh oh!

Uh oh!

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

mavasani commented Jun 10, 2020

Uh oh!

Evangelink commented Jul 3, 2020

Uh oh!

Evangelink commented Jul 3, 2020

Uh oh!

Evangelink commented Jul 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants