Skip to content

LFT-2910: Detect mutability from primary class constructors #977

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

Merged
j3parker merged 1 commit into from
Jun 3, 2026
Merged

LFT-2910: Detect mutability from primary class constructors #977

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
9 changes: 9 additions & 0 deletions src/D2L.CodeStyle.Analyzers/Diagnostics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,5 +774,14 @@ public static class Diagnostics {
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true
);

public static readonly DiagnosticDescriptor PrimaryClassConstructorIntroducesMutability = new DiagnosticDescriptor(
id: "D2L0105",
title: "Primary class constructors introduce mutability into classes via their parameters",
messageFormat: "Primary class constructors introduce mutability into classes via their parameters",
category: "Immutability",
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true
);
}
}
4 changes: 3 additions & 1 deletion src/D2L.CodeStyle.Analyzers/Immutability/ImmutabilityAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ public sealed partial class ImmutabilityAnalyzer : DiagnosticAnalyzer {

Diagnostics.MissingTransitiveImmutableAttribute,
Diagnostics.InconsistentMethodAttributeApplication,
Diagnostics.UnappliedConditionalImmutability
Diagnostics.UnappliedConditionalImmutability,

Diagnostics.PrimaryClassConstructorIntroducesMutability
);

private readonly ImmutableHashSet<string> m_additionalImmutableTypes;
Expand Down
33 changes: 33 additions & 0 deletions src/D2L.CodeStyle.Analyzers/Immutability/ImmutableDefinitionChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ public bool CheckDeclaration( INamedTypeSymbol type ) {
.Where( m => !m.IsStatic );

if( type.TypeKind == TypeKind.Class ) {
if( HasPrimaryClassConstructorMutability( type ) ) {
result = false;
}

// Check that the base class is immutable for classes
var baseClassOk = m_context.IsImmutable(
new ImmutabilityQuery(
Expand Down Expand Up @@ -81,6 +85,35 @@ out var diag
return result;
}

public bool HasPrimaryClassConstructorMutability( INamedTypeSymbol @class ) {
foreach( var constructor in @class.InstanceConstructors ) {
foreach( var syntaxRef in constructor.DeclaringSyntaxReferences ) {

@j3parker j3parker Jun 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is some open chatter in the compiler about exposing more of this stuff via the semantic model but the current situation is you need to go into the syntax.

@j3parker j3parker Jun 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You might wonder like don't they show up as properties or something in the GetMembers() call earlier, and the answer is that no they don't.

var syntax = syntaxRef.GetSyntax( m_cancellationToken );

// Primary constructors for classes introduce mutability.
if( syntax is not ClassDeclarationSyntax ctor ) {
continue;
}

// Unimportant edge case
if( ctor.ParameterList.Parameters.Count == 0 ) {
return false;
}

m_diagnosticSink(
Diagnostic.Create(
Diagnostics.PrimaryClassConstructorIntroducesMutability,
ctor.ParameterList.GetLocation()
)
);
Comment on lines +103 to +108

@omsmith omsmith Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So, this is only the case if the parameters get used outside of field initializers. Really we just want the Roslyn team's RS0062, or to add an equivalent.

https://andrewlock.net/blocking-primary-constructor-member-capture-using-an-analyzer/

using System;

sealed class A( string P ) {
    void M() {
        Console.WriteLine( P );
    }
}


sealed class B( string P ) {
    private readonly string m_p = P;

    void M() {
        Console.WriteLine( m_p );
    }
}
using System;
using System.Diagnostics;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Security;
using System.Security.Permissions;

[assembly: CompilationRelaxations(8)]
[assembly: RuntimeCompatibility(WrapNonExceptionThrows = true)]
[assembly: Debuggable(DebuggableAttribute.DebuggingModes.Default | DebuggableAttribute.DebuggingModes.IgnoreSymbolStoreSequencePoints | DebuggableAttribute.DebuggingModes.EnableEditAndContinue | DebuggableAttribute.DebuggingModes.DisableOptimizations)]
[assembly: SecurityPermission(SecurityAction.RequestMinimum, SkipVerification = true)]
[assembly: AssemblyVersion("0.0.0.0")]
[module: UnverifiableCode]
[module: RefSafetyRules(11)]
[NullableContext(1)]
[Nullable(0)]
internal sealed class A
{
	public A(string P)
	{
		<P>P = P;
		base..ctor();
	}

	private void M()
	{
		Console.WriteLine(<P>P);
	}
}
[NullableContext(1)]
[Nullable(0)]
internal sealed class B
{
	private readonly string m_p;

	public B(string P)
	{
		m_p = P;
		base..ctor();
	}

	private void M()
	{
		Console.WriteLine(m_p);
	}
}

https://lab.razor.fyi/#43rPyMUVUJSfXpSYq5dcLPSAsbQ4My9dIbiyuCQ115qLqzg1MSc1RSE5J7G4WMFRQ6G4pAgkH6CgqVDNpaCgoFCWn5mi4KsB44KAc35ecX5Oql54UWZJqk9mXqoGSL01WL6Wq5YLzVQnLKYWFGWWJZakKhSlJqbk5-VUwlTkxhco2CoEWHORYDdID8J2L6bk4ihOji3zu25fTBdgSWCsYAQA

@omsmith omsmith Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you want to do this as a first pass though, 👍

@j3parker j3parker Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure I agree with

Really we just want the Roslyn team's RS0062, or to add an equivalent.

In that I think this analyzer should stand on it's own (if that counts as "adding an equivalent" then OK).

As written this will prevent okay usages of primary class constructors for immutable types, so I do agree that this is a "first pass" in that sense (I wasn't planning to do any more of it, though -- this was motivated by the thread in Slack and existing (probably benign) usages that have snuck through).

@j3parker j3parker Jun 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess this is probably 99% agreement, I'll chat with you on Slack about next steps


return true;
}
}

return false;
}

/// <remarks>
/// Check that a member (e.g. field or property) always produces immutable
/// values.
Expand Down
10 changes: 10 additions & 0 deletions tests/D2L.CodeStyle.Analyzers.Test/Specs/ImmutabilityAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1461,4 +1461,14 @@ static ClassWithCapturedMutability() {
SometimesBad2 = static () => 2;
}
}

// This one doesn't create a diagnostic because there are no parameters.
[Immutable]
class ClassWithTrivialPrimaryConstructor();

[Immutable]
class ClassWithPrimaryConstructor /* PrimaryClassConstructorIntroducesMutability */ ( int X ) /**/;

[Immutable]
record class RecordClassCanHavePrimaryConstructor(int X);
}
Loading