-
Notifications
You must be signed in to change notification settings - Fork 844
fix 13607 #13648
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
fix 13607 #13648
Conversation
dsyme
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vzarytovskii more ideas for tests noted
| [<InlineData("DateTime", "DateTime.Now")>] | ||
| [<InlineData("int", "1")>] | ||
| [<InlineData("Guid", "(Guid.NewGuid())")>] | ||
| [<InlineData("Byte", "0x1")>] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Byte-typed literals are 1uy. Curious this compiles and passes. I guess the object is an integer and the first type test fails but the second still succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should test this kind of logic when testing array types. Particularly signed/unsigned integer arrays which have strange rules in .net (int32[] can runtime-coerce to unit32[]) . Likewise arrays of enums. Also covariant coercions for reference array types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Byte-typed literals are 1uy. Curious this compiles and passes. I guess the object is an integer and the first type test fails but the second still succeeds.
Sigh, yeah, I fixed it locally but didn't push. Well, obj is bad :D
Fixes #13607
The problem is if the type test on the struct succeeds, but the condition fails, then later type tests for related interfaces are assumed to fail.
This is actually noted as a case of concern in the comment but there was not a matching test for it, which shows that my testing for this change was inadequate - it should have at least covered every case identified in the comments.
Some baselines may need updating