Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Report nullable ctor warnings for struct primary constructors #74844

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Report nullable ctor warnings for struct primary constructors #74844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3581,7 +3581,7 @@ private BoundNode BindSimpleProgramCompilationUnit(CompilationUnitSyntax compila
private BoundNode BindPrimaryConstructorBody(TypeDeclarationSyntax typeDecl, BindingDiagnosticBag diagnostics)
{
Debug.Assert(typeDecl.ParameterList is object);
Debug.Assert(typeDecl.Kind() is SyntaxKind.RecordDeclaration or SyntaxKind.ClassDeclaration);
Debug.Assert(typeDecl.Kind() is SyntaxKind.RecordDeclaration or SyntaxKind.ClassDeclaration or SyntaxKind.RecordStructDeclaration or SyntaxKind.StructDeclaration);

BoundExpressionStatement initializer;
ImmutableArray<LocalSymbol> constructorLocals;
Expand Down
7 changes: 1 addition & 6 deletions src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1735,12 +1735,7 @@ private static void GetStateMachineSlotDebugInfo(

initializersBody ??= GetSynthesizedEmptyBody(method);

if (method is SynthesizedPrimaryConstructor primaryCtor && method.ContainingType.IsStructType())
{
body = BoundBlock.SynthesizedNoLocals(primaryCtor.GetSyntax());
Copy link
Member

Choose a reason for hiding this comment

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

What replicates this logic now? What body is created for the primary constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same logic which was used for class primary constructors is now also used for struct primary constructors. You can see that where the assertions were adjusted.

I reviewed the code around this area and didn't spot anything which would be problematic for structs, but, it's quite possible I missed something.

nullableInitialState = getInitializerState(body);
}
else if (method is SourceMemberMethodSymbol sourceMethod)
Comment on lines -1738 to -1743
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This path was causing us to miss nullable analyzing the struct primary constructor.

if (method is SourceMemberMethodSymbol sourceMethod)
{
CSharpSyntaxNode syntaxNode = sourceMethod.SyntaxNode;

Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,8 +639,7 @@ void enforceMemberNotNull(SyntaxNode? syntaxOpt, LocalState state)
Debug.Assert(thisParameter is object);
thisSlot = GetOrCreateSlot(thisParameter);
}
// https://github.com/dotnet/roslyn/issues/46718: give diagnostics on return points, not constructor signature
var exitLocation = method.DeclaringSyntaxReferences.IsEmpty ? null : method.TryGetFirstLocation();
var exitLocation = method is SynthesizedPrimaryConstructor || method.DeclaringSyntaxReferences.IsEmpty ? null : method.TryGetFirstLocation();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a type has a primary constructor, all other instance constructors will be required to chain to it (directly or indirectly), so it will be the only constructor which will get warnings for uninitialized non-nullable instance fields. That means the field/property declaration is really the best place to report diagnostics in such cases.

bool constructorEnforcesRequiredMembers = method.ShouldCheckRequiredMembers();

// Required properties can be attributed MemberNotNull, indicating that if the property is set, the field will be set as well.
Expand Down
90 changes: 90 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Semantics/PrimaryConstructorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22122,5 +22122,95 @@ public event EventHandler OnClosed
var comp = CreateCompilation(source, options: TestOptions.ReleaseDll);
comp.VerifyEmitDiagnostics();
}

[Theory]
[InlineData("class")]
[InlineData("struct")]
[WorkItem("https://github.com/dotnet/roslyn/issues/74726")]
public void PrimaryCtorNullableFieldWarning(string typeKind)
{
var source = $$"""
#nullable enable

public {{typeKind}} C()
{
public string Text { get; set; } // 1
public C(bool ignored) : this() { }
}

public {{typeKind}} C2
{
public string Text { get; set; }
public C2() { } // 2
}

public {{typeKind}} C3()
{
public string Text { get; set; } = "a";
public C3(bool ignored) : this() { }
}

public {{typeKind}} C4
{
public string Text { get; set; }
public C4() { Text = "a"; }
}
""";

var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (5,19): warning CS8618: Non-nullable property 'Text' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
// public string Text { get; set; } // 1
Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "Text").WithArguments("property", "Text").WithLocation(5, 19),
// (12,12): warning CS8618: Non-nullable property 'Text' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
// public C2() { } // 2
Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C2").WithArguments("property", "Text").WithLocation(12, 12)
);
}

[Theory]
[InlineData("class")]
[InlineData("struct")]
[WorkItem("https://github.com/dotnet/roslyn/issues/74726")]
public void RecordPrimaryCtorNullableFieldWarning(string typeKind)
{
var source = $$"""
#nullable enable

public record {{typeKind}} C()
{
public string Text { get; set; } // 1
public C(bool ignored) : this() { }
}

public record {{typeKind}} C2
{
public string Text { get; set; }
public C2() { } // 2
}

public record {{typeKind}} C3()
{
public string Text { get; set; } = "a";
public C3(bool ignored) : this() { }
}

public record {{typeKind}} C4
{
public string Text { get; set; }
public C4() { Text = "a"; }
}
""";

var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (5,19): warning CS8618: Non-nullable property 'Text' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
// public string Text { get; set; } // 1
Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "Text").WithArguments("property", "Text").WithLocation(5, 19),
// (12,12): warning CS8618: Non-nullable property 'Text' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
// public C2() { } // 2
Diagnostic(ErrorCode.WRN_UninitializedNonNullableField, "C2").WithArguments("property", "Text").WithLocation(12, 12)
);
}
}
}