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

Address some crashes found by fuzz testing. #43280

Merged
merged 6 commits into from
Apr 15, 2020

Conversation

gafter
Copy link
Member

@gafter gafter commented Apr 11, 2020

No description provided.

@gafter gafter added this to the Compiler.Net5 milestone Apr 11, 2020
@gafter gafter requested a review from a team as a code owner April 11, 2020 01:53
@gafter gafter self-assigned this Apr 11, 2020
@gafter
Copy link
Member Author

gafter commented Apr 13, 2020

@agocke @RikkiGibson @dotnet/roslyn-compiler May I please have a couple of reviews of this change to loosen assertions that were too tight (demonstrated by added tests)?

@@ -401,7 +401,7 @@ private BoundPattern BindDiscardPattern(DiscardPatternSyntax node, TypeSymbol in
diagnostics.Add(ErrorCode.ERR_PointerTypeInPatternMatching, typeSyntax.Location);
return true;
}
else if (patternType.IsNullableType() && patternTypeWasInSource)
else if (patternType.IsNullableType() || typeSyntax is NullableTypeSyntax && patternTypeWasInSource)
Copy link
Member

@cston cston Apr 13, 2020

Choose a reason for hiding this comment

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

patternType.IsNullableType() || typeSyntax is NullableTypeSyntax [](start = 21, length = 64)

Should this expression be parenthesized? (Should patternTypeWasInSource apply regardless?) #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I will remove patternTypeWasInSource. It is always true.


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

PatternMatchingFuzz(i + k);
}
}));
Task.WaitAll(tasks.ToArray());
Copy link
Member

@cston cston Apr 13, 2020

Choose a reason for hiding this comment

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

Task.WaitAll(tasks.ToArray()) [](start = 12, length = 29)

Is it important that the tests run in parallel rather than sequentially? It's not clear where in PatternMatchingFuzz() there is shared state. If it's not important, consider making this sequential or include a comment. #Resolved

Copy link
Member Author

@gafter gafter Apr 13, 2020

Choose a reason for hiding this comment

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

I have a 32-thread processor on my desk. This is just a way of getting much more testing done per unit time. It is otherwise the same as the adjacent sequential version. Both are skipped. #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 2)

@gafter gafter requested a review from a team as a code owner April 15, 2020 00:44
@gafter gafter merged commit 6582fb0 into dotnet:features/patterns3 Apr 15, 2020
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.

3 participants