Skip to content

This fixes a crash while typing a positional pattern.#26481

Merged
gafter merged 3 commits intodotnet:features/recursive-patternsfrom
gafter:rpatterns-typingcrash
Apr 29, 2018
Merged

This fixes a crash while typing a positional pattern.#26481
gafter merged 3 commits intodotnet:features/recursive-patternsfrom
gafter:rpatterns-typingcrash

Conversation

@gafter
Copy link
Copy Markdown
Member

@gafter gafter commented Apr 28, 2018

This fixes a crash found while typing in the IDE. It is a tiny bug fix.
Fixes #26467

@jcouv @agocke @cston @dotnet/roslyn-compiler May I please have a couple of reviews?

@gafter gafter added this to the 16.0 milestone Apr 28, 2018
@gafter gafter self-assigned this Apr 28, 2018
@gafter gafter requested review from agocke, cston and jcouv April 28, 2018 18:20
@gafter gafter requested a review from a team as a code owner April 28, 2018 18:20
BoundExpression deconstruct = MakeDeconstructInvocationExpression(
node.SubPatterns.Count, inputPlaceholder, node, diagnostics, outPlaceholders: out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders);
deconstructMethod = deconstruct.ExpressionSymbol as MethodSymbol;
if (deconstructMethod == null)
Copy link
Copy Markdown
Member

@jcouv jcouv Apr 28, 2018

Choose a reason for hiding this comment

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

deconstructMethod is a MethodSymbol. Use is null or (object). #Resolved

Copy link
Copy Markdown
Member

@agocke agocke Apr 29, 2018

Choose a reason for hiding this comment

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

Below you check outPlaceholders.IsDefaultOrEmpty to see if the deconstruction is an error, while here you check if the method is null. It seems like this code could be simplified significantly by settling on a single check and reusing it, including where we check isError below. #ByDesign

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.

That is a test for a particular subelement of the deconstruction being an error, not the whole thing. Checking that the method is null is one way the whole thing can be in error.


In reply to: 184870737 [](ancestors = 184870737)

}

[Fact]
public void CrashOnWrongArity()
Copy link
Copy Markdown
Contributor

@cston cston Apr 28, 2018

Choose a reason for hiding this comment

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

Perhaps rename to WrongArity since the test should not crash. And even without the fix, it sounds like the issue was an assert failure rather than a crash. #Resolved

@gafter
Copy link
Copy Markdown
Member Author

gafter commented Apr 29, 2018

Responded to review comments.

@agocke @jcouv May I please have a second review?

@gafter gafter added the 4 - In Review A fix for the issue is submitted for review. label Apr 29, 2018
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks

@gafter gafter merged commit 1d14dbb into dotnet:features/recursive-patterns Apr 29, 2018
@gafter gafter removed the 4 - In Review A fix for the issue is submitted for review. label Apr 29, 2018
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.

4 participants