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.

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

Omsmith/destructured assignments #741

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

Conversation

omsmith
Copy link
Contributor

@omsmith omsmith commented Apr 9, 2021

WIP - opening up to chat

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

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.

return AssignmentInfo.Create(
isInitializer: false,
expression: assignmentSyntax.Right,
assignedType: null
Copy link
Contributor Author

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

Comment on lines 444 to 465
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 ) {
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

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();
}

);
}

if( deconstruction.Method.Parameters.Length != lhsExpressions.Length ) {
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.

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

Copy link
Contributor Author

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.

Comment on lines +555 to +560
diagnostic = Diagnostic.Create(
Diagnostics.NonImmutableTypeHeldByImmutable,
assignment.Expression.GetLocation(),
"blarg", "blarg", "blarg"
);
return AssignmentQueryKind.Hopeless;
Copy link
Contributor Author

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)

@@ -333,6 +333,18 @@ ExpressionSyntax expression
assignedType: assignedType
);
}

public static AssignmentInfo Create(
Copy link
Member

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants