Skip to content

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

Merged
j3parker merged 1 commit into
mainfrom
primary-class-constructor-mutability
Jun 3, 2026
Merged

LFT-2910: Detect mutability from primary class constructors#977
j3parker merged 1 commit into
mainfrom
primary-class-constructor-mutability

Conversation

@j3parker

@j3parker j3parker commented Jun 2, 2026

Copy link
Copy Markdown
Member

No description provided.

@j3parker j3parker changed the title Detect mutability from primary class constructors LFT-2910: Detect mutability from primary class constructors Jun 2, 2026
@j3parker j3parker marked this pull request as ready for review June 2, 2026 21:06
@j3parker j3parker requested a review from omsmith as a code owner June 2, 2026 21:06

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

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.

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.

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

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

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

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).

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

@j3parker j3parker merged commit 832f247 into main Jun 3, 2026
2 checks passed
@j3parker j3parker deleted the primary-class-constructor-mutability branch June 3, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants