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

CreateTupleTypeSymbol shouldn’t reset nullability element types to None if elementNullableAnnotations is a default array. #40348

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

AlekseyTs
Copy link
Contributor

Fixes #40105.

…ne if elementNullableAnnotations is default array.

Fixes dotnet#40105.
@AlekseyTs AlekseyTs requested review from 333fred, cston and jcouv December 13, 2019 00:03
@AlekseyTs AlekseyTs requested a review from a team as a code owner December 13, 2019 00:03
Assert.True(tuple1.Equals(tuple2, TypeCompareKind.ConsiderEverything));

tuple2 = comp.CreateTupleTypeSymbol(elementTypes, elementNullableAnnotations: default);
Assert.True(tuple1.Equals(tuple2, TypeCompareKind.ConsiderEverything));
}
Copy link
Member

@cston cston Dec 13, 2019

Choose a reason for hiding this comment

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

} [](start = 8, length = 1)

Are we testing cases where elementTypes has annotations and so does elementNullableAnnotations? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we testing cases where elementTypes has annotations and so does elementNullableAnnotations?

I believe existing tests below sufficiently cover these cases.


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

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find any test where the annotations on elementTypes are not None and distinct from elementNullableAnnotations.


In reply to: 357436843 [](ancestors = 357436843,357434927)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find any test where the annotations on elementTypes are not None and distinct from elementNullableAnnotations.

This PR is focused on scenarios where elementNullableAnnotations is default.


In reply to: 357776096 [](ancestors = 357776096,357436843,357434927)

@AlekseyTs
Copy link
Contributor Author

@cston Do you have more feedback?

@AlekseyTs
Copy link
Contributor Author

@cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@AlekseyTs AlekseyTs merged commit b1ab981 into dotnet:master Dec 13, 2019
@jcouv jcouv added this to the 16.5.P2 milestone Dec 13, 2019
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.

CreateTupleTypeSymbol requires you to pass the element nullabilities explicitly
5 participants