Extend make type abstract to include records #48227

Youssef1313 · 2020-10-01T22:00:44Z

Fixes #48129

…stractCodeFixProvider

…ctCodeFixProvider

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs

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

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

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

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

Youssef1313 · 2020-10-01T22:15:02Z

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


 Namespace Microsoft.CodeAnalysis.VisualBasic.MakeClassAbstract
    <ExportCodeFixProvider(LanguageNames.VisualBasic, Name:=NameOf(VisualBasicMakeClassAbstractCodeFixProvider)), [Shared]>
    Friend NotInheritable Class VisualBasicMakeClassAbstractCodeFixProvider


Do you want to update the VB class name as well?

I would say yes for consistency.

davidwengier

Couple of minor test comments, but LGTM in general.

If you're bored, a test for a record with a primary constructor might be nice to ensure no regressions, but I'd be very surprised if it caught anything :)

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs

Youssef1313 · 2020-10-01T22:29:53Z

@davidwengier Thanks for review. I addressed your feedback. (Sorry for having much commits, working from web browser currently 😄)

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs

Youssef1313 · 2020-10-02T18:46:37Z

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

-                    editor.ReplaceNode(classDeclaration,
-                        (currentClassDeclaration, generator) => generator.WithModifiers(currentClassDeclaration, DeclarationModifiers.Abstract));
+                    editor.ReplaceNode(typeDeclaration,
+                        (currentTypeDeclaration, generator) => generator.WithModifiers(currentTypeDeclaration, DeclarationModifiers.Abstract));


This is not working due to lack of support for records in syntax generator.
Being fixed in #48096

Evangelink · 2020-10-03T09:00:38Z

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs

+            typeDeclaration = null;

-            switch (node?.Kind())
+            switch (node)


I don't know if we care about it but switching over the enum shall be "faster" than switching over the casted expressions.

@Evangelink I'd let @CyrusNajmabadi or @davidwengier decide on this.

for smal switches this is fine.

Evangelink · 2020-10-03T09:01:33Z

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


 Namespace Microsoft.CodeAnalysis.VisualBasic.MakeClassAbstract
    <ExportCodeFixProvider(LanguageNames.VisualBasic, Name:=NameOf(VisualBasicMakeClassAbstractCodeFixProvider)), [Shared]>
    Friend NotInheritable Class VisualBasicMakeClassAbstractCodeFixProvider


I would say yes for consistency.

Evangelink · 2020-10-03T09:04:01Z

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs

        public override ImmutableArray<string> FixableDiagnosticIds { get; } =
               ImmutableArray.Create(
-                   "CS0513" // 'C.M()' is abstract but it is contained in non-abstract class 'C'
+                   "CS0513" // 'C.M()' is abstract but it is contained in non-abstract type 'C'


I am assuming the analyzer was updated for this and the message updated, isn't it?

@Evangelink Yes the message for CS0513 was updated.

CyrusNajmabadi · 2020-10-04T05:39:29Z

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs


-            var enclosingType = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();
-            if (!enclosingType.IsKind(SyntaxKind.ClassDeclaration))
+            typeDeclaration = node.FirstAncestorOrSelf<TypeDeclarationSyntax>();


don't do this. you will assign the value, even when returning false. it would be far cleaner to keep the out-value 'null' when 'false' is returned.

CyrusNajmabadi · 2020-10-04T05:40:33Z

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs

+        [Theory, Trait(Traits.Feature, Traits.Features.CodeActionsMakeTypeAbstract)]
+        [InlineData("class")]
+        [InlineData("record")]
+        public async Task TestMethod(string typeKind)


can you revert all of this add only new tests for records. parameterizing here seems to provide no value and doubles the amount of tests without need.

davidwengier · 2020-11-04T21:08:32Z

@Youssef1313 is this blocked on anything still?

Youssef1313 · 2020-11-05T11:00:03Z

@Youssef1313 is this blocked on anything still?

@davidwengier I don't think. Let's see if the tests will pass.

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs

…stractCodeFixProvider.cs

Youssef1313 · 2020-11-05T15:43:25Z

@davidwengier Can you re-run the failed job? The failure is an unrelated test timeout:

Roslyn Error: test timeout exceeded, dumping remaining processes
Dumping dotnet 5320 to F:\workspace\_work\1\s\artifacts\log\Debug\dotnet-1.dmp ... succeeded (129216285 bytes)
Dumping dotnet 5668 to F:\workspace\_work\1\s\artifacts\log\Debug\dotnet-2.dmp ... succeeded (109926677 bytes)
Dumping testhost.net472.x86 5612 to F:\workspace\_work\1\s\artifacts\log\Debug\testhost.net472.x86-3.dmp ... succeeded (701572436 bytes)
##[error](NETCORE_ENGINEERING_TELEMETRY=Test) Tests failed

davidwengier · 2020-11-05T22:28:13Z

Thanks @Youssef1313

* upstream/master: (519 commits) Remove workaround in PEMethodSymbol.ExplicitInterfaceImplementations (dotnet#49246) Enable LSP pull model diagnostic for XAML. (dotnet#49145) Update dependencies from https://github.com/dotnet/roslyn build 20201109.8 (dotnet#49240) Add test for with expression in F1 help service (dotnet#49236) Cache RegexPatternDetector per compilation Fix RazorRemoteHostClient to maintain binary back-compat Further tweak inline hints Fix MemberNames API for records (dotnet#49138) Minor cleanups (dotnet#49204) Report warning for assignment or explicit cast of possibly null value of unconstrained type parameter type (dotnet#48803) Clean up of EditorFeatures.Cocoa.Snippets (dotnet#49188) Fix OK button state handling. Make relation between viewmodels more tightly coupled Extend make type abstract to include records (dotnet#48227) Remove duplicated implementations of C# event hookup Add select all/deselect all buttons Consolidate conditional compilation (dotnet#49150) Implement IEquatable on Microsoft.Cci.DefinitionWithLocation structure (dotnet#49162) Add new document extensions file Unify implementations Only disable structure tagger provider in LSP scenario ...

Youssef1313 added 5 commits October 1, 2020 23:36

Rename AbstractMakeClassAbstractCodeFixProvider to AbstractMakeTypeAb…

c94024e

…stractCodeFixProvider

Update VisualBasicMakeClassAbstractCodeFixProvider.vb

d8ca0db

Rename CSharpMakeClassAbstractCodeFixProvider to CSharpMakeTypeAbstra…

0f2f211

…ctCodeFixProvider

Update tests to include records

140e9a3

Update MakeTypeAbstractTests.cs

16450fb

Youssef1313 requested a review from a team as a code owner October 1, 2020 22:00

Youssef1313 mentioned this pull request Oct 1, 2020

CSharpMakeClassAbstractCodeFixProvider needs to be updated for records #48129

Closed

Youssef1313 commented Oct 1, 2020

View reviewed changes

Fix namespaces

a417b77

Youssef1313 commented Oct 1, 2020

View reviewed changes

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

Youssef1313 commented Oct 1, 2020

View reviewed changes

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

Youssef1313 added 4 commits October 2, 2020 00:06

Fix namespace

276875a

Fix

9dbf9f7

Rename

ff75e05

Update CSharpMakeTypeAbstractCodeFixProvider.cs

95f9685

Youssef1313 commented Oct 1, 2020

View reviewed changes

davidwengier approved these changes Oct 1, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated Show resolved Hide resolved

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated Show resolved Hide resolved

Youssef1313 added 4 commits October 2, 2020 00:23

Fix FixAll test

ba57424

Rename trait in Traits.cs

b46acea

Rename trait in C# test

b8bf84f

Rename trait in VB test

76c8ccf

Youssef1313 added 2 commits October 2, 2020 13:01

Fix typo

57027c3

Fix typo

a7cde2e

This was referenced Oct 2, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

Youssef1313 commented Oct 2, 2020

View reviewed changes

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

Youssef1313 commented Oct 2, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/MakeTypeAbstract/MakeTypeAbstractTests.cs Outdated Show resolved Hide resolved

Final fixes

1a70a53

Youssef1313 commented Oct 2, 2020

View reviewed changes

jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 3, 2020

jinujoseph assigned davidwengier Oct 3, 2020

Evangelink reviewed Oct 3, 2020

View reviewed changes

Youssef1313 added 2 commits October 3, 2020 13:06

Update VB codefix

69514cb

Update VB tests

b8a55f6

CyrusNajmabadi reviewed Oct 4, 2020

View reviewed changes

Youssef1313 added 3 commits October 5, 2020 02:08

Ensure that typeDeclaration is null when returning false

158d8b7

Revert InlineDatas

7f0bb1a

Fix traits

c0ab249

Merge branch 'master' into patch-38

ae218bc

Youssef1313 requested a review from a team as a code owner November 5, 2020 10:59

Youssef1313 commented Nov 5, 2020

View reviewed changes

src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAbstractCodeFixProvider.cs Outdated Show resolved Hide resolved

Youssef1313 added 4 commits November 5, 2020 13:25

Update src/Features/CSharp/Portable/MakeTypeAbstract/CSharpMakeTypeAb…

c0a39b2

…stractCodeFixProvider.cs

Update CSharpMakeTypeAbstractCodeFixProvider.cs

6e7bb15

Update CSharpMakeTypeAbstractCodeFixProvider.cs

35daead

Update CSharpMakeTypeAbstractCodeFixProvider.cs

6ecffc9

jcouv added the Feature - Records Records label Nov 5, 2020

davidwengier merged commit c73027c into dotnet:master Nov 5, 2020

ghost added this to the Next milestone Nov 5, 2020

Youssef1313 deleted the patch-38 branch November 6, 2020 02:39

shyamnamboodiripad pushed a commit that referenced this pull request Nov 6, 2020

Extend make type abstract to include records (#48227)

2959366

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Youssef1313 mentioned this pull request Feb 18, 2021

Test plan for "record structs" #51199

Closed

92 tasks

Extend make type abstract to include records #48227

Extend make type abstract to include records #48227

Uh oh!

Conversation

Youssef1313 commented Oct 1, 2020

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!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Youssef1313 commented Oct 1, 2020

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!

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!

davidwengier commented Nov 4, 2020

Uh oh!

Youssef1313 commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Youssef1313 commented Nov 5, 2020

Uh oh!

davidwengier commented Nov 5, 2020

Uh oh!

Uh oh!

Youssef1313 commented Nov 5, 2020 •

edited

Loading