Nullable API Additions #28860

333fred · 2018-07-26T19:53:32Z

Contains the additions we are considering to add to the symbol API to expose nullability. This PR only has API additions, no implementation. It is expected to fail CI. @jcouv, @cston, @gafter, please review to make sure this is in line with what you were expecting.

jcouv · 2018-07-26T19:56:17Z

src/Compilers/Core/Portable/Symbols/IDiscardSymbol.cs

@@ -14,5 +14,7 @@ public interface IDiscardSymbol : ISymbol
        /// The type of the discarded value.
        /// </summary>
        ITypeSymbol Type { get; }
+
+        Nullability TypeNullability { get; }


TypeNullability [](start = 20, length = 15)

Not sure if we need this one. Maybe just for regularity... #Closed

I debated on this one for a minute as well, but ultimately decided to add it for both the regularity, and because it will likely be needed by QuickInfo as it will still have to show a type for a discard node.

In reply to: 205583015 [](ancestors = 205583015)

Drop "Type" from its name. #Pending

cston · 2018-07-26T19:58:37Z

src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs

@@ -65,13 +65,17 @@ public interface INamedTypeSymbol : ITypeSymbol
        /// </summary>
        ImmutableArray<ITypeParameterSymbol> TypeParameters { get; }

+        ImmutableArray<Nullability> TypeParameterNullability { get; }


Type parameters do not have nullability. #Pending

jcouv · 2018-07-26T20:01:25Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        CanBeNull,
+        Unknown
+    }
+}


Let's discuss with Chuck. From our discussion yesterday, we thought maybe the public API should expose IsAnnotated (? or blank) plus NonNullTypes context as primitives rather than IsNullable (tri-state).

Also, you may want to take a look into the public APIs related to conversions. Maybe they also need to be extended. #Pending

Also, FWIW, the scenarios we could think of so far, for an analyzer writer:

should I add a ? to this type when I generate code? (for example, IntroduceLocal)

could I assign null to this type?

could I get a null when reading from this type?

In reply to: 205584504 [](ancestors = 205584504)

Honestly, I'm not sure what IsAnnotated + NonNullTypes gets public users over a tri-state, and we'd still need this anyway. IsAnnotated might be true even after a null check, but the type should be non-nullable at that point.
Note that, from the name, I've determined that IsAnnotated refers to the declaration of the variable, not current state. If that's not what you meant, then we probably need a better name.

In reply to: 205585491 [](ancestors = 205585491,205584504)

cston · 2018-07-26T20:04:37Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+    public enum Nullability
+    {
+        NonNull,
+        CanBeNull,


The term we've tended to use is MaybeNull. #Pending

cston · 2018-07-26T20:05:13Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+    {
+        NonNull,
+        CanBeNull,
+        Unknown


The enum values should probably be explicit, and it feels like Unknown should be 0. #Pending

gafter · 2018-07-27T17:27:12Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

@@ -17,12 +17,16 @@ public struct TypeInfo : IEquatable<TypeInfo>
        /// </summary>
        public ITypeSymbol Type { get; }

+        public Nullability TypeNullability { get; }


Doc comment on new APIs would be helpful. #Pending

The name "Type" as part of this API probably doesn't make sense. #Pending

I included Type to give the user something to key off of.

In reply to: 205845601 [](ancestors = 205845601)

gafter · 2018-07-27T17:29:16Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

        /// <summary>
        /// The type of the expression after it has undergone an implicit conversion. If the type
        /// did not undergo an implicit conversion, returns the same as Type.
        /// </summary>
        public ITypeSymbol ConvertedType { get; }

+        public Nullability ConvertedTypeNullability { get; }


The only way this could be different from TypeNullability is if it is a user-defined conversion. In all other cases whether the expression is null or not doesn't change. #Pending

I could see calling GetTypeInfo on an expression with a ! expressing the change in nullability via this field.

In reply to: 205845726 [](ancestors = 205845726)

gafter · 2018-07-27T17:30:43Z

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

        /// <summary>
        /// Get the type parameters on this method. If the method has not generic,
        /// returns an empty list.
        /// </summary>
        ImmutableArray<ITypeParameterSymbol> TypeParameters { get; }

+        ImmutableArray<Nullability> TypeParameterNullability { get; }


What does this API mean? I think it can be deleted. #Pending

gafter · 2018-07-27T17:31:47Z

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

@@ -135,6 +141,8 @@ public interface IMethodSymbol : ISymbol
        /// </summary>
        ITypeSymbol ReceiverType { get; }

+        Nullability ReceiverTypeNullability { get; }


A receiver can never be null (except in a reduced extension method). I suggest just removing this API. #Pending

gafter · 2018-07-27T17:32:22Z

src/Compilers/Core/Portable/Symbols/INamedTypeSymbol.cs

@@ -72,6 +72,8 @@ public interface INamedTypeSymbol : ITypeSymbol
        /// </summary>
        ImmutableArray<ITypeSymbol> TypeArguments { get; }

+        ImmutableArray<Nullability> TypeArgumentNullability { get; }


It feels like there should be an "s" somewhere in the name to make it plural. #Pending

I agree with the feeling, but I couldn't find a spot that felt good. TypeArgumentNullabilities is the closes I got, thoughts?

In reply to: 205846588 [](ancestors = 205846588)

Maybe TypeArgumentsNullability? #Pending

Is there any reason we aren't doing some struct that holds a TypeSymbol and it's nullability, and having an ImmutableArray of that struct to hold both at once? My gut right now is saying the IDE is immediately going to create such a thing, and use that wherever we're using ITypeSymbols today. #WontFix

Such a struct/interface to represent the pair (type+nullability) seems appealing, but I don't see how to introduce it without breaking existing API. #WontFix

gafter · 2018-07-27T17:35:14Z

src/Compilers/Core/Portable/Symbols/IDiscardSymbol.cs

@@ -14,5 +14,7 @@ public interface IDiscardSymbol : ISymbol
        /// The type of the discarded value.
        /// </summary>
        ITypeSymbol Type { get; }
+
+        Nullability TypeNullability { get; }


Drop "Type" from its name. #Pending

gafter · 2018-07-27T17:35:28Z

src/Compilers/Core/Portable/Symbols/IEventSymbol.cs

@@ -19,6 +19,8 @@ public interface IEventSymbol : ISymbol
        /// </summary>
        ITypeSymbol Type { get; }

+        Nullability TypeNullability { get; }


Drop "Type" #Pending

gafter · 2018-07-27T17:35:35Z

src/Compilers/Core/Portable/Symbols/IFieldSymbol.cs

@@ -44,6 +44,8 @@ public interface IFieldSymbol : ISymbol
        /// </summary>
        ITypeSymbol Type { get; }

+        Nullability TypeNullability { get; }


Ditto #Pending

gafter · 2018-07-27T17:36:05Z

src/Compilers/Core/Portable/Symbols/ILocalSymbol.cs

@@ -16,6 +16,8 @@ public interface ILocalSymbol : ISymbol
        /// </summary>
        ITypeSymbol Type { get; }

+        Nullability TypeNullability { get; }


This should be named "DeclaredNullability" for clarity. #Pending

gafter · 2018-07-27T17:36:20Z

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

@@ -90,19 +90,25 @@ public interface IMethodSymbol : ISymbol
        /// </summary>
        ITypeSymbol ReturnType { get; }

+        Nullability ReturnTypeNullability { get; }


ReturnNullability #Pending

cston · 2018-08-07T19:08:58Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        Unknown = 0,
+        NonNull = 1,
+        MaybeNull = 2
+    }


We should document this enum, perhaps including examples. For instance, what is the nullability of a reference to an unconstrained type parameter T? What is the nullability of int?? #Pending

…no implementation.

gafter · 2018-09-12T17:14:43Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+    /// o2.ToString(); // Warning reported
+    /// if (o2 != null) o2.ToString(); // No warning reported
+    /// </code>
+    /// <code>o1</code> is inferred to be <see cref="NonNull"/>, despite being declared


o1 is inferred to be [](start = 8, length = 55)

This should be clarified; it is not the variable that is inferred to be NonNull, but the use of the variable in its context. #Pending

gafter · 2018-09-12T17:15:39Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// not provide nullability information is being used. Expressions that
+        /// have unknown nullability are also referred to as oblivious expressions,
+        /// and they generally do not provide warnings when converting to 
+        /// <see langword="null"/>, or when dereferencing them.


I don't know what "converting to null" means. #Pending

gafter · 2018-09-12T17:16:23Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+namespace Microsoft.CodeAnalysis
+{
+    /// <summary>
+    /// Enumeration of the possible nullability states for types.


types [](start = 59, length = 5)

"types" should be "variables and expressions". #Pending

gafter · 2018-09-12T17:16:52Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// </remarks>
+        Unknown = 0,
+        /// <summary>
+        /// The variable is either known or declared to be non-null.


variable [](start = 16, length = 8)

variable or value #Pending

gafter · 2018-09-12T17:17:19Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// </remarks>
+        NonNull = 1,
+        /// <summary>
+        /// The variable is either known or declared to possibly be null.


variable [](start = 16, length = 8)

variable or value #Pending

gafter · 2018-09-12T17:18:15Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// </summary>
+        /// <remarks>
+        /// Nullable value types will always be considered to be <see cref="MaybeNull"/>.
+        /// </remarks>


I don't think this is correct. Didn't we decide to track nullability for nullable value types? If so, they can possibly be non-null. #Resolved

Currently, it does not, and it's relatively unlikely we'll get to this for preview one. I'm going to leave this as is for now, and I added it to the issue for tracking these types: #29817

In reply to: 217118070 [](ancestors = 217118070)

gafter · 2018-09-12T17:20:16Z

src/Compilers/Core/Portable/Symbols/IEventSymbol.cs

@@ -19,6 +19,11 @@ public interface IEventSymbol : ISymbol
        /// </summary>
        ITypeSymbol Type { get; }

+        /// <summary>
+        /// The declareted nullability of the this event.


declareted [](start = 16, length = 10)

mispel #Pending

gafter · 2018-09-12T17:21:05Z

src/Compilers/Core/Portable/Symbols/ILocalSymbol.cs

@@ -16,6 +16,12 @@ public interface ILocalSymbol : ISymbol
        /// </summary>
        ITypeSymbol Type { get; }

+        /// <summary>
+        /// The initial declared nullability of this local. If the local's type was inferred (i.e. via <code>var</code>), then


initial [](start = 16, length = 7)

I would drop "initial". #Pending

333fred · 2018-10-02T23:19:28Z

Remove Declared from the name of the Nullabilities. #Pending

333fred · 2018-10-02T23:19:57Z

Range Variables #Pending

333fred · 2018-10-02T23:23:03Z

src/Compilers/Core/Portable/Symbols/ISymbol.cs

+        /// <param name="nullability">The nullability to use for the symbol.</param>
+        /// <param name="format">Format or null for the default.</param>
+        /// <returns>A formatted string representation of the symbol.</returns>
+        string ToDisplayString(Nullability nullability, SymbolDisplayFormat format = null);


format [](start = 76, length = 6)

This default is not one that ignores nullability

333fred · 2018-10-04T23:02:27Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

        /// <summary>
        /// The type of the expression after it has undergone an implicit conversion. If the type
        /// did not undergo an implicit conversion, returns the same as Type.
        /// </summary>
        public ITypeSymbol ConvertedType { get; }

+        /// <summary>
+        /// The nullability of the expression after conversions have been applied. For most expressions,


conversions [](start = 52, length = 11)

Specify implicit

CyrusNajmabadi · 2018-10-04T23:06:48Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

+        /// <summary>
+        /// The inferred nullability of the expression represented by the syntax node.
+        /// </summary>
+        public Nullability Nullability { get; }


is there ever any concern that the 'Type' vs the 'ConvertedType' might have different Nullability? Or are tey guaranteed to be the same?

Lol. i can't read. sorry.

CyrusNajmabadi · 2018-10-04T23:09:05Z

src/Compilers/Core/Portable/Symbols/IArrayTypeSymbol.cs

+        /// <summary>
+        /// Gets the nullability of the elements stored in the array.
+        /// </summary>
+        Nullability ElementNullability { get; }


So, i'm not disagreeing with this approach. But how come this is not necessarily part of the ITypeSymobl itself at this projected level?

For example, there is no "IsElementTypeDynamic" even though "Dynamic" and "Object" are effectively the same under the covers. INstead, we represent each of those with different types. Why not the same here?

CyrusNajmabadi · 2018-10-04T23:09:55Z

src/Compilers/Core/Portable/Symbols/IEventSymbol.cs

+        /// <summary>
+        /// The declareted nullability of the this event.
+        /// </summary>
+        Nullability DeclaredNullability { get; }


I don't like the naming here. If type field is called ITypeSymbol Type it feels like this should be Nullability TypeNullability. That way you can easily track what this is referring to.

The key thing here is that Nullability is not referring to the nullability of the Type. It's referring to the nullability of the value.

CyrusNajmabadi · 2018-10-04T23:10:32Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

+        /// <summary>
+        /// The inferred nullability of the expression represented by the syntax node.
+        /// </summary>
+        public Nullability Nullability { get; }


I don't like the naming here. If type field is called ITypeSymbol Type it feels like this should be Nullability TypeNullability. That way you can easily track what this is referring to. This would also apply to 'ConvertedType' and 'ConvertedTypeNullability'. it basically augments the other field with that name.

CyrusNajmabadi · 2018-10-04T23:13:17Z

src/Compilers/Core/Portable/Symbols/IMethodSymbol.cs

+        /// Returns the nullability of the type arguments that have been substituted for the
+        /// type paramters.
+        /// </summary>
+        ImmutableArray<Nullability> TypeArgumentsNullability { get; }


this makes me like things even less. Note: the naming here is good (though the naming of ReturnNullability is inconsistent). But it seems pretty unfortunate that it's exposed through an entirely separate array, instead of being exposed on the individual TypeArgument type.

CyrusNajmabadi · 2018-10-04T23:13:41Z

src/Compilers/Core/Portable/Symbols/IPropertySymbol.cs

+        /// <summary>
+        /// Gets the declared nullability of the property
+        /// </summary>
+        Nullability DeclaredNullability { get; }


i feel like this shuld be TypeNullability. I don't know what 'Declared' means here.

CyrusNajmabadi · 2018-10-04T23:14:50Z

src/Compilers/Core/Portable/Symbols/ITypeParameterSymbol.cs

+        /// <summary>
+        /// The nullability of the constraints on the type parameter.
+        /// </summary>
+        ImmutableArray<Nullability> ConstraintsNullability { get; }


this name is inconsistent with ConstraintTypes. it also doesn't seem to match the naming of anything else in the PR.

333fred · 2018-10-04T23:15:43Z

@CyrusNajmabadi this PR is somewhat out of date, just FYI. Many of your naming concerns have already been addressed locally.

CyrusNajmabadi · 2018-10-04T23:15:45Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// <remarks>
+        /// This is used for legacy code scenarios, where a legacy API that does
+        /// not provide nullability information is being used. Expressions that
+        /// have unknown nullability are also referred to as oblivious expressions,


is there a good reason to have both names? if we refer to them as Oblivious, shoudl the name of this enum member be that?

I have it now because we used that terminology in the LDM, but I expect this will probably be removed. I don't know that we're fully satisfied with the work Unknown either, so that may also change.

CyrusNajmabadi · 2018-10-04T23:17:12Z

@CyrusNajmabadi this PR is somewhat out of date, just FYI. Many of your naming concerns have already been addressed locally.

Great. i'll look again when it's updaed. @jcouv pointed me this way due to questions i had around IDE impact of things.

CyrusNajmabadi · 2018-10-11T19:53:17Z

src/Compilers/Core/Portable/Symbols/Nullability.cs

+        /// </summary>
+        // PROTOTYPE(NullableReferenceTypes): Potentially link to the correct methods to compute?
+        // Some of them are defined on CSharpSemanticModel, so this could be difficult
+        NotComputed = 1,


exposing this publicly seems... weird. does that mean users can observe when we have haven't comptued this stuff?

CyrusNajmabadi · 2018-10-11T19:54:06Z

src/Compilers/Core/Portable/Symbols/ITypeSymbol.cs

+            SemanticModel semanticModel,
+            int position,
+            Nullability nullability,
+            SymbolDisplayFormat format = null);


this really feels like we're going to be virally spreading this outward. consdier anything in the IDE that ends up calling helpers like this. this info needs to pass outwards to any and all callpaths.

CyrusNajmabadi · 2018-10-11T19:55:30Z

src/Compilers/Core/Portable/Compilation/Extensions.cs

+        public static TypeInfo GetTypeWithNullability(this SemanticModel semanticModel, IFieldSymbol field, CancellationToken ct = default)
+        {
+            throw new NotImplementedException();
+        }


what if you don't have a semantic model... feels super weird that this would be at the semantic model level.

Consider any and all IDE features that do any sort of generation. Previously there was no need to pass along any sort of compilation. They could just use symbols.

Note: this affects public APIs we've shipped. These APIs would now seemingly become 'wrong' for any cases involving nullability. taht concerns me greatly.

CyrusNajmabadi · 2018-10-11T19:57:09Z

src/Compilers/Core/Portable/Compilation/TypeInfo.cs

@@ -35,11 +35,13 @@ public struct TypeInfo : IEquatable<TypeInfo>
        /// </summary>
        public Nullability ConvertedNullability { get; }

-        internal TypeInfo(ITypeSymbol type, ITypeSymbol convertedType)
+        internal TypeInfo(ITypeSymbol type, ITypeSymbol convertedType, Nullability nullability = Nullability.NotComputed, Nullability convertedNullability = Nullability.NotComputed)


TypeInfo is not strongly typed. So APIs that need to pass nullability information along now have hte unpalatable position of moving to this, but can lose the info if they were originally typed to something like an INamedTypeSymbol.

or they pass both along, but need to know that the both symbols must be the exact same instance.

CyrusNajmabadi · 2018-10-11T19:58:43Z

this feels super unclean. Let's do a thought experiment. Consider if we'd done 'dynamic' this way. Or tuple types this way. Where we had the 'underlying' type (like Object, or ValueTuple) and people needed to pass side-band information along about the 'projected type information' the compiler understood for that type.

Tihs would have a very viral and unpleasant impact on all downstream APIs.

CyrusNajmabadi · 2018-10-11T20:02:51Z

I dont' understand how i could use this API to support a scenario like "Generate me the Foo(string? s) method.

The way this API works today for genering Foo(string s) is that we have an IMethodSymbol with an appropriate IParameter symbol that has an appropriate ITypeSymbol representing "string".

But how would i generate such Foo(string? s) symbol today. If i'm trying to literally make the "ITypeSymbol" for that parameter symbol (which is how the IDE works), how do i do that? Note that i'm not using existing symbols (beyond stuff like 'string') i'm generating new stuff.

How would i ask the compiler to make me the IArrayTypeSymbol for string?[]? Today i can do: CreateArrayTypeSymbol(ITypeSymbol elementType, int rank). But now i need an overload of that that passes along nullability info. And i need to ensure that my 'elementType' is passed along internally with that nullability info. and that means all callers of anything that can bottom out here need to pass that along. Etc. etc.

we would now need sidechannels for anywhere that type information flowed to pass this stuff along. And it's not like that info is just at a single level. It can be deeply nested anywhere into where you might find a type signature.

Also note that we have many public APIs for users to do generation based on symbols. It feels like none of tehse APIs woudl work at all properly for nullable. This impacts not only our customers, but our own features. We'd end up having to deprecate and duplicate everything for this case of passing along side-channel info.

CyrusNajmabadi · 2018-10-11T20:06:46Z

I think any API likely needs to actually do the legwork to show the impact on actual consumptive components (like many of the IDE components). We have deeply baked in (and publicly exposed) the means to do things using ISymbols as our expected currency. That whole approach seems to be thrown out the window, with effectively much of our existing old API getting deprecated. For example, it seems virtually certain that using the old ToDisplayString is now wrong (as it won't properly reflect nullability info). So literally all code taht uses it has to move foward. It's most likely that lots of code will miss this (as well as huge swaths of external code). So we'll basically have things just not behaving right.

This is what would have happened with any other changes to symbols that we exposed in this manner. For example, if dynamic symbols were exposed as an extra boolean you passed along with your regular type symbol, or if value-tuples passed along their names as extra information next to the itypesymbol (instead of being exposed through the symbol).

If we go the sidechannel approach, then that basically will force a viral update of any and all codepaths that need to funnel this information down to interior compoments and will virtually guarantee that many existing entrypoints (which we need to keep for backcompat) will be doing the wrong thing.

333fred · 2018-10-12T00:49:52Z

@CyrusNajmabadi PR updated after today's meeting. At this point, I think we can separate the two concepts that we're trying to add with this API:

How to get some structure (the structure itself is part 2) with fully-calculated nullability information.

This is part of the API is answered in a similar way to getting a speculative semantic model: you call GetNullabilityAwareSemanticModel on an existing semantic model, and then query it as you would a normal semantic model. When you perform a query for an expression in a method, we will bind the method, perform the flow analysis, and return you a structure with fully-calculated nullability. This allows for caching and sharing of that semantic model, which should alleviate a lot of perf concerns and still allow for the wonderful idempotence of the API. It also has the benefit of needing only one new API to get at the information, and reuses a concept that semantic models already use (ie, parent/child models). In talking offline, we seemed to agree on this part.
How the structure is represented.

Containing Scope

On this point is where the main contention lies at this point. In this proposal, nullability information is contained by the containing symbol. So to get the nullability of the array type (ie string?[]), you would look at the ElementTypeNullability property on IArrayTypeSymbol. This follows for any other nullability. Nullability and ConvertedNullability are added to TypeInfo to allow GetTypeInfo to work correctly. This has some disadvantages:
1. We need to plum this information through large portions of code, both in the IDE and in external analyzers.
2. Many existing APIs that we cannot remove will effectively be doing the wrong thing.
Nullability on ITypeSymbol

Another option (what @CyrusNajmabadi is advocating) is to make ITypeSymbol have a Nullability property. This has gotten some heavy feedback from the compiler team (@gafter, you've been especially vehement) that this is going to negatively impact the memory pressure in the IDE. The technical changes behind the scene of making this change are
- We need to have separate public types for string! (not nullable), string~ (oblivious, legacy code), string? (nullable)
- This means that we need to have wrappers over some base string type, much like we do with generics
The assertion from the compiler team is that these non-generic types are significantly more prevalent, and this will cause issues where generics today do not. We could take some steps to cache these wrappers, but this would not be a trivial amount of work. When I call GetMembers on a string?, the ContainingSymbol of the returned members will need to point to string?, not string~ or string!. This means that we will need wrappers for all symbols, not just ITypeSymbols, and this problem gets bigger.

Separate Nullability data structure

Another options that was briefly discussed and has been mostly discarded is to create a separate object model for nullability. So you'd have the ITypeSymbol structure, and a parallel Nullability data structure that you would consult to get the real nullabilities of things. We didn't like this for a variety of reasons, including creating an entirely new object model, being difficult to map between these object models, and being a generally unworkable API for the IDE.

CyrusNajmabadi · 2018-10-12T01:09:23Z

that this is going to negatively impact the memory pressure in the IDE.

I agree that this is a significant thing to be concerned about. I would very much be curious though what the actual end impact is and would appreciate a true measurement to understand the impact.

CyrusNajmabadi · 2018-10-12T01:16:51Z

For comparison, the TypeScript compiler needs to deal with this situation and worse. it is super common to have all sorts of ephemeral types being sythesized through code-flow analysis due to recursion in structural types. TS also has to handle things like null vs undefined. Finally, it also has to deal without having any sort of concept like "metadata". All symbols are from source, in every compilation. In practice, representing these types was not actually problematic.

However, it is worth noting that TS represents these things differently. Nullable reference types are represented similarly to how we would represent nullable value types. In that, it's not a property on a type. Instead, there's a wrapper type that is instantiated. For TS, these are types like string, string | undefined, string | null and string | undefined | null. Heavy caching ensures that you can reuse the same instantiations from before.

Despite all this, the memory overhead of being able to represent nullable types was not actually problematic, and did not impact IDE scenarios. Note that TypeScript has an even harder job than Roslyn/C#/VB. That's because in IDE scenarios, TypeScript has to deal with inferred function return types (something which doesn't exist in teh C# language). So TS constantly has to jump across functions to analyze them and determine their return types. Even with this, perf (both latency and memory) remains super good there. So my gut makes me feel that it would be unlikely to have a perf issue for C#/VB.

IMO, we should try these approaches out and actually measure the core scenarios. For example we should see the impact on:

total compilation time. We don't want to majorly regress this.
compilation memory metrics. We don't want to incur much high overall memory usage, given that the compiler might be used in scenarios where memory is premium.
IDE latency cases. We don't want to be slowing down things like typing.
IDE memory metrics. We don't want significantly cause problems for VS

At the same time, we be willing to understand what we might think was acceptable marginal overhead. For example, if this causes memory usage in VS to jump from 100MB to 200MB, that's a lot. But, for example, if it only increased memory in real use cases by a few percent, then it can be argued that the benefit may be worth it. Analysis of measurements should take into account relative impact and what should be acceptable bounds here.

CyrusNajmabadi · 2018-10-12T01:23:21Z

Note: if we do '1' (GetNullabilityAwareSemanticModel), then it seems like the concerns with 'Nullability on ITypeSymbol' are vastly diminished right? For example, the compiler would practically never call GetNullabilityAwareSemanticModel. Internally it would still use the appropriate efficient struct-based impl and would avoid the 'make lots of type symbols' problem. Basically, it would use the API as described as per dcbd24e internally. But when these symbols were exposed through the public API, that view would flatten into synthesized symbols that exposed through something like a 'Nullability' property.

This would only impact IDE features that then wanted this information. And IDE features are really only going to light up and need this for analyzing the code right around where the user is. So while there will be extra symbols here, it should be fairly well contained and not explosive in impact (IMO at least).

This also then takes care of 2.i and 2.ii really nicely.

To me, this seems like it would be the best of all worlds, and would be easy to test for. Thoughts?

333fred requested a review from a team as a code owner July 26, 2018 19:53

333fred requested review from gafter, cston and jcouv July 26, 2018 19:54

333fred added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 26, 2018

jcouv reviewed Jul 26, 2018

View reviewed changes

cston reviewed Jul 26, 2018

View reviewed changes

jcouv reviewed Jul 26, 2018

View reviewed changes

cston reviewed Jul 26, 2018

View reviewed changes

cston approved these changes Jul 27, 2018

View reviewed changes

gafter reviewed Jul 27, 2018

View reviewed changes

jcouv self-assigned this Jul 29, 2018

cston reviewed Aug 7, 2018

View reviewed changes

333fred added 4 commits September 10, 2018 17:17

Adds public API changes to support nullable. Interface changes only, …

ce05391

…no implementation.

Added typeinfo and display apis.

d8ebb44

Update API surface from review

28ddad6

Started added docs.

ce8f58e

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 11, 2018

Updates to the API names and first stab at documentation.

02909f6

333fred force-pushed the nullable-api branch from d0b8c54 to 02909f6 Compare September 11, 2018 22:59

gafter reviewed Sep 12, 2018

View reviewed changes

333fred commented Oct 2, 2018

View reviewed changes

333fred commented Oct 4, 2018

View reviewed changes

CyrusNajmabadi reviewed Oct 4, 2018

View reviewed changes

Current proposal update.

a21916e

CyrusNajmabadi reviewed Oct 11, 2018

View reviewed changes

Further updates after today's meeting.

dcbd24e

333fred mentioned this pull request Oct 12, 2018

[WIP] Do Not Merge Disable incremental binding in the MethodBodySemanticModel. #30487

Closed

jasonmalinowski closed this Apr 26, 2019

333fred deleted the nullable-api branch April 29, 2019 21:13

Nullable API Additions #28860

Nullable API Additions #28860

Conversation

333fred commented Jul 26, 2018

jcouv Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 26, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

jcouv Jul 26, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 26, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Jul 26, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

jasonmalinowski Aug 3, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

jcouv Aug 3, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Jul 27, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

cston Aug 7, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

gafter Sep 12, 2018 • edited by 333fred Loading

Choose a reason for hiding this comment

333fred commented Oct 2, 2018 • edited Loading

333fred commented Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 26, 2018 •

edited

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

cston Jul 26, 2018 •

edited by 333fred

Loading

jcouv Jul 26, 2018 •

edited by 333fred

Loading

cston Jul 26, 2018 •

edited by 333fred

Loading

cston Jul 26, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

jcouv Jul 27, 2018 •

edited by 333fred

Loading

jasonmalinowski Aug 3, 2018 •

edited by 333fred

Loading

jcouv Aug 3, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

gafter Jul 27, 2018 •

edited by 333fred

Loading

cston Aug 7, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

gafter Sep 12, 2018 •

edited by 333fred

Loading

333fred commented Oct 2, 2018 •

edited

Loading

333fred commented Oct 2, 2018 •

edited

Loading

CyrusNajmabadi commented Oct 11, 2018 •

edited

Loading

CyrusNajmabadi commented Oct 11, 2018 •

edited

Loading

333fred commented Oct 12, 2018 •

edited

Loading

CyrusNajmabadi commented Oct 12, 2018 •

edited

Loading