Added: C# Language Design Notes for Aug 7, 2017

[MadsTorgersen]

C# Language Design Notes for Aug 7, 2017

We continued refining the nullable reference types feature set with the aim of producing a public prototype for experimentation and learning.

1. Warnings
2. Local variables revisited
3. Opt-in mechanisms

Please discuss!

[CyrusNajmabadi]

@jcouv and I discussed ideas around a migration strategy if we take the "one big switch" approach (though the idea may also apply to any of the other opt in approaches). The idea was based on the observation that when you turn on the switch you don't really get warnings for:

1. Direct dereference of nullable values
2. Indirect dereference of nullable values - conversion to not-nullable

You won't really get warnings here** because you're not using nullable values (and all unannotated references will be non-null), so there won't be a place** for nulls to enter into the system. The one place you would get warnings would be through:

3. Not-nullable is non-null - Conversion of null value to not-nullable type

In a way, this makes sense. Once you turn on this feature, the usage of 'null' is precisely the point at which null 'flows in' to your system and which you'll be warned about.

The potential downside of this 'one big switch' approach is that then all your nulls end up warning you, and there may literally be thousands of these cases.

To help with that, we think we could supply a pair of fixers to ease migration:

Fixer#1 would go find all the warnings due to using null, and would change them to null! (or whatever syntax we choose for the 'damnit, let me just use null right now, even if you think it's bad' feature). This would immediately nip these warnings in the bud and would get you back to a clean slate**. At this point you could now examine the null!s one by one deciding what to do. For example, if you had string s = null!; you might decide to replace null! with a literal string. or you might make that into string? s = null;.

Fixer#2 would be a warning we would give you if you were using null! unnecessarily. i.e. shutting up the null warning in a place where you would not get a warning. Why would that be helpful? Well consider the following code scattered around your project:

Foo(null);
...
Foo(null);
...
void Foo(string s) { ... }

Initially, thanks to fixer#1 we would convert that into:

Foo(null!);
...
Foo(null!);
...
void Foo(string s) { ... }

Now, say the user goes and wants to actually address the usage of the null!. In this case they see the first usage and they go and examine the Foo method. They realize that Foo is fine with taking null values, so they change it to void Foo(string? s). At this point, they start getting warnings that Foo(null!) is unnecessary. We can provide a fix-all to then clear up all the cases for them, so that all downstream effects of the change are appropriately updated.

--

This approach allows for the user to get themselves into a 'clean slate' state without a deluge of warnings. It also provides them a means to then go investigate and manually update cases one at a time, with fixers to help update affected reference locations quickly and easily.

--

** Ok, i lied a bit. It is true that using null is the primary way that null values flow into the system. However, it's not true that "you don't really get warnings for 'Direct dereference' or 'Indirect dereference'". Consider the following case:

var s = b ? "" : null;
var len = s.Length;

If you turn on the feature, then you will get a warning on "s.Length" (a direct dereference). This is due to the inference of the type "string?" due to the usage of 'null' in the ternary. For Fixer#1 to work, it will need to find all the cases where a nullable type was inferred due to null being used in an expression as well. Basically any place where we might infer a type and might have special null handling rules. These cases would include, but would not necessarily be limited to:

1. Ternary expressions. var s = b ? "" : null
2. Coalescing expressions: var x = b ?? null
3. Array initializers?: var v = new [] { "", null };

etc. etc.

Fortunately, we should be able to enumerate these cases and detect them from a fixer. As before, the fix would be to use null! here instead of null to ensure that only non-nullable types are inferred from expressions like this. This should then ensure that the user is always in a clean-slate vis-a-vis warnings to start working from.

--

Finally, as we discussed in teh meeting with @jcouv and @DustinCampbell , we'll have a fixer that detects usages of nullable types (like string?) and will offer to upgrade the user's language version. When the user invokes this language-version upgrade, could proactively have the upgrade offer to perform fixer#1, with a brief explanation (maybe with a link to a webpage) telling the user the benefits.

As many users may be frightened away from opting in due to the number of warnings, having documentation to help explain more palatable ways to migrate may likely be necessary.

[HaloFour]

@CyrusNajmabadi

There seriously must be a better approach to this than to throw "damnits" literally all over the project. That completely masks the very problem that you're trying to solve.

[CyrusNajmabadi]

@HaloFour I don't see how that's the case. The intent would not be that you would be "done" at this point. The intent is that now you're at a good place to start actually tackling the issue of 'null' in your project. Consider a project the size of Roslyn. If we turn on this feature we will literally have 50 thousand warnings to have to deal with. Attempting to turn on the feature and fix all of these will be insurmountable. Just trying to keep up with the PR as our team continually changes things would be very difficult, and the review itself could take weeks.

However, let's say we could turn on the feature and be in a good clean state. Now, the team can appropriately tackle 'nulls' in any particular area as appropiate when it fit into the owner's particular schedule. For example, i could decide one day to fixup all the code in the tagging subsystem of the IDE. I could simply go to those locations where 'null!' was used and i could process each one at a pace that made sense to me. I wouldn't even have to fix all of them. I'd just fix what i thought made sense for me in the time i had, could send out a small PR, and move on. In this way, organically, over a long period of time, Roslyn itself could totally adopt this feature.

[CyrusNajmabadi]

@HaloFour @Joe4evr please see the part where i mention:

The potential downside of this 'one big switch' approach is that then all your nulls end up warning you, and there may literally be thousands of these cases.

To help with that, we think we could supply a pair of fixers to ease migration:"

The goal here is to make it feasible for people to actually effectively migrate code. And, to that end, people need a way to 'cut off' the feature to make it non viral. An analogy I used when discussing this idea with people is how we were able to introduce 'async/await' such that it did not have to be viral in a codebase. Specifically, someone could async/await'ify some pieces of their code, but then still nip it in the bud at points that made sense to them by using .Wait(). This allowed for teams (including Roslyn itself) to effectively migrate large codebases over a reasonable period of time. If you don't have a mechanism to allow the viral-nature of these types of changes to be stopped, you effectively make the cost of adopting the feature prohibitive for any sort of large codebase.

To that end, i was proposing actual IDE helpers that would both enable you to prevent both the deluge of warnings and the viral problem endemic here, while also giving helpers to then fix up all the fallout once you'd made the appropriate changes to your code.

Through this, you would also get a clear indicator in the code that there was work to be done. null! (final syntax TBD) acts as a marker telling you "right now there is something wonky about this code wrt your desire to have strict null checks." Just as ".Wait()" on async calls served as a wonky-ness marker that we could use in our codebase to help us find and continue to push-out our async-migration.

[HaloFour]

@CyrusNajmabadi

The intent would not be that you would be "done" at this point.

That might not be your intent, a savvy developer of a technologically advanced codebase. But that will be the end result in the vast majority of instances. The problem will have "gone away".

Specifically, someone could async/await'ify some pieces of their code, but then still nip it in the bud at points that made sense to them by using .Wait(). This allowed for teams (including Roslyn itself) to effectively migrate large codebases over a reasonable period of time.

Thank you for both introducing and accidentally encouraging a massive anti-pattern. The number of people I deal with who think that Wait() is the appropriate way of getting the result from a Task outnumbers the people I encounter who understand await easily by an order of magnitude. You illustrate my point well.

[CyrusNajmabadi]

@HaloFour Again, these things are necessary in order to actually make it possible to adopt the feature.

But that will be the end result in the vast majority of instances. The problem will have "gone away".

And the codebase will be no worse than today.

The alternative is to make things painful. In which case people will turn it on, go 'noope', turn if off and never think about it again. For them, "the problem will have 'gone away'" as well. Except now they'll likely never turn it on again.

[CyrusNajmabadi]

Thank you for both introducing and accidentally encouraging a massive anti-pattern.

Again, without this possibility Roslyn itself could not have become async. What you see as a negative was actually necessary in order to adopt the feature that was actually quite valuable and beneficial. We have to be cognizant of the cost and difficulty people have migrating over existing code bases. This is something we hear over and over again, and it's something that has prevented even small introductions of warnings being added to the language. What i'm proposing allows for a way for people to say "yes, i really do want this, but i need to not have to rip the band aid off in one fell swoop. Needing to do that will make it so that i cannot use this feature at all".

I'd rather make it possible for everyone to opt-in, even if it means their code may contain unpalatable constructs for a quite a while, versus making it impossible for many to opt-in ever for their existing codebases.

[HaloFour]

@CyrusNajmabadi

I wholeheartedly agree that enabling this feature (especially via "one big switch") will be a cumbersome event and that there should be ways to mitigate that. Placing 40,000 ! characters (or whatever) throughout the code does not seem like the appropriate way. I can't imagine any team that would accept a PR like that. That's not an iterative or isolated approach.

I'd rather something at the project level. Maybe something to suppress the warnings but still provide the flow analysis and "squigglies" at the file level. Or something like unchecked warnings in Java where you're informed that there are warnings and you have to enable another option to get the actual list of issues. You get the iterative approach to solving the problem without having to make massive changes across the codebase just to shut up single project-level feature that you're trying to enable.

[Joe4evr]

The number of people I deal with who think that Wait() is the appropriate way of getting the result from a Task outnumbers the people I encounter who understand await easily by an order of magnitude.

Or .Result.

I might've said this before, but one thing that maybe could help with this migration story is to only allow the expression null! (final syntax TBD) under a certain condition. Maybe a flag on the "URTANN"-attribute would be something here?

Wouldn't make it completely fool-proof if some kid were to look around the internet, but it would be a step for those big companies you mention: A large project by a professional team (just like Roslyn) would be subject to code review, so once that project got properly migrated, they could set the flag to false and on subsequent reviews confirm that nobody set it to true again. Bonus point: Setting the flag to false and not having migrated all the places would also produce warnings, so you can tell if the null! stepping-stone expression wasn't completely removed from the codebase.

[CyrusNajmabadi]

I can't imagine any team that would accept a PR like that. That's not an iterative or isolated approach.

I don't actually see an issue there. It would be akin to accepting a PR containing a commit that was only the rename of a commonly used type. It would hit a ton of files, but would be completely mechanical and trivial to review.

[CyrusNajmabadi]

Or .Result.

Again, if there is no way to mesh async with existing sync systems, async is dead in the water from day one for any existing codebase. Code-bases simply will not accept any sort of viral feature that cannot be effectively suppressed at a reasonable boundary (or a boundary of the developer's choosing).

You view this as an anti-pattern. I view this as a necessity to get to a minimum viable product.

--

Note: things would certainly be different if this was a V1 language. You could have no-block-async, and true-non-nullable from the start, if you had a V1 language. But we don't. We're going to be on version 8, and we have to deal with the problems inherent in migration and the needs of people who want to be able to adopt on a large scale.

[CyrusNajmabadi]

I'd rather something at the project level. Maybe something to suppress the warnings but still provide the flow analysis and "squigglies" at the file level.

As i said multiple times, i was just using null! as an example of what could be done, but not the actual thing that would be done. I even said:

or whatever syntax we choose for the 'damnit, let me just use null right now, even if you think it's bad' feature

I would certainly be ok with any approach that suppressed the warnings effectively, but allowed users to go in and incrementally update things.

As these would be warnings, it would of course be possible to even use existing suppression mechanism (like #pragma warning disable, potentially applied at a file level).

Please do not focus on the "you're going to put 50k !'s everywhere". The crux of my post was that we could provide suitable mechanisms to allow one to opt in, while not being flooded with tens of thousands of warnings. We'd also want a way for them to remove those mechanisms once they became unnecessary as they started migrating hteir code step by step. It actually sounds like you're behind such an idea, so i'm not sure what is even worth arguing about.

[HaloFour]

@CyrusNajmabadi

You view this as an anti-pattern. I view this as a necessity to get to a minimum viable product.

The pattern remains long after the concerns of migration. I routinely catch developers trying to use Wait() or Result within async methods. I caught one yesterday.

Please do not focus on the "you're going to put 50k !'s everywhere". The crux of my post was that we could provide suitable mechanisms to allow one to opt in, while not being flooded with tens of thousands of warnings.

Ok, we're certainly in the same camp when it comes to the nature of the problem and that there needs to be some method to mitigate the "wall of warnings" to enable an iterative approach. We can take a step back here and focus on that instead.

[CyrusNajmabadi]

The pattern remains long after the concerns of migration

Again, that's better than not being able to ship async. :)
I view the potential for people to not do the best thing possible as far better than situation where they can't do anything at all.

We can take a step back here and focus on that instead.

Great.

[qrli]

Can we try to make the Fixer smarter so that it does flow analysis to infer nullables, string? for example, and convert declarations to type? in a conservative way? That should reduce a lot of the warnings and reduce a lot of manual work.

Specifically, if flow analysis are confident a method parameter is safe to be null, fix its type to type?; if flow analysis is confident the return value may be null, fix its type to type?.

[HaloFour]

@CyrusNajmabadi

One thing i want to emphasize is that this whole feature space can be thought of as a spectrum between usable and precise.

So maybe rather than a switch this is a dial? Get the flow analysis in there, be as aggressively pessimistic as possible, expose the metadata, ship a reference "stock" analyser and let's hack at it to see what fits. Maybe it can't be one size fits all?

Either way I think it would be massively beneficial for the flow analysis and metadata to always exist. That way people can sic analyzers on their codebase without having to flip something project-wide.

[CyrusNajmabadi]

So maybe rather than a switch this is a dial? Get the flow analysis in there, be as aggressively pessimistic as possible, expose the metadata, ship a reference "stock" analyser and let's hack at it to see what fits. Maybe it can't be one size fits all?

These are exactly the sort of discussions we're trying to have :)

[iam3yal]

BTW, Kotlin has the same issue as F# isn't? you can still have NRE there if say I wrote the library in Scala/Java and used Kotlin to consume it, I haven't used Kotlin yet so just wondering.

[CyrusNajmabadi]

@eyalsk Yes, kotlin has this as well. They even spell these out as examples of null refs that can occur:

1. External Java code has caused it
2. There's some data inconsistency with regard to initialization (an uninitialized this available in a constructor is used somewhere)

This is actually a great sort ot thing to look at as it shows the recognition and pragmatism of recognizing that shutting out all nulls in an existing, established ecosystem is very hard. Kotlin takes an approach that isn't completely safe, but which still provides a ton of value. You are both unsafe against the existing ecosystem not abiding by your annotations (same with our proposal) and you're also unsafe against certain coding constructs (same with our proposal). Being completely safe against both would likely be too onerous or too impactful to perf to be acceptable. They struck a compromise that they likely felt provided the maximum benefit with the least amount of pain (both for using the feature, and for runtime cost).

[qrli]

To me, the most useful thing is only the nullable annotation in member declaration, baked into the language.
Then all the analyzer, fixer, document generator, etc. may come later and evolve along time.

[CyrusNajmabadi]

I think the Warning 3 is wrong maybe? Isn't that warning is for n = null ?

@Thaina No, "n" is declared as "string?", so it's allowed to be null. However "s" is declared as "string" (so it's non-null). So if you assign 'null' into 's' you get a warning.

[Thaina]

@CyrusNajmabadi Thanks I misread it at first

[DavidArno]

@CyrusNajmabadi,

The only proper way to fix the warnings is to analyse the code, understand what it does and adjust it to properly remove those warnings.

That is not true. Again, remember that our analysis is extremely limited in what it can do.

Just to clarify (as I was clearly unclear with my comment here), by "analysis", I mean one or more people sitting down, working through the code to determine what it is doing and correcting it to work with this feature. I wasn't referring to the compiler's flow analysis, which I agree will be extremely limited.

And that "extremely limited" is my issue with the whole idea of flow analysis. Arrays are an obvious case where the flow analysis cannot be trusted as you are preparing the dammit operator as a work around for those before the feature gets going.

With your example,

void Foo(string[] nonNullValues)
{
}

I would argue that the best (though possible naive, see below) way to handle arrays is to not allow arrays of non-nullable reference types. So the compiler would complain about the above and insist it were changed to string?[].

Of course generics screws this up. If we swap string for T, then the compiler cannot enforce this restriction any more, unless T? were used as a pseudo generic constraint which then required the compiler to only allow value types and nullable reference types for T?:

void Foo<T?>(T value) ...

int a = 1;
string b = "";
string? c = "";

Foo(a); // ok
Foo(b); // warning
Foo(c); // ok

So do you want an approach where 95% of the libraries can be ported over easily, even if hte author had to go put a dozen or so 'damnits' in their code? Or do you want only 5% ported over because the authors say "it just isn't worth my time to have to do all the work to get the compiler to be ok with code that i am certain is safe"?

I want the latter. With your proposed fix,

string Foo() => null;

would become:

string Foo() => null!;

The library author opts in, applies your "easy fix" and ships. How can I trust an opted-in library that may contain code like that? I'd far prefer conversion require manual effort and that it remain a niche feature for a couple of years, than it quickly gain the reputation as a feature that cannot be trusted.

[CyrusNajmabadi]

I would argue that the best (though possible naive, see below) way to handle arrays is to not allow arrays of non-nullable reference types.

That seems like it would make for a pretty terrible experience :(

Say i actually create an array, and fill it completely. Now i have no way to express that i've done that. And that means from that point on, the compiler is telling me at every location in my code where i use that array that now i have to be adding null checks in. That's precisely the type of "bad warning" that i want to avoid and that will just end up frustrating most people. If you can't express things that are true about your own code then this feature ends up just being a PITA nuisance.

[CyrusNajmabadi]

I want the latter. With your proposed fix ...
The library author opts in, applies your "easy fix" and ships.

I can't imagine a scenario where that would actually happen. If anything, even if it mistakingly happened, it would be fixed immediately the moment someone actually tried to use such an API.

As i mentioned before, i believe we exist in an ecosystem where people are not working to actually screw you over. Not only is that due to actual observation around how most libraries work, but it's also because we cannot effectively supply a feature that could possibly defend you against this.

See the kotlin example above. Even they don't bother trying to make you safe from this sort of thing. F# doesn't bother trying to make you safe from this.

[CyrusNajmabadi]

than it quickly gain the reputation as a feature that cannot be trusted.

"trust" is invariably subjective. As mentioned already, other ecosystems out there have adopted these styles of null checking without trust problems. TypeScript has done this without having a trust problem. F# has done this. etc. etc. None of these systems are fully trustworthy. However, what they are is trustworthy enough, and that's precisely due to the ecosystem not being full of people who are actively trying to subvert these systems.

The existence proofs are there. The concern you have isn't an issue in practice, and we find that enough of the developer ecosystem does a proper job to warrant these sorts of approaches.

[DavidArno]

@CyrusNajmabadi,

You may well be right that dammit won't be the problem I fear it will. I've been wrong before, eg allowing variable scope leakage from if's hasn't proved the problem I feared it would. So I may well be being paranoid here too. Time will tell...

[iam3yal]

@CyrusNajmabadi

TypeScript has done this without having a trust problem.

The way I see it is it really make sense for TypeScript because part of their design is to have an unsound type system whereas C# has a sound type system which I wouldn't say doesn't make sense but if we take the spectrum example you gave above, it's certainly a shift in thinking for me and for many others here because we're used to think about our code through the type system and be exact about it where with this feature you explicitly say what you want but you can no longer "count on it", it looks like a contract from usability point of view but it really isn't. :)

[jrmoreno1]

As someone who has been moving to option strict on for a couple of VB.net projects for the last year, I would like to encourage both a big flip (project level), and a small flip (file level), as well as fixers. Half of the changes I have to make is adding ToString to ints, which could easily be fixed by a tool. Depending upon the fixer, I might want it to be either on a file or project level (optionally both?).

[Pzixel]

@DavidArno

You may well be right that dammit won't be the problem I fear it will

Don't be affraid, it won't! be a problem :)