Nullability Rewriter Step by 333fred · Pull Request #34017 · dotnet/roslyn

333fred · 2019-03-11T21:26:52Z

This adds an initial implementation of the NullableWalker part of
rewriting the bound tree with nullability information, and hooks up the
rewriter to this. It additionally adds a debug verifier for the
NullableWalker that will verify that we're actually visiting and
recording results for all BoundExpression nodes in the tree. Some tests
are currently skipped where this does not hold true to allow the review
cycle to start while work continues.

Additionally, the VerifyTypes compilation helper is hooked up to this
rewritten information as a basic smoke test of the information being
produced by the rewriter.

For this PR, I'm most concerned about the general approach of getting
the information from the NullableWalker. If you have specific concerns
about some of the information from the walker being incorrect, please
note it and I'll put a prototype comment to follow up and add a test
to verify the behavior, but I'm not going to correct individual
scenarios in this PR.

To make this PR reviewable, I've separated the changes into 3 commits, and
I suggest going commit by commit. Commit 1 is the public API rename from
Nullability to NullableAnnotation and NullableFlowState. Commit 2 is
separating the public version of NullableAnnotation from the private
version, as that affects what we emit in the Nullable attribute and
we don't want to have a concept of default in that code path. Commit 3
adds the rewriting logic to the NullableWalker and skips some tests
that are currently not passing. There is some bleed-over of the rename
in NullableReferenceTypeTests.cs as I did not reorganize the commit
line-by-line in that file. I was going to clean up the locations
of NullableAnnotation/NullableFlowState in this PR, but no matter what
I do I'm going to have some conflicts with Neal's change so I'll fix
those up after this is merged.

…notation and NullableFlowState, and update the the rewriter to use that instead of the Nullability enum.

…e walker and rewriting them.

333fred · 2019-03-12T00:11:36Z

The bootstrap build is failing, but that's not due to anything that will cause massive changes during review as I work on fixing the issue, so I'll ask for reviews from @dotnet/roslyn-compiler while I start working on them. #Closed

cston · 2019-03-12T14:49:50Z

{

What is the change to the .sln file? #ByDesign

Refers to: Compilers.slnf:1 in 5b91a6c. [](commit_id = 5b91a6c, deletion_comment = False)

cston · 2019-03-12T15:27:39Z

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


        // PROTOTYPE(nullable-api): Doc Comment
-        public Nullability Nullability { get; }
+        public NullabilityInfo NullabilityInfo { get; }


Info [](start = 42, length = 4)

Consider dropping Info suffix from field names. #Resolved

cston · 2019-03-12T15:35:34Z

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

-                && object.Equals(this.Nullability, other.Nullability)
-                && object.Equals(this.ConvertedNullability, other.ConvertedNullability);
+                && object.Equals(this.NullabilityInfo, other.NullabilityInfo)
+                && object.Equals(this.ConvertedNullabilityInfo, other.ConvertedNullabilityInfo);


NullabilityInfo should implement IEquatable<NullabilityInfo> and the implementation should be used here to avoid boxing. #Resolved

cston · 2019-03-12T15:37:10Z

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

+    }
+
+    /// <summary>
+    /// The nullable state of an rvalue computed in NullableWalker.


NullableWalker [](start = 52, length = 14)

Doc comment should not refer to internal types. #Resolved

cston · 2019-03-12T15:38:53Z

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

 namespace Microsoft.CodeAnalysis.CSharp.Symbols
 {
-    internal enum NullableAnnotation : byte
+    internal enum CSharpNullableAnnotation : byte


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

Please remove the CSharp prefix. #Resolved

cston · 2019-03-12T15:54:55Z

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

        }

-        public new Nullability TopLevelNullability
+        public virtual new NullabilityInfo TopLevelNullabilityInfo


TopLevelNullabilityInfo [](start = 43, length = 23)

Consider dropping the Info suffix. #Resolved

cston · 2019-03-12T16:04:10Z

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

+            // Bit 3: Whether nullability has been set
+            // Bit 4: 1 if the node is nullable, 0 if the node is not nullable
+            // Bits 5 and 6: 01 if the node is not annotated, 10 if the node is annotated, 00 if the node is disabled
+            TopLevelNullableInfoSet = 1 << 3,


InfoSet [](start = 28, length = 7)

InfoSet prefix seems unnecessary. #Resolved

333fred · 2019-03-12T21:48:52Z

{

There were a couple of missing tools projects like the generator projects. It also looks like VS sorted some projects when I saved the solution filter.

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

Refers to: Compilers.slnf:1 in 5b91a6c. [](commit_id = 5b91a6c, deletion_comment = False)

333fred · 2019-03-12T21:52:48Z

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


-                    case BoundNodeAttributes.TopLevelUnknown:
-                        return Nullability.Unknown;
+                var annotation = (_attributes & BoundNodeAttributes.TopLevelAnnotationMask) switch


@jaredpar here's a switch expression for you :). #Closed

cston · 2019-03-12T22:16:00Z

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

+        NotAnnotated, // Type is not annotated - string, int, T (including the case when T is unconstrained).
+        Annotated,    // Type is annotated - string?, T? where T : class; and for int?, T? where T : struct.
+        NotNullable,  // Explicitly set by flow analysis
+        Nullable,     // Explicitly set by flow analysis


Do we need these two values? #Closed

Neal is removing them in the master. The next merge will remove them.

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

jcouv · 2019-03-13T00:21:34Z

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


        // PROTOTYPE(nullable-api): Document
-        Nullability ReceiverNullability { get; }
+        NullableAnnotation ReceiverNullableAnnotation { get; }


ReceiverNullableAnnotation [](start = 27, length = 26)

What's a scenario where receiver could be annotated? #Closed

Reduced extension methods.

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

jcouv · 2019-03-13T00:23:01Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

+Microsoft.CodeAnalysis.NullableAnnotation.Annotated = 3 -> Microsoft.CodeAnalysis.NullableAnnotation
+Microsoft.CodeAnalysis.NullableAnnotation.Default = 0 -> Microsoft.CodeAnalysis.NullableAnnotation
+Microsoft.CodeAnalysis.NullableAnnotation.NotAnnotated = 2 -> Microsoft.CodeAnalysis.NullableAnnotation
+Microsoft.CodeAnalysis.NullableAnnotation.NotNullable = 4 -> Microsoft.CodeAnalysis.NullableAnnotation


NotNullable [](start = 42, length = 11)

We should probably remove Nullable/NotNullable from the annotations enum at this point. #Closed

Neal has done this in master. When I merge that in, these will go away.

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

Neal changed the internal enum (Microsoft.CodeAnalysis.CSharp.Symbols.NullableAnnotation), not the public one which you're creating.
Is this PR negatively affected if it's doesn't add public Nullable/NotNulable members?

In reply to: 265363390 [](ancestors = 265363390,264935207)

I don't want to have to add logic to it to correctly handle converting Nullable to Annotated and NotNullable to NotAnnotated.

In reply to: 265676688 [](ancestors = 265676688,265363390,264935207)

jcouv · 2019-03-13T00:23:54Z

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

    public readonly struct TypeInfo : IEquatable<TypeInfo>
    {
-        internal static readonly TypeInfo None = new TypeInfo(null, null, Nullability.NotComputed, Nullability.NotComputed);
+        internal static readonly TypeInfo None = new TypeInfo(null, null, default, default);


default [](start = 74, length = 7)

nit: consider naming the arguments #Closed

jcouv · 2019-03-13T00:25:54Z

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

        bool HasReferenceTypeConstraint { get; }

-        Nullability ReferenceTypeConstraintNullability { get; }
+        NullableAnnotation ReferenceTypeConstraintNullableAnnotation { get; }


ReferenceTypeConstraintNullableAnnotation [](start = 27, length = 41)

// PROTOTYPE(nullable-api): Document #Closed

jcouv · 2019-03-13T00:29:38Z

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

+        public override int GetHashCode() =>
+            Hash.Combine(Annotation.GetHashCode(), FlowState.GetHashCode());
+
+    }


} [](start = 4, length = 1)

nit: unnecessary empty line above #Closed

jcouv · 2019-03-13T00:34:11Z

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

+            IsSuppressed = 1 << 2,
+
+            // Bit 3: Whether nullability has been set
+            // Bit 4: 1 if the node is nullable, 0 if the node is not nullable


// Bit 4: 1 if the node is nullable, 0 if the node is not nullable [](start = 12, length = 66)

nit: Perhaps "// Bit 4: 1 if the node has maybe-null state, 0 if the node is not-null" #Closed

jcouv · 2019-03-13T00:37:17Z

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

+            TopLevelFlowStateNullable = 1 << 4,
+            TopLevelNotAnnotated = 1 << 5,
+            TopLevelAnnotated = 1 << 6,
+            TopLevelDisabled = 0,


TopLevelDisabled = 0, [](start = 12, length = 21)

Do we need this value? #Closed

jcouv · 2019-03-13T00:40:23Z

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

+            // Bit 3: Whether nullability has been set
+            // Bit 4: 1 if the node is nullable, 0 if the node is not nullable
+            // Bits 5 and 6: 01 if the node is not annotated, 10 if the node is annotated, 00 if the node is disabled
+            TopLevelNullableInfoSet = 1 << 3,


TopLevelNullableInfoSet [](start = 12, length = 23)

I don't think we need this extra bit to know if nullability has been set. You can use the two later bits to encode that: 00 is unset, 01 is not-annotated, 10 is annotated, and 11 is disabled. #Closed

jcouv · 2019-03-13T00:43:15Z

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

            get { return this.ConstantValueOpt; }
        }
+
+        public override NullabilityInfo TopLevelNullabilityInfo


TopLevelNullabilityInfo [](start = 40, length = 23)

~~Can we remove the override and seal the definition in BoundNode?~~
Can we remove BoundExpression.TopLevelNullabilityInfo and overrides? The implementation in BoundNode should be sufficient. #Resolved

This is working around an issue that Neal has fixed in master that I haven't checked to ensure is resolved in my branch yet: you couldn't have a TSWA with a null symbol that wasn't considered Unknown. That may be fixed, but I haven't checked it yet.

As to the BoundNode vs BoundExpression, I specifically did this because only things that derive from BoundExpression should be given a nullability. The info needs to be stored in the BoundNode because that's where the flags live, but it shouldn't be possible to set nullability on anything that isn't a BoundExpression.

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

Please add a PROTOTYPE comment to make the BoundExpression implementation non-virtual.

In reply to: 265247636 [](ancestors = 265247636,264938565)

nit: comment could be more specific to help remind to remove this property

In reply to: 265667308 [](ancestors = 265667308,265247636,264938565)

There's a comment on BoundExpression to make the property non-virtual.

In reply to: 265805034 [](ancestors = 265805034,265667308,265247636,264938565)

cston · 2019-03-14T22:07:02Z

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

+{
+    internal static class PublicNullableFlowStateExtensions
+    {
+        public static NullableFlowState ToInternalFlowState(this CodeAnalysis.NullableFlowState flowState)


ToInternalFlowState [](start = 40, length = 19)

Is this necessary? When do we need to convert from the public values to internal values? #Resolved

Currently, it's being used in the SymbolDisplay visitor and the VerifyTypes helpers. I'll work on removing them in a later PR if it's possible, but it will require removing TypeWithState and TypeSymbolWithAnnotations from a few places so I'll hold off on that for now.

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

333fred · 2019-03-14T23:13:06Z

@jcouv @cston addressed feedback #Resolved

jcouv

LGTM Thanks (iteration 7)

cston · 2019-03-15T15:13:38Z

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

+    public enum NullableAnnotation : byte
+    {
+        NotApplicable = 0,
+        Disabled,      // No information. Think oblivious.


Disabled [](start = 8, length = 8)

Disabled seems misleading because it seems to imply that annotations were ignored rather than the more likely scenario that annotations were never included. I think this should be None or Unset. #Resolved

Noted. These APIs will need to go through a formal design review, so I'll bring up your concerns there. This is what the current plan of record has, so it's what I'm going with for now.

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

cston · 2019-03-15T18:16:22Z

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

+        /// Sets the analyzed nullability of the expression to be the given result.
+        /// </summary>
+        private void SetAnalyzedNullability(BoundExpression expr, VisitResult result, bool? isLvalue = null)
+        {


{ [](start = 8, length = 1)

We should assert the type matches the expected type. (See VisitExpressionWithoutStackGuard.)

Debug.Assert(AreCloseEnough(result.RValueType.Type, expr.Type); ``` #Resolved

See the disabled assertion below.

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

Would AreCloseEnough() allow enabling the assertion now?

In reply to: 266121801 [](ancestors = 266121801,266096745)

Unfortunately not currently: how we handle conversions means that this api gets called with the wrong node.

In reply to: 266126506 [](ancestors = 266126506,266121801,266096745)

cston · 2019-03-15T18:19:25Z

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

+        NotAnnotated, // Type is not annotated - string, int, T (including the case when T is unconstrained).
+        Annotated,    // Type is annotated - string?, T? where T : class; and for int?, T? where T : struct.
+        NotNullable,  // Explicitly set by flow analysis
+        Nullable,     // Explicitly set by flow analysis


Can these be removed? #Resolved

Not yet. I'll remove them when I do the merge from master.

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

cston · 2019-03-15T18:21:50Z

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

-            TopLevelNullabilityMask = TopLevelUnknown,
+            IsSuppressed = 1 << 2,
+
+            // Bit 3: 1 if the node is has maybe-null state, 0 if the node is not null


is has [](start = 36, length = 6)

Typo? #Closed

cston · 2019-03-15T18:29:45Z

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

+            public VisitResult WithType(TypeSymbol newType) => new VisitResult(new TypeWithState(newType, RValueType.State), TypeSymbolWithAnnotations.Create(newType, LValueType.NullableAnnotation));
+            private string GetDebuggerDisplay() => $"{{LValue: {LValueType.GetDebuggerDisplay()}, RValue: {RValueType.GetDebuggerDisplay()}}}";
+
+            public NullabilityInfo ToNullabilityInfo() => new NullabilityInfo(LValueType.NullableAnnotation.ToPublicAnnotation(), RValueType.State.ToPublicFlowState());


ToNullabilityInfo [](start = 35, length = 17)

Consider inlining in the caller. #Closed

cston · 2019-03-15T18:52:09Z

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

            //if (this.State.Reachable) // Consider reachability: see https://github.com/dotnet/roslyn/issues/28798
            {
+                // Because we use an iterative algorithm for binary operators to avoid blowing the stack, we need to ensure
+                // that _currentExpression is correct.


that _currentExpression is correct [](start = 19, length = 34)

The comment appears to be stale. Can the entire comment be removed? #Closed

cston · 2019-03-15T18:55:23Z

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

+
+                // For nested binary operators, this can be the only time they're visited due to explicit stack used in AbstractFlowPass.VisitBinaryOperator,
+                // so we need to set the flow-analyzed type here.
+                SetAnalyzedNullability(binary, _visitResult);


SetAnalyzedNullability(binary, _visitResult); [](start = 16, length = 45)

Is _visitResult different than the return value of InferResultNullability() above? If not, this call can be removed and replaced with updateAnalyzedNullability: true above. #Closed

cston · 2019-03-15T18:55:46Z

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

Please remove blank line. #Closed

cston · 2019-03-15T18:59:41Z

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

            ImmutableArray<RefKind> refKindsOpt,
            ImmutableArray<ParameterSymbol> parameters,
            ImmutableArray<int> argsToParamsOpt,
+            ImmutableArray<BoundExpression> argumentsWithConversions,


ImmutableArray argumentsWithConversions [](start = 12, length = 56)

Not used. #Closed

cston · 2019-03-15T19:13:21Z

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

+            if (!_disableNullabilityAnalysis)
+            {
+                var bag = new DiagnosticBag();
+                Analyze(compilation, node, bag, node.Type.GetDelegateType()?.DelegateInvokeMethod, returnTypes: null, initialState: GetVariableState(), _analyzedNullabilityMapOpt);


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

Are we visiting the lambda body twice in non-error scenarios? #Resolved

Yes. The prototype comment above is tracking trying to remove this.

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

cston · 2019-03-15T19:14:36Z

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

@@ -1260,7 +1527,7 @@ protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpress
        {


{ [](start = 8, length = 1)

Please assert _analyzedNullabilityMapOpt does not contain an entry for node to ensure we are not visiting expressions multiple times. #WontFix

We can't do this: currently, we're explicitly visiting arguments twice.

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

cston · 2019-03-15T19:19:23Z

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

+            }
+
+            // VisitTypeExpression is called recursively, so we need to manually set the previous expression before
+            // updating the result;


Can this comment be removed? #Closed

cston · 2019-03-15T19:24:40Z

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

+            {
+                var verifier = new DebugVerifier(analyzedNullabilityMap);
+                verifier.Visit(node);
+                // Can't just remove nodes from _analyzedNullabilityMap and verify no nodes remaining because nodes can be reused.


// Can't just remove nodes from _analyzedNullabilityMap and verify no nodes remaining because nodes can be reused. [](start = 16, length = 114)

The comment appears to be stale. Can it be removed? #ByDesign

I suppose it's more explaining why I didn't just make _analyzedNullabilityMap mutable and remove nodes from it as I visited them, and am instead just doing a count comparison.

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

cston · 2019-03-15T20:06:13Z

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

        #region ITypeParameterTypeSymbol Members

-        Nullability ITypeParameterSymbol.ReferenceTypeConstraintNullability
+        // PROTOTYPE(nullable-api): Document


// PROTOTYPE(nullable-api): Document [](start = 8, length = 36)

Not needed. The documentation belongs on the interface member. #Closed

cston · 2019-03-15T20:07:53Z

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

        }

-        internal static TypeSymbolWithAnnotations Create(TypeSymbol typeSymbol, NullableAnnotation nullableAnnotation = NullableAnnotation.Unknown, ImmutableArray<CustomModifier> customModifiers = default)
+        internal static TypeSymbolWithAnnotations Create(TypeSymbol typeSymbol, NullableAnnotation CSharpNullableAnnotation = NullableAnnotation.Unknown, ImmutableArray<CustomModifier> customModifiers = default)


CSharpNullableAnnotation [](start = 99, length = 24)

Rename can be reverted. #Closed

cston · 2019-03-15T20:08:07Z

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

        }

-        private static TypeSymbolWithAnnotations CreateNonLazyType(TypeSymbol typeSymbol, NullableAnnotation nullableAnnotation, ImmutableArray<CustomModifier> customModifiers)
+        private static TypeSymbolWithAnnotations CreateNonLazyType(TypeSymbol typeSymbol, NullableAnnotation CSharpNullableAnnotation, ImmutableArray<CustomModifier> customModifiers)


CSharpNullableAnnotation [](start = 109, length = 24)

Can be reverted. #Closed

cston · 2019-03-15T20:08:23Z

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


            TypeSymbol typeSymbol = other.TypeSymbol;
-            NullableAnnotation nullableAnnotation = MergeNullableAnnotation(this.NullableAnnotation, other.NullableAnnotation, variance);
+            NullableAnnotation CSharpNullableAnnotation = MergeNullableAnnotation(this.NullableAnnotation, other.NullableAnnotation, variance);


CSharpNullableAnnotation [](start = 31, length = 24)

Can be reverted. #Closed

cston · 2019-03-15T20:10:11Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

        }

-        [Fact, WorkItem(31370, "https://github.com/dotnet/roslyn/issues/31370")]
+        // PROTOTYPE(nullable-api): wants to change the type of the literal 2 to error type from int


PROTOTYPE(nullable-api): wants to change the type of the literal 2 to error type from int [](start = 11, length = 89)

Does this comment still apply? #Resolved

Yes. This is one of the tests that had errors, so I want to leave it here as I've already spend some time to determine what the actual issue is.

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

cston · 2019-03-15T20:16:52Z

src/Compilers/Test/Utilities/CSharp/CompilationTestUtils.cs

+                    base.Visit(node);
+                    if (node is BoundExpression expr)
+                    {
+                        _map[expr.Syntax] = new TypeWithState(expr.Type, expr.TopLevelNullability.FlowState.ToInternalFlowState()).ToTypeSymbolWithAnnotations();


new TypeWithState(expr.Type, expr.TopLevelNullability.FlowState.ToInternalFlowState()).ToTypeSymbolWithAnnotations() [](start = 44, length = 116)

Consider extracting a local function. #WontFix

This visitor isn't intended to survive past the introduction of the public API. I'll leave this as is.

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

cston · 2019-03-15T20:19:05Z

src/Compilers/Test/Utilities/CSharp/CompilationTestUtils.cs

            }
        }
+
+        private sealed class TopLevelNullabilityRetriever : BoundTreeWalker


TopLevelNullability [](start = 29, length = 19)

Consider renaming since this returns not just top-level nullability but types with top-level and nested nullability. #Closed

333fred added 3 commits March 11, 2019 14:18

Update the stub API to remove Nullability and replace with NullableAn…

a50702a

…notation and NullableFlowState, and update the the rewriter to use that instead of the Nullability enum.

Convert NullableAnnotation to public/private type.

fc95ee8

Add support for recording information about BoundNodes in the nullabl…

a23c233

…e walker and rewriting them.

333fred added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Mar 11, 2019

333fred marked this pull request as ready for review March 12, 2019 00:10

333fred requested review from a team as code owners March 12, 2019 00:10

Address compilation and test issues.

5b91a6c

cston reviewed Mar 12, 2019

View reviewed changes

333fred commented Mar 12, 2019

View reviewed changes

cston reviewed Mar 12, 2019

View reviewed changes

jcouv reviewed Mar 13, 2019

View reviewed changes

cston reviewed Mar 14, 2019

View reviewed changes

Address PR feedback.

97b0006

jcouv approved these changes Mar 14, 2019

View reviewed changes

cston reviewed Mar 15, 2019

View reviewed changes

Address PR feedback.

0ac711a

cston approved these changes Mar 15, 2019

View reviewed changes

Skip test DecimalBinaryOp_03 (dotnet#34199)

cfe02cf

333fred merged commit bfad2d5 into dotnet:features/nullable-api Mar 18, 2019

333fred deleted the rewriter2 branch March 18, 2019 20:08

sharwell mentioned this pull request Sep 8, 2020

Use solution filter instead of multiple solutions #47531

Draft

                                   }
                               }
                           }

		@@ -1260,7 +1527,7 @@ protected override BoundExpression VisitExpressionWithoutStackGuard(BoundExpress
		{

Conversation

333fred commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Mar 12, 2019

Uh oh!

333fred Mar 12, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 12, 2019 • edited by jcouv 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 Mar 13, 2019 • 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 Mar 13, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 13, 2019 • 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 Mar 13, 2019 • 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 Mar 13, 2019 • 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 Mar 13, 2019 • 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 Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

333fred commented Mar 11, 2019 •

edited

Loading

333fred commented Mar 12, 2019 •

edited

Loading

cston commented Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

cston Mar 12, 2019 •

edited by 333fred

Loading

333fred Mar 12, 2019 •

edited by jcouv

Loading

cston Mar 12, 2019 •

edited by jcouv

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited

Loading

jcouv Mar 13, 2019 •

edited by 333fred

Loading

cston Mar 14, 2019 •

edited by 333fred

Loading

333fred commented Mar 14, 2019 •

edited

Loading

cston Mar 15, 2019 •

edited by 333fred

Loading

cston Mar 15, 2019 •

edited by 333fred

Loading

cston Mar 15, 2019 •

edited by 333fred

Loading