Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Aug 27, 2020

Backport of #41382 to release/5.0

/cc @carlossanlop @krwq

Customer Impact

This is a potential regression fix with extra null check. Regression is due to the compiler bug related to combining ?. with ! (i.e. foo?.bar!.baz can throw NRE when foo is null as you will get (foo != null ? foo.bar : null).baz instead of foo != null ? foo.bar.baz : null).

Testing

Not obvious how to test this behavior. It also requires Code Contracts to be enabled. Since this is returning to pre-annotations state with extra null check risk of breaking anything is extremely low.

Risk

Low, we return to the previous (3.1) behavior with extra null check.

@carlossanlop
Copy link
Contributor

@jeffhandley @krwq @ericstj PTAL

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks good. Please fill in the PR description sections though.

@ericstj
Copy link
Member

ericstj commented Aug 30, 2020

Did you look for any other cases in our codebase that would be impacted by the compiler bug? Why aren't we fixing the compiler bug instead? cc @jaredpar

@krwq
Copy link
Member

krwq commented Aug 31, 2020

@ericstj yes I did search for this pattern (using Regexes, not Roslyn so there is possibility I missed something). I found total (including this) of 2 instances. Another one is already merged into 5.0 (#41315; port PR: #41339)

@jaredpar
Copy link
Member

Why aren't we fixing the compiler bug instead?

The bug is non-trivial, more like a feature than a bug. It stems from how the ?. operator is parsed and it's impact on the !. operator. The compiler is technically behaving to the specification at this point but we pretty much all agree this is a bug and should be fixed. The fix will be coming but not in time for C# 9 as it's decently involved.

@ericstj
Copy link
Member

ericstj commented Aug 31, 2020

Ok, I approve this fix as it stands and it seems folks have done a reasonable amount of thinking into generalizing the problem. @jaredpar my understanding was this behavior changed in the compiler between releases, so I don't think customers would perceive it as a feature, or even an acceptable bug to postpone. You might want to publish a breaking change notice if you're unable to fix it in time for release.

@ericstj ericstj merged commit 54e597d into release/5.0 Sep 1, 2020
@jkotas jkotas deleted the backport/pr-41382-to-release/5.0 branch September 1, 2020 14:18
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants