Skip to content

Instantly share code, notes, and snippets.

@jcouv
Last active August 3, 2017 22:20
Show Gist options
  • Save jcouv/497103ca4f734b6f23284f292289e4ac to your computer and use it in GitHub Desktop.
Save jcouv/497103ca4f734b6f23284f292289e4ac to your computer and use it in GitHub Desktop.
Notes on non-null

Discussion started off from Chuck's PR following recent LDM discussion, and continued with Neal.

The way we initially tried to formalize this was with two or maybe three states (?, !, oblivious), two warning scenarios (dotting and assignment), and three contexts (C# 7.0, C# 8.0, C# 8.0 + non-null flag for additional warnings).

We started with two warnings:

  • assigning a ? type to a ! type is a warning in 8.0
  • dotting into a ? type is a warning in 8.0

Two scenarios cause problems:

[scenario 1]

var s = "";
s = null;
string q = s; // expect no warning (compat)

If s is a ? and q is a !, then this code compiles in 7.0, but breaks in 8.0.

[scenario 2]

var s = c ? "" : null;
Print(s.Length); // expect no warning (compat)

Problems:

  • If the conditional expression is given a ? type, then this code compiles in 7.0, but breaks in 8.0.
  • If the conditional expression is given an oblivious type, then this code will never produce a much needed warning.

Some other scenarios we discussed: [scenario 3]

var s = ""; 
s = null; // Print is external API, declared with Print(string!)
Print(s.F()); // expect no warning (compat)

[scenario 4]

var s = Foo();
if (s == null)
    FailFast(); // throws, but the compiler doesn't know that
Print(s.Length); // unfortunate warning, breaks compat

I think all the designs we discussed produce a false alert on scenario 4 (FailFast method throws, but the compiler doesn't know it).

We fell back onto having three states (?, !, oblivious) instead of just two (?, !).

We added a third warning: 'conversion of null to !'. So string /*!*/ s = null; would warn in 8.0, but that is a compat issue.

We added the better type rule for conditional expression: c ? "" : null is gets a ?. The "" is a !, there is a 'null conversion to ?' on the null. So this expression does not warn.

The problem is that string s /*!*/ = c ? "" : null; would warn on the assignment in 8.0 (compat break).

So we think that all three warnings must be disabled for 8.0, and only enabled when you set the non-null warnings on.

This leaves us with three warnings (only when non-null flag is set):

  • warn when dotting on ?
  • warn on 'null conversion to !'
  • warn when assigning from ? (determined by flow analysis) to ! (from symbol)

The information from symbol tells you "what you can put into it". The information from flow analysis tells you "what you get out" (for local variables). For fields there is an open question.

Assign to (from symbol) \ from (from flow analysis) ! ? oblivious
! warn
?
oblivious

Options

Option 0 (Mads)

  • string => !
  • string? => ?
  • string! => disallowed
  • var (local) => inferred from RHS

This option strongly relies on the import setting per library (see details below). Somehow the warnings are not produced for types from opted-out libraries.

The upside is that there are only two syntax forms and two types (simpler). The downsides are that:

  • it requires an out-of-band mechanism for library authors to coordinate with clients.
  • migration process: all-or-nothing
  • difficulty of analysis for types from opted-out libraries
  • tooling issues for storing and managing opted-out references: command-line, IDE, project file, etc

Option 1

For both fields and locals:

  • [URTANN] string => !, and [URTANN] applies to method bodies
  • [URTANN(false)] string => oblivious
  • string? => ?
  • string => oblivious
  • var (local) => ?
  • string! => disallowed

The downsides are that:

  • migration requires more code change: once you try to migrate an API, the body will requires a lot of ? sprinkled in
  • you don't get the benefits of flow analysis until you make such code change
  • the meaning of "string" depends on URTANN above

Option 2 (Neal)

Same as option 1, but with a difference for locals ([URTANN] does not apply to method bodies):

  • [URTANN] string => !
  • [URTANN(false)] string => oblivious
  • string? field => ?
  • string field => oblivious
  • string? local => ?
  • string local => kinda oblivious (flow analysis gives you the type at the use-site of the local)
  • var (local) => ?
  • string! => disallowed

The upsides are that:

  • you get helpful warnings in method bodies.
  • the migration story is incremental:
    1. turn on non-null flag: you should get very few warnings at this stage
    2. gradually add URTANN to APIs and fix clients (rarely needed)
    3. repeat and move URTANN up until top-level
    4. done

The downsides are that

  • "string" doesn't mean the same in APIs and in methods (because URTANN does not apply inside method bodies).
  • you cannot get some benefits of ! in method bodies.

Option 3

Same as option 2, but URTANN applies to method bodies.

URTANN dictates how "string" is interpreted in a method.

The upsides are that you get helpful warnings in method bodies, and "string" means the same in APIs and in methods. The downside is that the migration becomes more difficult (you need to add ? on locals).

Option 4 (Julien)

Another way to keep "string" to have the same meaning between APIs and locals, is for APIs to align with the rules for locals.

  • No URTANN
  • string => oblivious (for field), and kinda oblivious for locals (we let flow analysis warn)
  • string! => !
  • string? => ?
  • var (local) => ?

The upside of this option is that "string" has the same meaning everywhere (in field and in local). The downside is that we introduce a syntax for ! and APIs will be littered with it.

Importing libraries

There is also an orthogonal discussion on what to do with types from libraries. It can be bolted onto the various proposals.

There is a proposal to import a library with some opt-out, which means that every type is treated as oblivious.

legacy lib modern lib
opt-out oblivious oblivious
no opt-out oblivious ! and ?

Open issues

Open issues we mentioned but didn't discuss in details:

  • TryGet API
Map<int, string /*!*/> map = ...
map.TryGet(...);
  • FirstOrDefault API
  • default(T)
  • overrides
  • what flow analysis does for fields?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment