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

Handling of null values in computing switch expression exhaustiveness #30597

Closed
gafter opened this issue Oct 18, 2018 · 9 comments
Closed

Handling of null values in computing switch expression exhaustiveness #30597

gafter opened this issue Oct 18, 2018 · 9 comments
Assignees
Labels
Area-Compilers Feature - Pattern Matching Pattern Matching Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Milestone

Comments

@gafter
Copy link
Member

gafter commented Oct 18, 2018

The "nullable reference types" feature tracks when values in executable code likely can or cannot contain null. This would be useful in performing exhaustiveness analysis of a switch expression, because warning about a "possible" unhandled null in a switch expression is sort of pointless if the nullability analysis determines that the value ought not be null.

We'd like to organize the analysis so that exhaustiveness checking is sensitive to the results of the nullable analysis. To do that, the binding phase will assume (for the purposes of exhaustiveness analysis) that no value in a pattern-matching operation can be a reference null. If the nullable feature is enabled, the nullable checked pass will complete the analysis by checking for exhaustiveness in cases where some input value is null.

That means code like this

    int M(string s1, string s2)
    {
        return (s1, s2) switch {
            (string x, string y) => x.Length + y.Length
            };
    }

Will not get a warning that the switch expression is incomplete when the nullable feature is not enabled. When the nullability feature is enabled, the declarations of M's parameters are treated as non-null, and you won't get a warning on the switch expression because the two input values ought not to contain null. You would get a warning if a caller attempts to pass a null value to M though. If you change the code to take advantage of the "nullable reference types" feature

    int M(string? s1, string? s2) // NOTE: added nullability annotations on parameters
    {
        return (s1, s2) switch {
            (string x, string y) => x.Length + y.Length
            };
    }

Then a warning that the switch expression is incomplete will be produced by the nullable analysis pass.

(I don't know precisely when this was discussed in the LDM, but @jcouv, @AlekseyTs and I @gafter remember this being a decision)

@gafter gafter added this to the 16.0 milestone Oct 18, 2018
@gafter gafter self-assigned this Oct 18, 2018
@alrz
Copy link
Member

alrz commented Nov 8, 2018

Does this include the codegen where we check for null?

@gafter
Copy link
Member Author

gafter commented Nov 8, 2018

@alrz This would have no affect on the compile-time or runtime meaning of the code. It only affects warnings.

gafter added a commit to gafter/roslyn that referenced this issue Nov 10, 2018
Addresses part of dotnet#30597
Later, in a separate PR, the null paths will be checked in the nullable walker
gafter pushed a commit that referenced this issue Nov 14, 2018
Addresses part of #30597
Later, in a separate PR, the null paths will be checked in the nullable walker
@gafter
Copy link
Member Author

gafter commented Nov 14, 2018

The binding work was done in #31093. All that remains is the work in the nullable analysis pass.

@alrz
Copy link
Member

alrz commented Dec 21, 2018

From design notes:

Current design: warning about not handling all inputs except null.

I think when this work is done one could use e! switch { .. } to exclude nulls.

@gafter
Copy link
Member Author

gafter commented Dec 21, 2018

@alrz That only affects whether the compiler thinks there could be null at the top level. It still tracks whether members of that top-level value contain null.

@alrz
Copy link
Member

alrz commented Dec 21, 2018

Yes, I'm proposing that it also affect subsumption checking when #nullable is enabled.

 int M(string? s1, string? s2) 
    {
        return (s1!, s2!) switch {  // ignore null on input
            (string x, string y) => x.Length + y.Length 
        }; // no warning on unhandled null because nulls are already ignored.
    }

I think this is better than "warning about not handling all inputs except null" because user opts in.

Furthermore, I think there should an easier way to suppress subsumption warnings because they could happen with other values as well, not just null.

@gafter
Copy link
Member Author

gafter commented Dec 22, 2018

Agreed; the compiler will (but doesn't yet) warn for incompleteness, but where it is incomplete because null isn't handled, only warn if the compiler infers that null is possible through the normal nullable analysis. It will still be possible, of course, to smuggle in a null and get a runtime exception for not handling it.

@alrz
Copy link
Member

alrz commented Dec 22, 2018

It will still be possible, of course, to smuggle in a null and get a runtime exception for not handling it.

This will be expected when you use ! anywhere, but it's visible and "forced" by user, also it only applies to that exact value, so a warning is still possible when null on some other input value is not handled.

@gafter
Copy link
Member Author

gafter commented Dec 22, 2018

@alrz It is also possible reading a not-yet-initialized field, a parameter that an evil caller passed null to, and many other ways.

@gafter gafter modified the milestones: 16.0, 16.1 Feb 26, 2019
gafter added a commit to gafter/roslyn that referenced this issue Mar 19, 2019
gafter pushed a commit to gafter/roslyn that referenced this issue Mar 19, 2019
gafter pushed a commit to gafter/roslyn that referenced this issue Mar 19, 2019
jaredpar pushed a commit that referenced this issue Mar 30, 2019
* Implement pattern-matching in the nullable walker

Fixes #29909
Fixes #31881
Fixes #30952
Fixes #33499
Fixes #30597
Fixes #32414
Fixes #23944

* Remove infinite recursion by using an empty struct cache.

* Changes per code review comments.

* Remove debugging code accidentally left behind.

* Analysis of patterns-matching in the nullable walker requires valid (>0) slots.

* Skip a flaky test

* Patch after merge.

* Make ctor private to force use of factory methods

* Correct a typo.

* Fixup after merge.
@sharwell sharwell added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Mar 30, 2019
@sharwell sharwell modified the milestones: 16.1, 16.1.P2 Mar 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Feature - Pattern Matching Pattern Matching Feature Request Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented
Projects
None yet
Development

No branches or pull requests

3 participants