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

Put record parameters in scope withing instance initializers #44906

Merged
merged 5 commits into from
Jun 6, 2020
Merged

Put record parameters in scope withing instance initializers #44906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
544916f
Put record parameters in scope withing instance initializers
AlekseyTs Jun 5, 2020
edab8c7
Merge remote-tracking branch 'dotnet/features/records' into Records_04
AlekseyTs Jun 6, 2020
f96f815
Follow-up on merge
AlekseyTs Jun 6, 2020
1cc4a0a
Share closure across initializers and the base call.
AlekseyTs Jun 6, 2020
2b0ca70
Merge remote-tracking branch 'dotnet/features/records' into Records_04
AlekseyTs Jun 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Initializers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,13 @@ internal static void BindRegularCSharpFieldInitializers(
}

Binder parentBinder = binderFactory.GetBinder(initializerNode);
Debug.Assert((parentBinder.ContainingMemberOrLambda is TypeSymbol containing && TypeSymbol.Equals(containing, fieldSymbol.ContainingType, TypeCompareKind.ConsiderEverything2)) || //should be the binder for the type
fieldSymbol.ContainingType.IsImplicitClass); //however, we also allow fields in namespaces to help support script scenarios

if (firstDebugImports == null)
{
firstDebugImports = parentBinder.ImportChain;
}

parentBinder = new LocalScopeBinder(parentBinder).WithAdditionalFlagsAndContainingMemberOrLambda(BinderFlags.FieldInitializer, fieldSymbol);
parentBinder = parentBinder.GetFieldInitializerBinder(fieldSymbol);

BoundFieldEqualsValue boundInitializer = BindFieldInitializer(parentBinder, fieldSymbol, initializerNode, diagnostics);
boundInitializers.Add(boundInitializer);
Expand Down Expand Up @@ -145,6 +143,21 @@ internal static void BindRegularCSharpFieldInitializers(
}
}

internal Binder GetFieldInitializerBinder(FieldSymbol fieldSymbol, bool suppressBinderFlagsFieldInitializer = false)
{
Debug.Assert((ContainingMemberOrLambda is TypeSymbol containing && TypeSymbol.Equals(containing, fieldSymbol.ContainingType, TypeCompareKind.ConsiderEverything2)) || //should be the binder for the type
fieldSymbol.ContainingType.IsImplicitClass); //however, we also allow fields in namespaces to help support script scenarios

Binder binder = this;

if (!fieldSymbol.IsStatic && fieldSymbol.ContainingType.GetMembersUnordered().OfType<SynthesizedRecordConstructor>().SingleOrDefault() is SynthesizedRecordConstructor recordCtor)
{
binder = new InMethodBinder(recordCtor, binder);
}

return new LocalScopeBinder(binder).WithAdditionalFlagsAndContainingMemberOrLambda(suppressBinderFlagsFieldInitializer ? BinderFlags.None : BinderFlags.FieldInitializer, fieldSymbol);
}

/// <summary>
/// In script C#, some field initializers are assignments to fields and others are global
/// statements. There are no restrictions on accessing instance members.
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ public override void VisitConstructorDeclaration(ConstructorDeclarationSyntax no

public override void VisitRecordDeclaration(RecordDeclarationSyntax node)
{
Debug.Assert(((RecordDeclarationSyntax)node).ParameterList is object);
Debug.Assert(node.ParameterList is object);

if (node.BaseWithArguments is SimpleBaseTypeSyntax baseWithArguments)
{
Expand Down
9 changes: 1 addition & 8 deletions src/Compilers/CSharp/Portable/Compilation/SyntaxTreeSemanticModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1261,15 +1261,8 @@ private FieldSymbol GetDeclaredFieldSymbol(VariableDeclaratorSyntax variableDecl

private Binder GetFieldOrPropertyInitializerBinder(FieldSymbol symbol, Binder outer, EqualsValueClauseSyntax initializer)
{
BinderFlags flags = BinderFlags.None;

// NOTE: checking for a containing script class is sufficient, but the regular C# test is quick and easy.
if (this.IsRegularCSharp || !symbol.ContainingType.IsScriptClass)
{
flags |= BinderFlags.FieldInitializer;
}

outer = new LocalScopeBinder(outer).WithAdditionalFlagsAndContainingMemberOrLambda(flags, symbol);
outer = outer.GetFieldInitializerBinder(symbol, suppressBinderFlagsFieldInitializer: !this.IsRegularCSharp && symbol.ContainingType.IsScriptClass);

if (initializer != null)
{
Expand Down
7 changes: 5 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

Expand Down Expand Up @@ -49,7 +49,10 @@ internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory F,
var param = F.Parameter(Parameters[0]);
foreach (var field in ContainingType.GetFieldsToEmit())
{
statements.Add(F.Assignment(F.Field(F.This(), field), F.Field(param, field)));
if (!field.IsStatic)
{
statements.Add(F.Assignment(F.Field(F.This(), field), F.Field(param, field)));
}
}
}
}
Expand Down
75 changes: 75 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/LookupPositionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2045,6 +2045,81 @@ interface C : Base(X)
TestLookupNames(text, expectedNames);
}

[Fact]
public void RecordInitializers_01()
{
var text = @"
record C(int X)
`{
int Z `= X + 1`;
`}
";
var expectedNames = MakeExpectedSymbols(
Add( // Global
"System",
"Microsoft",
"C"),
Add( // Members
"C C.Clone()",
"System.Boolean C.Equals(C? )",
"System.Boolean C.Equals(System.Object? )",
"System.Boolean System.Object.Equals(System.Object obj)",
"System.Boolean System.Object.Equals(System.Object objA, System.Object objB)",
"System.Boolean System.Object.ReferenceEquals(System.Object objA, System.Object objB)",
"System.Int32 C.GetHashCode()",
"System.Int32 C.X { get; init; }",
"System.Int32 C.Z",
"System.Int32 System.Object.GetHashCode()",
"System.Object System.Object.MemberwiseClone()",
"System.String System.Object.ToString()",
"System.Type System.Object.GetType()",
"void System.Object.Finalize()"),
Combine(
Remove("System.Int32 C.X { get; init; }"),
Add("System.Int32 X")
),
Combine(s_pop, s_pop),
s_pop
);

TestLookupNames(text, expectedNames);
}

[Fact]
public void RecordInitializers_02()
{
var text = @"
record C(int X)
`{
static int Z = X + 1;
`}
";
var expectedNames = MakeExpectedSymbols(
Add( // Global
"System",
"Microsoft",
"C"),
Add( // Members
"C C.Clone()",
"System.Boolean C.Equals(C? )",
"System.Boolean C.Equals(System.Object? )",
"System.Boolean System.Object.Equals(System.Object obj)",
"System.Boolean System.Object.Equals(System.Object objA, System.Object objB)",
"System.Boolean System.Object.ReferenceEquals(System.Object objA, System.Object objB)",
"System.Int32 C.GetHashCode()",
"System.Int32 C.X { get; init; }",
"System.Int32 C.Z",
"System.Int32 System.Object.GetHashCode()",
"System.Object System.Object.MemberwiseClone()",
"System.String System.Object.ToString()",
"System.Type System.Object.GetType()",
"void System.Object.Finalize()"),
s_pop
);

TestLookupNames(text, expectedNames);
}
Copy link
Member

@cston cston Jun 6, 2020 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Consider testing a const field as well. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider testing a const field as well.

Const fields are static fields, so I am pretty confident this test is fairly equivalent to verify scoping rules. I will add a test like that for completeness sake in a separate PR

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


/// <summary>
/// Given a program, calls LookupNames at each character position and verifies the results.
///
Expand Down
191 changes: 164 additions & 27 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,15 +341,7 @@ record C1(object O1)
public object O1 { get; } = O1;
public object O2 { get; } = O1;
}");
// PROTOTYPE: primary ctor parameters not currently in scope
comp.VerifyDiagnostics(
// (4,33): error CS0236: A field initializer cannot reference the non-static field, method, or property 'C1.O1'
// public object O1 { get; } = O1;
Diagnostic(ErrorCode.ERR_FieldInitRefNonstatic, "O1").WithArguments("C1.O1").WithLocation(4, 33),
// (5,33): error CS0236: A field initializer cannot reference the non-static field, method, or property 'C1.O1'
// public object O2 { get; } = O1;
Diagnostic(ErrorCode.ERR_FieldInitRefNonstatic, "O1").WithArguments("C1.O1").WithLocation(5, 33)
);
comp.VerifyDiagnostics();
}

[Fact]
Expand Down Expand Up @@ -389,15 +381,10 @@ public void RecordProperties_10()
}";
var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (1,1): warning CS1717: Assignment made to same variable; did you mean to assign something else?
// record C(object P)
Diagnostic(ErrorCode.WRN_AssignmentToSelf, @"record C(object P)
{
const int P = 4;
}").WithLocation(1, 1),
// (1,17): error CS0102: The type 'C' already contains a definition for 'P'
// record C(object P)
Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "P").WithArguments("C", "P").WithLocation(1, 17));
Diagnostic(ErrorCode.ERR_DuplicateNameInClass, "P").WithArguments("C", "P").WithLocation(1, 17)
);
}

[Fact]
Expand Down Expand Up @@ -2936,17 +2923,7 @@ public object P3 { set { } }
public ref object P5 => throw null;
}";
var comp = CreateCompilation(source);
comp.VerifyDiagnostics(
// (1,1): warning CS1717: Assignment made to same variable; did you mean to assign something else?
// record C(object P1, object P2, object P3, object P4, object P5)
Diagnostic(ErrorCode.WRN_AssignmentToSelf, @"record C(object P1, object P2, object P3, object P4, object P5)
{
public object P1 { get { return null; } set { } }
public object P2 { get; }
public object P3 { set { } }
public static object P4 { get; set; }
public ref object P5 => throw null;
}").WithLocation(1, 1));
comp.VerifyDiagnostics();

var actualMembers = GetProperties(comp, "C").ToTestDisplayStrings();
var expectedMembers = new[]
Expand All @@ -2958,6 +2935,22 @@ public object P3 { set { } }
"ref System.Object C.P5 { get; }",
};
AssertEx.Equal(expectedMembers, actualMembers);

var verifier = CompileAndVerify(source);

verifier.VerifyIL("C..ctor(C)", @"
{
// Code size 19 (0x13)
.maxstack 2
IL_0000: ldarg.0
IL_0001: call ""object..ctor()""
IL_0006: ldarg.0
IL_0007: ldarg.1
IL_0008: ldfld ""object C.<P2>k__BackingField""
IL_000d: stfld ""object C.<P2>k__BackingField""
IL_0012: ret
}
");
}

[Fact]
Expand Down Expand Up @@ -4298,5 +4291,149 @@ interface C : Base(X)
Assert.Empty(model.LookupSymbols(x.SpanStart, name: "X"));
Assert.DoesNotContain("X", model.LookupNames(x.SpanStart));
}

[Fact]
public void BaseArguments_15()
{
var src = @"
using System;

record Base
{
public Base(int X, int Y)
{
Console.WriteLine(X);
Console.WriteLine(Y);
}

public Base() {}
}

partial record C
{
}

partial record C(int X, int Y) : Base(X, Y)
{
int Z = 123;
public static void Main()
{
var c = new C(1, 2);
Console.WriteLine(c.Z);
}
}

partial record C
{
}
";
var verifier = CompileAndVerify(src, expectedOutput: @"
1
2
123");
verifier.VerifyIL("C..ctor(int, int)", @"

{
// Code size 31 (0x1f)
.maxstack 3
IL_0000: ldarg.0
IL_0001: ldarg.1
IL_0002: stfld ""int C.<X>k__BackingField""
IL_0007: ldarg.0
IL_0008: ldarg.2
IL_0009: stfld ""int C.<Y>k__BackingField""
IL_000e: ldarg.0
IL_000f: ldc.i4.s 123
IL_0011: stfld ""int C.Z""
IL_0016: ldarg.0
IL_0017: ldarg.1
IL_0018: ldarg.2
IL_0019: call ""Base..ctor(int, int)""
IL_001e: ret
}
");

var comp = CreateCompilation(src);

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);

var x = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Where(id => id.Identifier.ValueText == "X").ElementAt(1);
Assert.Equal("Base(X, Y)", x.Parent!.Parent!.Parent!.ToString());

var symbol = model.GetSymbolInfo(x).Symbol;
Assert.Equal(SymbolKind.Parameter, symbol!.Kind);
Assert.Equal("System.Int32 X", symbol.ToTestDisplayString());
Assert.Equal("C..ctor(System.Int32 X, System.Int32 Y)", symbol.ContainingSymbol.ToTestDisplayString());
Assert.Same(symbol.ContainingSymbol, model.GetEnclosingSymbol(x.SpanStart));
Assert.Contains(symbol, model.LookupSymbols(x.SpanStart, name: "X"));
Assert.Contains("X", model.LookupNames(x.SpanStart));
}

[Fact]
public void Initializers_01()
{
var src = @"
using System;

record C(int X)
{
int Z = X + 1;

public static void Main()
{
var c = new C(1);
Console.WriteLine(c.Z);
}
}";
var verifier = CompileAndVerify(src, expectedOutput: @"2");

var comp = CreateCompilation(src);

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);

var x = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Where(id => id.Identifier.ValueText == "X").First();
Assert.Equal("= X + 1", x.Parent!.Parent!.ToString());

var symbol = model.GetSymbolInfo(x).Symbol;
Assert.Equal(SymbolKind.Parameter, symbol!.Kind);
Assert.Equal("System.Int32 X", symbol.ToTestDisplayString());
Assert.Equal("C..ctor(System.Int32 X)", symbol.ContainingSymbol.ToTestDisplayString());
Assert.Equal("System.Int32 C.Z", model.GetEnclosingSymbol(x.SpanStart).ToTestDisplayString());
Assert.Contains(symbol, model.LookupSymbols(x.SpanStart, name: "X"));
Assert.Contains("X", model.LookupNames(x.SpanStart));
}

[Fact]
public void Initializers_02()
{
var src = @"
record C(int X)
{
static int Z = X + 1;
}";

var comp = CreateCompilation(src);
comp.VerifyDiagnostics(
// (4,20): error CS0236: A field initializer cannot reference the non-static field, method, or property 'C.X'
// static int Z = X + 1;
Diagnostic(ErrorCode.ERR_FieldInitRefNonstatic, "X").WithArguments("C.X").WithLocation(4, 20)
);

var tree = comp.SyntaxTrees.First();
var model = comp.GetSemanticModel(tree);

var x = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Where(id => id.Identifier.ValueText == "X").First();
Assert.Equal("= X + 1", x.Parent!.Parent!.ToString());

var symbol = model.GetSymbolInfo(x).Symbol;
Assert.Equal(SymbolKind.Property, symbol!.Kind);
Assert.Equal("System.Int32 C.X { get; init; }", symbol.ToTestDisplayString());
Assert.Equal("C", symbol.ContainingSymbol.ToTestDisplayString());
Assert.Equal("System.Int32 C.Z", model.GetEnclosingSymbol(x.SpanStart).ToTestDisplayString());
Assert.Contains(symbol, model.LookupSymbols(x.SpanStart, name: "X"));
Assert.Contains("X", model.LookupNames(x.SpanStart));
}
Copy link
Member

@cston cston Jun 6, 2020 •
edited by AlekseyTs
Loading

Choose a reason for hiding this comment

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

Consider testing capturing:

record C(int X)
{
    Func<int> F = () => X;
}
``` #Resolved

}
}