Create synthesized record constructor #40625

agocke · 2019-12-27T21:44:10Z

This just adds a synthesized symbol (no body) for the constructor

Relates to #40726 (test plan for records)

gafter · 2020-01-06T03:51:01Z

https://github.com/dotnet/csharplang/blob/master/proposals/records-wip.md doesn't say anything about a constructor.

gafter · 2020-01-06T03:53:12Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

+}");
+            comp.VerifyDiagnostics();
+            var c = comp.GlobalNamespace.GetTypeMember("C");
+            var ctors = c.GetMembers(".ctor");


Assert ctors.Length? #Resolved

agocke · 2020-01-06T08:25:07Z

Spec work at dotnet/csharplang#3076

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

agocke · 2020-01-13T23:34:51Z

@dotnet/roslyn-compiler for one more review

333fred · 2020-01-15T22:28:34Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -1,5 +1,7 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.

+#nullable enable


In the future, please consider doing any nullable changes for a full file as a separate commit, so it's easy to see what's nullable annotation changes and the actual work being done in the PR.

333fred · 2020-01-15T22:30:22Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -199,7 +201,7 @@ public bool SetNullableContext(byte? value)
            _flags = new Flags(specialType, typeKind);

            var containingType = this.ContainingType;
-            if ((object)containingType != null && containingType.IsSealed && this.DeclaredAccessibility.HasProtected())
+            if (containingType?.IsSealed == true && this.DeclaredAccessibility.HasProtected())


Nit: Our general approach to nullable annotating has been to keep changes minimal. If you're going to go as far as to use ?., you may was well go all the way to pattern matching.

This is minimal only because the conversion was producing a nullable warning.

333fred · 2020-01-15T22:32:34Z

    internal override void ForceComplete(SourceLocation locationOpt, CancellationToken cancellationToken)

Should be annotated.

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs:429 in 2f48801. [](commit_id = 2f48801, deletion_comment = False)

333fred · 2020-01-15T22:46:23Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -1528,7 +1531,7 @@ private void CheckMemberNameConflicts(DiagnosticBag diagnostics)

                    var conversion = symbol as SourceUserDefinedConversionSymbol;
                    var method = symbol as SourceMemberMethodSymbol;
-                    if ((object)conversion != null)
+                    if (!(conversion is null))


!(conversion is null) [](start = 24, length = 21)

Consider is object instead.

When you say never, you're being inconsistent with the rest of the compiler codebase.

I am not convinced by the assertion that is object is a clear and straightforward way to test for null.

There are plenty of places we don't use is object in the code base today. Until we settle on a single pattern, any of them are acceptable.

333fred · 2020-01-15T22:46:49Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -1559,7 +1562,7 @@ private void CheckMemberNameConflicts(DiagnosticBag diagnostics)
                        // Do not add the conversion to the set of previously-seen methods; that set
                        // is only non-conversion methods.
                    }
-                    else if ((object)method != null)
+                    else if (!(method is null))


333fred · 2020-01-15T22:55:47Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

                    {
                        diagnostics.Add(ErrorCode.ERR_PartialMethodMustHaveLatent, method.Locations[0], method);
                    }
-                    else if ((object)method.OtherPartOfPartial != null && MemberSignatureComparer.ConsideringTupleNamesCreatesDifference(method, method.OtherPartOfPartial))
+                    else if (!(method.OtherPartOfPartial is null) && MemberSignatureComparer.ConsideringTupleNamesCreatesDifference(method, method.OtherPartOfPartial))


is object

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedRecordConstructor.cs

333fred · 2020-01-15T23:01:39Z

Done review pass (commit 4)

333fred · 2020-01-16T17:57:06Z

@agocke still waiting for this to be addressed before approving: #40625 (comment)

agocke · 2020-01-16T21:19:21Z

@333fred done

cston · 2020-01-16T21:30:32Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

@@ -1303,14 +1306,14 @@ private MembersAndInitializers GetMembersAndInitializers()
            AddDeclarationDiagnostics(diagnostics);
            diagnostics.Free();

-            return membersAndInitializers;
+            return _lazyMembersAndInitializers!;


_lazyMembersAndInitializers [](start = 19, length = 27)

Was this change needed?

Probably not. I think I misread the early return there.

cston · 2020-01-16T21:35:12Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+            var memberSignatures = new HashSet<Symbol>(members, MemberSignatureComparer.DuplicateSourceComparer);
+
+            var ctor = new SynthesizedRecordConstructor(this, binder, paramList, diagnostics);
+            if (!memberSignatures.Contains(ctor))


Rather than constructing a HashSet<>, would it be more efficient to just call members.Contains(ctor)?

But I don't want to know if it has the exact symbol, I want to know if it has any symbol with a conflicting signature.

members.Contains(ctor, MemberSignatureComparer.DuplicateSourceComparer)

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

Right, a linear walk. But this is just the ctor. I'm also gonna need to query for the other methods, like the generated properties, equals, hashcode, etc.

cston · 2020-01-16T21:40:52Z

[CompilerTrait(CompilerFeature.ReadOnlyReferences)]

Is this correct?

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs:9 in 8d91657. [](commit_id = 8d91657, deletion_comment = False)

cston · 2020-01-16T21:41:10Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/RecordTests.cs

+
+namespace Microsoft.CodeAnalysis.CSharp.UnitTests
+{
+    public class RecordTests : CompilingTestBase


RecordTests [](start = 17, length = 11)

Why create a new class rather than adding to Semantics.RecordTests?

Just splitting apart the checks of the symbol API from behavioral checks.

agocke force-pushed the record-ctor branch 3 times, most recently from 59e8ec1 to 95a6de9 Compare January 2, 2020 04:47

jinujoseph added the Area-Compilers label Jan 5, 2020

agocke marked this pull request as ready for review January 5, 2020 23:25

agocke requested review from a team as code owners January 5, 2020 23:25

Generate ctor from the record syntax

08f6625

agocke force-pushed the record-ctor branch from 52e486b to 08f6625 Compare January 5, 2020 23:26

Add checking for existing ctor

3ff96cd

gafter reviewed Jan 6, 2020

View reviewed changes

gafter approved these changes Jan 6, 2020

View reviewed changes

agocke added 2 commits January 6, 2020 19:59

Add error for parameter list without 'data'

d5b861c

Respond to PR feedback

2f48801

gafter reviewed Jan 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Show resolved Hide resolved

gafter reviewed Jan 7, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Show resolved Hide resolved

agocke requested a review from a team January 10, 2020 01:51

333fred reviewed Jan 15, 2020

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedRecordConstructor.cs Show resolved Hide resolved

nullable enable in one more file

7d4b5c1

Add '?'

8d91657

cston reviewed Jan 16, 2020

View reviewed changes

Revert change to GetMembersAndInitializers

28d0443

agocke mentioned this pull request Jan 16, 2020

Synthesize record properties and ctor #41011

Merged

Accidentally added compiler trait

019f110

333fred approved these changes Jan 17, 2020

View reviewed changes

agocke closed this Jan 17, 2020

agocke reopened this Jan 17, 2020

agocke merged commit 28b8eb5 into dotnet:features/records Jan 17, 2020

agocke deleted the record-ctor branch January 17, 2020 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create synthesized record constructor #40625

Create synthesized record constructor #40625

agocke commented Dec 27, 2019 •

edited by jcouv

Loading

gafter commented Jan 6, 2020

gafter Jan 6, 2020 •

edited

Loading

agocke commented Jan 6, 2020

agocke commented Jan 13, 2020

333fred Jan 15, 2020

333fred Jan 15, 2020

agocke Jan 15, 2020

333fred commented Jan 15, 2020

333fred Jan 15, 2020

agocke Jan 16, 2020

333fred Jan 16, 2020

gafter Jan 16, 2020

agocke Jan 16, 2020

333fred Jan 15, 2020

333fred Jan 15, 2020

333fred commented Jan 15, 2020

333fred commented Jan 16, 2020

agocke commented Jan 16, 2020

cston Jan 16, 2020

agocke Jan 16, 2020

cston Jan 16, 2020

agocke Jan 16, 2020

cston Jan 16, 2020

agocke Jan 16, 2020

cston commented Jan 16, 2020

cston Jan 16, 2020

agocke Jan 16, 2020

		@@ -1,5 +1,7 @@
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

		#nullable enable

Create synthesized record constructor #40625

Create synthesized record constructor #40625

Conversation

agocke commented Dec 27, 2019 • edited by jcouv Loading

gafter commented Jan 6, 2020

gafter Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

agocke commented Jan 6, 2020

agocke commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Jan 15, 2020

333fred commented Jan 16, 2020

agocke commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Jan 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke commented Dec 27, 2019 •

edited by jcouv

Loading

gafter Jan 6, 2020 •

edited

Loading