Fix false assumption in Expression.TypeEqual #6273
Conversation
|
I confirmed with a test in System.Dynamic.Runtime based on #6258 which went into an infinite loop that this fixed that loop, but can't add it here as it will fail in CI until the packages are updated with the change this makes. |
Can you add it but with an [ActiveIssue] flagging the original issue? |
|
cc: @VSadov, @gregg-miskelly |
|
(You added a test based on .NET 4.6 to the issue... could that be added here as well?) |
|
Good idea. Especially since that test will fail prior to this too, just in a different way. |
JonHanna
force-pushed
the
fix_6258
branch
2 times, most recently
from
6820e21 to
864c74e
Compare
|
There. The thought "can't commit those tests" led to me not committing any at all, when those in expressions should obviously be there, and as you say we can remove the |
|
@stephentoub is there any case in which the ActiveIssue tests are run? Concerned that these hang rather than fail. |
In the CI system, no. They can be run explicitly when invoking xunit from the command line, but the CI system always uses |
| private static int Foo(string s, dynamic d, object o) | ||
| { | ||
| return s?.Length + d?.ToString()?.Length + o?.ToString()?.Length; | ||
| } |
There was a problem hiding this comment.
C#6 ,
@stephentoub - is it now ok to use yet?
I was holding off using these. Tantalizing though...
There was a problem hiding this comment.
It is as of #6181,and indeed there's nameof all over the place now. This test is my little act of celebration.
There was a problem hiding this comment.
I'm confused about what is this trying to accomplish. If any of the parameters is null, you'll get a RuntimeBindingException about not being able to convert null to int. I get that the parameters are never null, so it doesn't really matter, but in that case, what is the point of using ?. in the first place?
There was a problem hiding this comment.
@svick yeah, while it's just do-something-here-don't-care-what test code, there's a limit on how silly even code like that should be. Changed
|
There is a minor concern about compat since old code could give false positives and we are fixing that. |
|
LGTM. |
|
@gregg-miskelly, would you be able to build this branch and check if it does indeed fix your issue, rather than the reproduction only reproducing something similar but separate? |
ReduceTypeEqual has a false assumption that if obj is T and T.IsSealed then typeof(obj) == typeof(T). It will reduce to Expression.Constant(true) based on this assumption, and as of d5e3d82 will reduce to Expression.Constant(false) based on it too. Remove the assumption entirely, removing both false positives and false negatives. Fixes #6258
|
Test Innerloop Ubuntu Release Build and Test |
|
Test Innerloop Ubuntu Release Build and Test (WS-CLEANUP again) |
|
This talk of C# 6 has made something occur to me, viz that the operation of this whole method could be simpler if it just (perhaps after reducing the value type cases only) compiled TypeEqual as C# 6 would be compiled for |
Fix false assumption in Expression.TypeEqual
These tests had to wait for the fix in dotnet#6273 to be available in the feeds before the [ActiveIssue] attribute could be removed. Do this now.
Fix false assumption in Expression.TypeEqual Commit migrated from dotnet/corefx@f139656
These tests had to wait for the fix in dotnet/corefx#6273 to be available in the feeds before the [ActiveIssue] attribute could be removed. Do this now. Commit migrated from dotnet/corefx@0e4f668
6 participants
ReduceTypeEqualhas a false assumption that ifobj is Tandtypeof(T).IsSealedthenobj.GetType() == typeof(T).It will reduce to
Expression.Constant(true)based on this assumption, and as of d5e3d82 will reduce toExpression.Constant(false)based on it too.Remove the assumption entirely, removing both false positives and false negatives.
Fixes #6258