Fix null check in S.Diagnostics.Contract regressed by nullability annotations #41382
Conversation
|
@danmosemsft this one has a bit smaller scope I think but it does affect Code Contracts (is anyone even using it still?) |
krwq
force-pushed
the
nullability-compiler-bug-regression-contracts
branch
from
5fa6bde to
2482f61
Compare
| @@ -628,7 +628,8 @@ private static void AssertMustUseRewriter(ContractFailureKind kind, string contr | |||
| Assembly? probablyNotRewritten = null; | |||
| for (int i = 0; i < stack.FrameCount; i++) | |||
| { | |||
| Assembly? caller = stack.GetFrame(i)!.GetMethod()?.DeclaringType!.Assembly; | |||
| MethodBase stackMethod = stack.GetFrame(i)!.GetMethod(); | |||
| Assembly? caller = (stackMethod != null) ? stackMethod.DeclaringType!.Assembly : null; | |||
There was a problem hiding this comment.
DeclaringType can be null. I do not think there is anything guaranteeing that it will be non-null here. This is one of those cases nullable annotations caught a real bug.
Should the fix be to just change ! after DeclaringType to ?? No need to split it into multiple lines.
There was a problem hiding this comment.
We try to avoid any product changes during nullability annotations as those are always risky and can get lost in a large PR. In this case since PR is only affecting this specific change so I think we should fix it.
What are the tests that caught this? |
|
/backport to release/5.0 |
|
Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/227480299 |
ghost
locked as resolved and limited conversation to collaborators
While annotating #41261 I've introduced a product bug which luckily got caught by tests. It is weird compiler behavior (dotnet/csharplang#3393) which changes
?.precedence when adding!, see i.e.:https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABATARgFgAoEvAZgAIBvEy+y7fANkYBZKAFCGAC3wAUAYXyUADgEoadBrIBuEBJQB2EMHEoBecQH4AdENx6AcmrgBuGbPpMAnANXqJl4rIC+Vyp6atsHbny4wqKS0q7W9ApKjhraYvqGAIQmZi4RNvj2Mc6eHsR5ZLiUIiS04TZUhsVF1JQA5nAw5pRQjc15BcR41aXeVEwADJSm6jT1bS1tHUA
Note: this is fixing a regression introduced by a8b7c22#diff-918e20adb7bd65c2e6e73caa106465d6R658