Skip to content

Omsmith/destructured assignments #741

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

Open
omsmith wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Omsmith/destructured assignments #741

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d00a59f
refactor AssignmentInfo linq expression to support extra logic before…
omsmith Apr 9, 2021
d78e2ba
handle deconstructed assignments
omsmith Apr 9, 2021
1bfe273
commentary
omsmith Apr 12, 2021
330a3a6
fixup tests some. test some nesting
omsmith Apr 12, 2021
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
114 changes: 100 additions & 14 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 @@ -333,6 +333,18 @@ ExpressionSyntax expression
assignedType: assignedType
);
}

public static AssignmentInfo Create(
Copy link
Member

@j3parker j3parker Apr 9, 2021

Choose a reason for hiding this comment

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

Could the constructor just be public?

Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

Yeah for sho. Discovering the end solution organically.

bool isInitializer,
ExpressionSyntax expression,
ITypeSymbol assignedType
) {
return new AssignmentInfo(
isInitializer: isInitializer,
expression: expression,
assignedType: assignedType
);
}
}


Expand All @@ -357,13 +369,9 @@ ExpressionSyntax initializer
.Select( sr => sr.GetSyntax() )
.SelectMany( constructorSyntax => constructorSyntax.DescendantNodes() )
.OfType<AssignmentExpressionSyntax>()
.Where( assignmentSyntax => IsAnAssignmentTo( assignmentSyntax, memberSymbol ) )
.Select( assignmentSyntax => assignmentSyntax.Right )
.Select( expr => AssignmentInfo.Create(
model: m_compilation.GetSemanticModel( expr.SyntaxTree ),
isInitializer: false,
expression: expr
) );
.Select( assignmentSyntax => GetAssignmentInfoIfToSymbolOrNull( assignmentSyntax, memberSymbol ) )
.Where( info => info.HasValue )
.Select( info => info.Value );

if ( initializer != null ) {
assignmentExpressions = assignmentExpressions.Append(
Expand All @@ -378,19 +386,86 @@ ExpressionSyntax initializer
return assignmentExpressions;
}

private bool IsAnAssignmentTo(
private AssignmentInfo? GetAssignmentInfoIfToSymbolOrNull(
AssignmentExpressionSyntax assignmentSyntax,
ISymbol memberSymbol
) {
var semanticModel = m_compilation.GetSemanticModel( assignmentSyntax.SyntaxTree );

var leftSideSymbol = semanticModel.GetSymbolInfo( assignmentSyntax.Left )
.Symbol;
var lhsExpressions = assignmentSyntax.Left switch {
TupleExpressionSyntax tuple => tuple.Arguments.Select( arg => arg.Expression ).ToImmutableArray(),
_ => ImmutableArray.Create( assignmentSyntax.Left )
};

for( var i = 0; i < lhsExpressions.Length; ++i ) {
var lhs = lhsExpressions[ i ];

return SymbolEqualityComparer.Default.Equals(
memberSymbol,
leftSideSymbol
);
var leftSideSymbol = semanticModel.GetSymbolInfo( lhs )
.Symbol;

if( !SymbolEqualityComparer.Default.Equals(
memberSymbol,
leftSideSymbol
) ) {
continue;
}

if( lhsExpressions.Length == 1 ) {
return AssignmentInfo.Create(
model: semanticModel,
isInitializer: false,
expression: assignmentSyntax.Right
);
}

if( semanticModel.GetTypeInfo( assignmentSyntax.Right ).Type is INamedTypeSymbol assignedType
&& assignedType.IsTupleType
) {
Comment on lines +424 to +426
Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

Handle Value Tuples.

GetDeconstructionInfo does return stuff for them, but they don't have a Method, just some nested deconstructioninfos with Identity conversions - so need to extract the type info separately anyway.

if( assignedType.TypeArguments.Length != lhsExpressions.Length ) {
return AssignmentInfo.Create(
isInitializer: false,
expression: assignmentSyntax.Right,
assignedType: null
Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

assignedType: null

This is an "error / unknown assignment" - there's probably a better way to signal this...

);
}

ExpressionSyntax rhs = assignmentSyntax.Right switch {
TupleExpressionSyntax tuple => tuple.Arguments[ i ].Expression,
_ => assignmentSyntax.Right
};

return AssignmentInfo.Create(
isInitializer: false,
expression: rhs,
assignedType: assignedType.TypeArguments[ i ]
);
}

DeconstructionInfo deconstruction = semanticModel.GetDeconstructionInfo( assignmentSyntax );
if( deconstruction.Method == null ) {
return AssignmentInfo.Create(
isInitializer: false,
expression: assignmentSyntax.Right,
assignedType: null
);
}

if( deconstruction.Method.Parameters.Length != lhsExpressions.Length ) {
Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Don't believe this can happen, just a sanity check.

Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

Actually I lied there, this is what's filtering out nested deconstruction.

I don't think it would be terribly hard to support, I just don't care to write the code.

return AssignmentInfo.Create(
isInitializer: false,
expression: assignmentSyntax.Right,
assignedType: null
);
}

return AssignmentInfo.Create(
isInitializer: false,
expression: assignmentSyntax.Right,
assignedType: deconstruction.Method.Parameters[ i ].Type
);
Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

Only handles simple deconstructions, not nested ones like so:

class A {
  public void Deconstruct( out string X, out B foo );
}

class B {
  public void Deconstruct( out string Y, out string Z );
}

public void Foo() {
  var (x, y, z) = new A();
}

}

return null;
}

private enum AssignmentQueryKind {
Expand Down Expand Up @@ -474,6 +549,17 @@ .Symbol is not IMethodSymbol methodSymbol
break;
}

if( assignment.AssignedType == null
|| assignment.AssignedType.TypeKind == TypeKind.Error
) {
diagnostic = Diagnostic.Create(
Diagnostics.NonImmutableTypeHeldByImmutable,
assignment.Expression.GetLocation(),
"blarg", "blarg", "blarg"
);
return AssignmentQueryKind.Hopeless;
Comment on lines +581 to +586
Copy link
Contributor Author

@omsmith omsmith Apr 9, 2021

Choose a reason for hiding this comment

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

Need to add some sort of new "Unexpected" diagnostic. (We currently have UnexpectedTypeKind and UnexpectedMemberKind)

}

// If nothing above was caught, then fallback to querying.

if( assignment.Expression is BaseObjectCreationExpressionSyntax _ ) {
Expand Down
61 changes: 59 additions & 2 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 @@ -122,6 +122,63 @@ public SomeClassWithConstructor8() {
(m_interface) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/;
}
}

[Immutable]
public sealed class SomeClassWithConstructor9 {
public readonly RegularInterface m_interface = new Good();
public readonly RegularInterface m_interface2 = new Good();

public SomeClassWithConstructor9() {
(m_interface, m_interface2) = (
/* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, (or [ImmutableBaseClass])) */ new Bad() /**/,
new Good();
);
}
}

[Immutable]
public sealed class SomeClassWithConstructor10 {
public readonly RegularInterface m_interface = new Good();
public readonly RegularInterface m_interface2 = new Good();

public SomeClassWithConstructor10() {
(m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/;
}

public static (Bad, Good) GetValues() => default;
}

[Immutable]
public sealed class SomeClassWithConstructor11 {
public readonly RegularInterface m_interface = new Good();
public readonly RegularInterface m_interface2 = new Good();

public SomeClassWithConstructor11() {
(m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(class, SpecTests.Types.Bad, ) */ GetValues() /**/;
}

public static DeconstructingClass GetValues() => default;

private sealed class DeconstructingClass {
public void Deconstruct( out Bad bad, out Good good ) => (bad, good) = (default, default);
}
}

[Immutable]
public sealed class SomeClassWithConstructor12 {
public readonly RegularInterface m_interface = new Good();
public readonly RegularInterface m_interface2 = new Good();

public SomeClassWithConstructor12() {
(m_interface, m_interface2) = /* NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) | NonImmutableTypeHeldByImmutable(blarg, blarg, blarg) */ GetValues() /**/;
}

public static DeconstructingClass GetValues() => default;

private sealed class DeconstructingClass {
public void Deconstruct( out Good A, out Good B, out Good C ) => (A, B, C) = (default, default, default);
}
}
#endregion


Expand Down Expand Up @@ -1199,10 +1256,10 @@ public record SomeRecord {

int /* MemberIsNotReadOnly(Property, Y, SomeRecord) */ Y /**/ { get; set; }

/* NonImmutableTypeHeldByImmutable(class, object, ) */ object /**/ Z { get; }
object Z { get; }

public SomeRecord( SomeRecord v, int w, Types.SomeImmutableClass x, int y, object z )
=> (V, W, X, Y, Z) = (v, w, x, y, z);
=> (V, W, X, Y, Z) = (v, w, x, y, /* NonImmutableTypeHeldByImmutable(class, object, ) */ z /**/);
}

record NonImmutableBaseRecord(object x);
Expand Down