Use record keyword to display records by jcouv · Pull Request #46338 · dotnet/roslyn

jcouv · 2020-07-26T06:10:15Z

AlekseyTs · 2020-07-27T15:44:05Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

                {
                    switch (symbol.TypeKind)
                    {
+                        case TypeKind.Class when !symbol.GetMembers(WellKnownMemberNames.CloneMethodName).IsEmpty:


!symbol.GetMembers(WellKnownMemberNames.CloneMethodName).IsEmpty [](start = 49, length = 64)

I think we should duplicate the filter condition from SynthesizedRecordClone.FindValidCloneMethod and leave a comment there to keep this place in sync in case of future changes to the logic. #Closed

I'm okay to do the more complete check, but would prefer not to duplicate logic. I'm pushing a change to leverage FindValidCloneMethod. Let me know if okay

AlekseyTs · 2020-07-27T15:46:34Z

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs

        }
+
+        [Fact]
+        public void RecordDeclaration()


RecordDeclaration [](start = 20, length = 17)

In RecordTests.cs we have a bunch of IL tests with invalid Clone declarations, I think we should add a ToTestDisplayString validation to them. #Closed

AlekseyTs · 2020-07-27T15:47:07Z

Done with review pass (iteration 1) #Closed

AlekseyTs · 2020-07-28T15:52:36Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+            static bool hasCloneMethod(INamedTypeSymbol symbol)
+            {
+                HashSet<DiagnosticInfo> ignored = null;
+                return symbol is Symbols.PublicModel.NamedTypeSymbol namedTypeSymbol &&


symbol is Symbols.PublicModel.NamedTypeSymbol namedTypeSymbol [](start = 23, length = 61)

This looks wrong. Please add a test that imports a record to VB, gets a symbol for it (VB symbol) and pushes it through here by feeding the symbol to C# symbol display. This is a supported scenario. #Closed

AlekseyTs · 2020-07-28T15:59:50Z

Done with review pass (iteration 2) #Closed

AlekseyTs · 2020-07-28T23:43:24Z

    public void Clone_05()

Could you please preserve the number in the name at the same position? #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:1575 in 4fdfe55. [](commit_id = 4fdfe55, deletion_comment = True)

cston · 2020-07-28T23:52:19Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+
+            return candidate;
+
+            bool IsEqualToOrDerivedFrom(ITypeSymbol one, ITypeSymbol other, SymbolEqualityComparer comparison)


Can this local function be static?

In reply to: 461958297 [](ancestors = 461958297)

AlekseyTs · 2020-07-28T23:55:33Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+
+            return candidate;
+
+            bool IsEqualToOrDerivedFrom(ITypeSymbol one, ITypeSymbol other, SymbolEqualityComparer comparison)


IsEqualToOrDerivedFrom [](start = 17, length = 22)

Is this a local function? The name shouldn't start with a capital letter. #Closed

cston · 2020-07-28T23:57:31Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

        }

+        [Fact]
+        public void RecordLoadedInVisualBasicDisplaysAsRecord()


RecordLoadedInVisualBasicDisplaysAsRecord [](start = 20, length = 41)

Consider moving to SymbolDisplayTests.

AlekseyTs · 2020-07-29T00:03:43Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+
+            return candidate;
+
+            bool IsEqualToOrDerivedFrom(ITypeSymbol one, ITypeSymbol other, SymbolEqualityComparer comparison)


SymbolEqualityComparer comparison [](start = 76, length = 33)

We are always passing a singleton, is there a reason to pass it as an argument? #Closed

trying to keep code closer to original

trying to keep code closer to original

I do not believe this is useful, this method already diverged significantly from the one we are trying to clone

In reply to: 461962590 [](ancestors = 461962590)

AlekseyTs · 2020-07-29T00:06:08Z

Done with review pass (iteration 5) #Closed

AlekseyTs

LGTM (iteration 7)

cston · 2020-07-29T00:15:27Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+        /// <summary>
+        /// Copy of <see cref="SynthesizedRecordClone.FindValidCloneMethod(TypeSymbol, ref HashSet{DiagnosticInfo}?)"/>
+        /// </summary>
+        internal static IMethodSymbol FindValidCloneMethod(ITypeSymbol containingType)


internal [](start = 8, length = 8)

private #Closed

cston · 2020-07-29T00:16:23Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+                {
+                    if (candidate is object)
+                    {
+                        // An ammbiguity case, can come from metadata, treat as an error for simplicity.


ammbiguity [](start = 30, length = 10)

Typo. #Closed

cston · 2020-07-29T00:25:21Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

+                }
+
+                return false;
+            }


} [](start = 12, length = 1)

Consider using a single call to Equals() and a single call to BaseType.

do { if (one.Equals(other, comparison)) return true; one = one.BaseType; } while (one != null); return false; ``` #Closed

* upstream/master: (304 commits) Tweak diagnostics to account for records (dotnet#46341) Diagnose precedence inversion in a warning wave (dotnet#46239) Remove PROTOTYPE tag (dotnet#45965) Only run a single pass of NullableWalker per-member (dotnet#46402) Fix crash, and offer "declare as nullable" for tuple fields (dotnet#46437) Simplify contract for RunWithShutdownBlockAsync Fix optprof plugin input check if EditorAdaptersFactoryService gives us a null buffer Cannot assign maybe-null value to TNotNull variable (dotnet#41445) Fix overload resolution to handle binary compat in the face of covariant returns (dotnet#46367) Same failure on Linux Skip some tests on Mac Added search option for inline parameter name hints Spelling tweak docs Improve comment Improve the "not exhaustive" diagnostic in the presence of a when clause. (dotnet#46143) PR feedback Use record keyword to display records (dotnet#46338) remove test that aserts .NET Standard should be prefered over .NET Framework ...

jcouv added Area-IDE Area-Compilers Feature - Records Records labels Jul 26, 2020

jcouv self-assigned this Jul 26, 2020

jcouv added this to the 16.8 milestone Jul 26, 2020

Use record keyword to display records

456f9b6

jcouv force-pushed the record-info branch from a0098d7 to 456f9b6 Compare July 26, 2020 14:19

jcouv marked this pull request as ready for review July 27, 2020 01:20

jcouv requested review from a team as code owners July 27, 2020 01:20

RikkiGibson approved these changes Jul 27, 2020

View reviewed changes

davidwengier approved these changes Jul 27, 2020

View reviewed changes

AlekseyTs reviewed Jul 27, 2020

View reviewed changes

CyrusNajmabadi approved these changes Jul 27, 2020

View reviewed changes

jasonmalinowski approved these changes Jul 27, 2020

View reviewed changes

Address feedback

4fdfe55

jcouv requested a review from AlekseyTs July 28, 2020 14:19

AlekseyTs reviewed Jul 28, 2020

View reviewed changes

Duplicate FindValidCloneMethod logic

9794c22

Julien Couvreur added 2 commits July 28, 2020 16:45

Merge remote-tracking branch 'dotnet/master' into record-info

fc7a25d

Tweak

e1957e3

jcouv requested a review from AlekseyTs July 28, 2020 23:48

cston reviewed Jul 28, 2020

View reviewed changes

AlekseyTs reviewed Jul 28, 2020

View reviewed changes

cston reviewed Jul 28, 2020

View reviewed changes

dotnet deleted a comment from jcouv Jul 28, 2020

AlekseyTs reviewed Jul 29, 2020

View reviewed changes

Julien Couvreur added 2 commits July 28, 2020 17:06

Fix local function

0a23b53

Number test names

97025be

AlekseyTs approved these changes Jul 29, 2020

View reviewed changes

cston reviewed Jul 29, 2020

View reviewed changes

Julien Couvreur added 2 commits July 28, 2020 21:29

Address feedback from Chuck

bf46efd

static

74fa8f4

cston approved these changes Jul 29, 2020

View reviewed changes

jcouv merged commit 64bbbc4 into dotnet:master Jul 29, 2020

ghost modified the milestones: 16.8, Next Jul 29, 2020

jcouv deleted the record-info branch July 29, 2020 22:02

RikkiGibson modified the milestones: Next, 16.8.P2 Aug 11, 2020

jcouv mentioned this pull request Sep 15, 2020

Custom record constructors are highlighted as struct #46985

Closed


		return candidate;

		bool IsEqualToOrDerivedFrom(ITypeSymbol one, ITypeSymbol other, SymbolEqualityComparer comparison)

Conversation

jcouv commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

cston Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 28, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jcouv commented Jul 26, 2020 •

edited

Loading

AlekseyTs Jul 27, 2020 •

edited

Loading

AlekseyTs Jul 27, 2020 •

edited

Loading

AlekseyTs commented Jul 27, 2020 •

edited

Loading

AlekseyTs Jul 28, 2020 •

edited

Loading

AlekseyTs commented Jul 28, 2020 •

edited

Loading

AlekseyTs commented Jul 28, 2020 •

edited

Loading

AlekseyTs Jul 28, 2020 •

edited

Loading

AlekseyTs Jul 29, 2020 •

edited

Loading

AlekseyTs commented Jul 29, 2020 •

edited

Loading

cston Jul 29, 2020 •

edited

Loading

cston Jul 29, 2020 •

edited

Loading

cston Jul 29, 2020 •

edited

Loading