Use struct for TypeSymbolWithAnnotations by cston · Pull Request #28943 · dotnet/roslyn

cston · 2018-07-30T18:30:52Z

The struct TypeSymbolWithAnnotations includes an allocation for less common cases only: for nullable type parameters; and when the type has associated custom modifiers.

jcouv · 2018-07-30T23:52:19Z

src/Compilers/CSharp/Portable/Binder/Binder.NamespaceOrTypeOrAliasSymbolWithAnnotations.cs

-                Debug.Assert(symbolOrTypeSymbolWithAnnotations != null);
-                Debug.Assert(!(symbolOrTypeSymbolWithAnnotations is TypeSymbol));
-                _symbolOrTypeSymbolWithAnnotations = symbolOrTypeSymbolWithAnnotations;
+                Debug.Assert(type == null != (symbol is null));


type == null [](start = 29, length = 12)

~~How does type == null work? (or _type != null below)~~
Found public static bool operator ==(TypeSymbolWithAnnotations? x, TypeSymbolWithAnnotations? y).

I wonder if having a type.IsDefault would be clearer. #Resolved

I've tried both: IsNull and operator==(?, ?). The latter seemed slightly better since the result was closer to the existing code. But I'm open to either approach.

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

It's not blocking this PR (ie. up to you), but I do think IsNull or IsDefault would be easier to follow. Already Jared and I tripped on this.

In reply to: 206539416 [](ancestors = 206539416,206357806)

jcouv · 2018-07-31T00:26:53Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs

+                        type = type.WithTypeAndModifiers(
+                            CustomModifierUtils.CopyTypeCustomModifiers(overriddenPropertyType.TypeSymbol, type.TypeSymbol, this.ContainingAssembly, nonNullTypesContext: this),
                            overriddenPropertyType.CustomModifiers);
+                        _lazyType = default;


_lazyType [](start = 24, length = 9)

This makes me nervous (race condition?) #Closed

This assignment is in the .ctor, so the field is not available to other threads.

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

jcouv · 2018-07-31T00:39:08Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

                var expression = node.ExpressionOpt;
                var type = (expression is null) ?
-                    NoReturnExpression :
+                    TypeSymbolWithAnnotations.CreateUnannotated(NonNullTypesFalseContext.Instance, NoReturnExpression) :


NonNullTypesFalseContext.Instance [](start = 64, length = 33)

Feels like we should use the context from the containing method/symbol. #Closed

NoReturnExpression is used as a placeholder only. And the TypeSymbol field is the only field used.

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

Added NonNullTypesUnusedContext.

In reply to: 206540166 [](ancestors = 206540166,206364611)

jcouv · 2018-07-31T00:39:48Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

+            var returnType = inferredReturnType.Type;
+            if (returnType == null)
+            {
+                returnType = TypeSymbolWithAnnotations.CreateUnannotated(NonNullTypesFalseContext.Instance, LambdaSymbol.InferenceFailureReturnType);


NonNullTypesFalseContext.Instance [](start = 73, length = 33)

Same here #Resolved

Similar to NoReturnExpression, this value is a placeholder only, with only TypeSymbol used.

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

Still, I think we should consider it. It probably doesn't make a big difference, but it will simplify the upcoming scrub (to make sure we pass proper contexts everywhere).

In reply to: 206540785 [](ancestors = 206540785,206364705)

jcouv · 2018-07-31T00:42:28Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

                case ConversionKind.ExplicitDynamic:
                case ConversionKind.ImplicitDynamic:
-                    isNullableIfReferenceType = operandType?.IsNullable;
+                    isNullableIfReferenceType = operandType == null ? null : operandType.IsNullable;


operandType == null ? null : operandType.IsNullable [](start = 48, length = 51)

~~Seems equivalent to the original code~~ Never mind #Closed

jcouv · 2018-07-31T00:46:58Z

src/Compilers/CSharp/Portable/Symbols/SubstitutedParameterSymbol.cs

                var mapOrType = _mapOrType;
-                var type = mapOrType as TypeSymbolWithAnnotations;
-                if ((object)type != null)
+                if (mapOrType is TypeSymbolWithAnnotations)


if (mapOrType is TypeSymbolWithAnnotations) [](start = 16, length = 43)

nit: if (mapOrType is TypeSymbolWithAnnotations typeSymbolWithAnnotations) return typeSymbolWithAnnotations; #Closed

jcouv · 2018-07-31T00:49:24Z

src/Compilers/CSharp/Portable/Symbols/Source/LambdaSymbol.cs

            _syntax = unboundLambda.Syntax;
            _refKind = refKind;
-            _returnType = returnType ?? ReturnTypeIsBeingInferred;
+            _returnType = returnType == null ? TypeSymbolWithAnnotations.CreateUnannotated(NonNullTypesFalseContext.Instance, ReturnTypeIsBeingInferred) : returnType;


NonNullTypesFalseContext.Instance [](start = 91, length = 33)

Can we pass a better context here too? #Closed

Placeholder, for TypeSymbol only.

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

jcouv · 2018-07-31T00:52:28Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+            _isAnnotated = type.IsAnnotated;
+            Interlocked.CompareExchange(ref _nonNullTypesContext, type.NonNullTypesContext, null);
+            Interlocked.CompareExchange(ref _implementation, type.Implementation, null);
+            return (object)Interlocked.CompareExchange(ref _defaultType, type.DefaultType, null) == null;


return [](start = 12, length = 6)

Does anyone care for the return value? #Closed

Yes, several callers decide whether to perform additional checks based on the return value.

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

jcouv · 2018-07-31T00:56:40Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+
+        internal static TypeSymbolWithAnnotations CreateLazyNullableType(CSharpCompilation compilation, TypeSymbolWithAnnotations underlying)
+        {
+            return new TypeSymbolWithAnnotations(defaultType: underlying.DefaultType, nonNullTypesContext: null, isAnnotated: true, TypeSymbolWithAnnotationsImplementation.CreateLazy(compilation, underlying));


null [](start = 107, length = 4)

~~The support for nonNullTypesContext = null is temporary. Could we avoid adding more scenarios that rely on it?~~

Actually, I think TypeSymbolWithAnnotations should not allow null nonNullTypesContext. See line 146 for solution.
Please add an assertion to the constructor too. Thanks #Closed

jcouv · 2018-07-31T00:59:04Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

-                Debug.Assert((object)typeSymbol != null);
                Debug.Assert(!customModifiers.IsDefault);
-                Debug.Assert(!typeSymbol.IsNullableType() || isAnnotated);
-                Debug.Assert(nonNullTypesContext != null);


Debug.Assert(nonNullTypesContext != null); [](start = 16, length = 42)

I think we should keep this assert #Closed

jcouv · 2018-07-31T01:00:48Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

            {
-                Debug.Assert((object)typeSymbol != null);
                Debug.Assert(!customModifiers.IsDefault);
-                Debug.Assert(!typeSymbol.IsNullableType() || isAnnotated);


This assertions also seems useful to keep #Closed

jcouv · 2018-07-31T01:02:49Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

        /// true for int?, T? where T : struct.
        /// </summary>
-        public abstract bool IsAnnotated { get; }
+        public readonly bool IsAnnotated;


IsAnnotated [](start = 29, length = 11)

Let's move those fields to the top. Thanks #Closed

jcouv · 2018-07-31T01:04:34Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+    internal struct TypeSymbolWithAnnotations : IFormattable
    {
-        public sealed override string ToString() => TypeSymbol.ToString();
+        internal readonly TypeSymbol DefaultType;


DefaultType and Implementation were not obvious. Maybe useful to rename and/or comment. #Closed

jcouv

Done with most files except SymbolWithAnnotations. I'll finish tomorrow.

jaredpar · 2018-07-31T14:07:52Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+            }
+            _isAnnotated = type.IsAnnotated;
+            Interlocked.CompareExchange(ref _nonNullTypesContext, type.NonNullTypesContext, null);
+            Interlocked.CompareExchange(ref _implementation, type.Implementation, null);


Don't think we can safely use CompareExchange here. The fields of a struct can always be assigned directly by way of simple struct assignment. Example

TSWABuilder t1 = someOtherTSWABuilder

The use of CompareExchange here is only safe if we also categorically disable assignment of these builder types. #ByDesign

The builder is intended for use for lazy type calculations in Symbol implementations. Those callers are responsible for hiding the underlying field and ensuring access is otherwise thread-safe, as they are today.

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

jaredpar · 2018-07-31T14:10:03Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

    /// </summary>
    [DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
-    internal abstract class TypeSymbolWithAnnotations : IFormattable
+    internal struct TypeSymbolWithAnnotations : IFormattable


readonly struct? #Resolved

jaredpar · 2018-07-31T14:12:50Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

            public override int GetHashCode(TypeSymbolWithAnnotations obj)
            {
-                if (obj is null)
+                if (obj == null)


The null check here is invalid now that TSWA is a struct #Resolved

This uses operator==(TypeSymbolWithAnnotations?, TypeSymbolWithAnnotations?). As @jcouv suggested, this might be easier to read as obj.IsDefault or some similar property.

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

jaredpar · 2018-07-31T14:13:10Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

                {
-                    return y is null;
+                    return y == null;
                }


The null check is invalid now that TSWA is a struct #Resolved

…s' into struct

cston · 2018-07-31T17:58:16Z

@dotnet/roslyn-compiler please review.

jcouv · 2018-07-31T18:08:20Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+            private bool _isAnnotated;
+            private Extensions _extensions;
+
+            internal TypeSymbol DefaultType => _defaultType;


DefaultType [](start = 32, length = 11)

Consider xml doc on the builder's properties as well. That'll be useful for QuickInfo in code. #Closed

jcouv · 2018-07-31T18:13:40Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+
+        internal static TypeSymbolWithAnnotations CreateLazyNullableType(CSharpCompilation compilation, TypeSymbolWithAnnotations underlying)
+        {
+            return new TypeSymbolWithAnnotations(defaultType: underlying._defaultType, nonNullTypesContext: underlying.NonNullTypesContext, isAnnotated: true, Extensions.CreateLazy(compilation, underlying));


Extensions.CreateLazy(compilation, underlying) [](start = 159, length = 46)

It seems we're still allocating for every TSWA.
To save on allocations, I assumed we'd have null extensions for the common case. But that doesn't seem to be the current design. Would that be possible? #Closed

We're allocating for every TypeSymbolWithAnnotations that represent either an annotated type parameter (this case) or a type with associated custom modifiers. For other cases, there shouldn't be allocations. (Those other cases use the shared Extensions.Default value.)

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

Thanks for the clarification.

In reply to: 206635252 [](ancestors = 206635252,206631539)

jcouv · 2018-07-31T19:25:32Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

        }

-        public virtual void ReportDiagnosticsIfObsolete(Binder binder, SyntaxNode syntax, DiagnosticBag diagnostics)
+        public void ReportDiagnosticsIfObsolete(Binder binder, SyntaxNode syntax, DiagnosticBag diagnostics) => _extensions.ReportDiagnosticsIfObsolete(this, binder, syntax, diagnostics);


=> [](start = 109, length = 2)

nit: I don't think we've established a style, but I'd suggest breaking the lines with =>, especially the longer ones (if not all).

public void ReportDiagnosticsIfObsolete(Binder binder, SyntaxNode syntax, DiagnosticBag diagnostics) => _extensions.ReportDiagnosticsIfObsolete(this, binder, syntax, diagnostics); ``` #Resolved

jcouv · 2018-07-31T19:27:58Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+                return new LazyNullableTypeParameter(compilation, underlying);
+            }
+
+            public abstract TypeSymbol GetResolvedType(TypeSymbol defaultType);


public [](start = 12, length = 6)

nit: there's a mix of internal and public. Consider standardizing on one (my preference is internal, but that's up to you). #Resolved

jcouv · 2018-07-31T19:30:33Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

        }

-        internal string GetDebuggerDisplay() => ToDisplayString(DebuggerDisplayFormat);
+        internal string GetDebuggerDisplay() => _defaultType is null ? "null" : ToDisplayString(DebuggerDisplayFormat);


GetDebuggerDisplay [](start = 24, length = 18)

nit: would it be useful to display something about the extension too?

Let's add that later, when it's more obvious what to display for _extension.

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

jcouv

LGTM (iteration 7).
As we discussed, switching to IsNull will be even better. Thanks

jcouv · 2018-07-31T22:52:41Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

        }

-        private void AddNullableAnnotations(TypeSymbolWithAnnotations typeOpt)
+        private void AddNullableAnnotations(TypeSymbolWithAnnotations? typeOpt)


TypeSymbolWithAnnotations? [](start = 44, length = 26)

Would it make sense to use a non-nullable TSWA, and use the default value for "nothing"? #Closed

jcouv

LGTM Thanks! (iteration 10)

333fred · 2018-08-01T22:20:46Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+            /// <summary>
+            /// Set the fields of the builder.
+            /// </summary>
+            internal bool InterlockedInitialize(TypeSymbolWithAnnotations type)


We should add a comment that we're ok with caller A setting some fields and caller B setting other fields, and that it's not guaranteed that the exact same caller will set all fields of the builder.
. #Closed

333fred · 2018-08-01T22:22:16Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+            internal TypeSymbolWithAnnotations ToType()
+            {
+                return (object)_defaultType == null ?
+                    default :


Is this a valid case? Should we add an assert? #ByDesign

This is a valid case, when the TypeSymbolWithAnnotations is used to represent a null type.

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

333fred · 2018-08-01T22:23:45Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

+
+        /// <summary>
+        /// Returns:
+        /// false for string, int, T;


T [](start = 35, length = 1)

Can return true for unannotated T after adjusting in nullablewalker (and if we move up checking for unconstrained type parameters to initial binding, potentially before #Closed

We may want a different field than IsAnnotated in that case, or a renamed field, since T was unannotated. Let's revisit this comment then.

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

333fred · 2018-08-01T22:37:09Z

src/Compilers/CSharp/Portable/Symbols/SymbolWithAnnotations.cs

    /// </summary>
    [DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
-    internal abstract class TypeSymbolWithAnnotations : IFormattable
+    internal readonly struct TypeSymbolWithAnnotations : IFormattable


Consider renaming this file to match the type. #Closed

Yes. I'll rename the file in a separate PR.

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

333fred · 2018-08-01T22:53:16Z

src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.cs

                // another diagnostic for the same issue.
            }
-            else if (!_alreadyReported[slot] && VariableType(symbol)?.IsErrorType() != true)
+            else if (!_alreadyReported[slot] && VariableType(symbol).IsErrorType() != true)


Should check for non-null and remove the != true #Closed

Changed the != true, but did not add the null check since I think you can only get here with a symbol that has a type.

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

333fred

LGTM (commit 16)

… into unconstrained-type-parameter-nullability * dotnet/features/NullableReferenceTypes: Include nullability in CheckConstraints (dotnet#28959) Use struct for TypeSymbolWithAnnotations (dotnet#28943) Publish Linux Test Results Force retest Migrate Linux tests to VSTS Port determinism fixes Make sure TLS 1.2 is used to fetch from https://dot.net Move to language version 8 Make mutex creation more robust Disable icacls use on Helix Disable leak detection tests on x64 VSTS YAML file

cston requested a review from a team as a code owner July 30, 2018 18:30

cston added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 30, 2018

Use struct for TypeSymbolWithAnnotations

d7d3350

cston force-pushed the struct branch from 02296e8 to d7d3350 Compare July 30, 2018 19:06

jcouv reviewed Jul 30, 2018

View reviewed changes

jcouv reviewed Jul 31, 2018

View reviewed changes

jaredpar reviewed Jul 31, 2018

View reviewed changes

cston added 4 commits July 31, 2018 08:05

PR feedback

e463a7f

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

27d8cc0

…s' into struct

Fix build

625fa3e

Fix tests

085f7c5

jcouv reviewed Jul 31, 2018

View reviewed changes

Add NonNullTypesUnusedContext

f6c357a

jcouv reviewed Jul 31, 2018

View reviewed changes

Add comments

fd2fc2d

jcouv reviewed Jul 31, 2018

View reviewed changes

jcouv approved these changes Jul 31, 2018

View reviewed changes

cston added 3 commits July 31, 2018 12:38

PR feedback

246f16b

IsNull

c1bab66

Misc.

273127c

jcouv reviewed Jul 31, 2018

View reviewed changes

jcouv approved these changes Jul 31, 2018

View reviewed changes

cston added 2 commits July 31, 2018 16:10

Misc.

d1e524e

Fix tests

dac9e36

jcouv self-assigned this Aug 1, 2018

cston added 2 commits July 31, 2018 21:28

Fix tests

d2f59f1

Misc.

25b20e6

333fred reviewed Aug 1, 2018

View reviewed changes

Fix tests

93bb84e

333fred reviewed Aug 1, 2018

View reviewed changes

PR feedback

1a773a2

333fred approved these changes Aug 1, 2018

View reviewed changes

cston merged commit 14efbde into dotnet:features/NullableReferenceTypes Aug 2, 2018

cston deleted the struct branch August 2, 2018 14:46

Conversation

cston commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 30, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar Jul 31, 2018 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

cston commented Jul 30, 2018 •

edited

Loading

jcouv Jul 30, 2018 •

edited by cston

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited by cston

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

jaredpar Jul 31, 2018 •

edited by cston

Loading

jaredpar Jul 31, 2018 •

edited by cston

Loading

jaredpar Jul 31, 2018 •

edited by jcouv

Loading

jaredpar Jul 31, 2018 •

edited by jcouv

Loading

jcouv Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited

Loading

cston Jul 31, 2018 •

edited

Loading

jcouv Jul 31, 2018 •

edited by cston

Loading

jcouv Jul 31, 2018 •

edited by cston

Loading

jcouv Jul 31, 2018 •

edited

Loading