Use [NotNullWhenFalse] and [EnsuresNotNull] to drive nullability analysis #26656

jcouv · 2018-05-05T17:51:28Z

This PR implements NotNullWhenFalse (for string.IsNullOrEmpty) and EnsuresNotNull (for ThrowIfNull) attributes, and their effects on nullable analysis.
The EnsuresTrue (for Assert.Debug) and EnsuresFalse attributes will come in a separate PR (jcouv@1a78718).

~~Assert.Debug(condition) is analyzed as if (!condition) throw;.~~
string.IsNullOrEmpty(s) is analyzed as s == null || someBoolean.

Relates to #23259
Fixes #9977 (string.IsNullOrEmpty / IsNullOrWhitespace not recognized)

More design notes

CyrusNajmabadi · 2018-05-05T19:44:38Z

I feel like it would be better if these methods could somehow be annotated. Or if this info could be plugged intot he compiler through some extensibility mechanism. Otherwise, this is just going to be wackamole. #Resolved

jcouv · 2018-05-05T20:06:22Z

@CyrusNajmabadi We're planning to do a little of both. #Resolved

cston · 2018-05-05T20:11:09Z

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

+            return method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Diagnostics_Debug_Assert1)) ||
+                method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Diagnostics_Debug_Assert2)) ||
+                method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Diagnostics_Debug_Assert3)) ||
+                method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_Diagnostics_Debug_Assert4));


Why use WellKnownMember rather than the extra annotations table? (Presumably, we won't need these well-known members once we support annotations.) #Resolved

What we discussed in LDM was that some members would be well-known. That said, maybe that was only for built-in operators?
I like the extra annotations approach. I'll try that. #Closed

cston · 2018-05-05T20:13:33Z

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

+            {
+                VisitTrueWhenNotNull(arguments[0], method.ReturnType.TypeSymbol);
+                _result = method.ReturnType;
+                return null;


Are the last two statements necessary? #Closed

We need to return, since the assertion below would be violated.
We need to set the result, otherwise another assertion breaks (we check that the result is not left as invalid type). #Closed

cston · 2018-05-05T20:17:14Z

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

+                new BoundBinaryOperator(operand.Syntax, BinaryOperatorKind.Equal, operand,
+                    new BoundLiteral(operand.Syntax, ConstantValue.Null, operand.Type),
+                    constantValueOpt: null, methodOpt: null, LookupResultKind.Viable, boolType),
+                new BoundLiteral(operand.Syntax, constantValueOpt: null, boolType));


We're tracking "maybe null" rather than "definitely null". In short, is the || someUnknownBoolean necessary? #Closed

Can this be implemented without synthesizing nodes? #Closed

cston · 2018-05-05T20:29:29Z

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

+
+            // if (!condition) throw;
+            VisitIfStatement(new BoundIfStatement(condition.Syntax, SyntheticBoundNodeFactory.NotCore(condition),
+                new BoundThrowStatement(condition.Syntax, expressionOpt: null), alternativeOpt: null));


Can we implement this visit method without synthesizing nodes, by mimicking the behavior of VisitIfStatement and VisitThrowExpression? #Closed

I didn't want to duplicate that logic, but we can discuss some alternatives. #Closed

cston · 2018-05-05T20:32:23Z

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

+                return false;
+            }
+
+            return method.Equals(compilation.GetWellKnownTypeMember(WellKnownMember.System_String_IsNullOrEmpty));


Please add a // PROTOTYPE comment to replace this with checking annotations. Same for MethodEnsuresTrueWhenExits. #Resolved

cston · 2018-05-05T20:33:52Z

src/Compilers/CSharp/Portable/Lowering/SyntheticBoundNodeFactory.cs

+            return NotCore(expression);
+        }
+
+        internal static BoundUnaryOperator NotCore(BoundExpression expression)


Perhaps change the return type of the original method. #Resolved

cston · 2018-05-05T21:12:23Z

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

@@ -1568,6 +1574,18 @@ public override BoundNode VisitCall(BoundCall node)
                ReplayReadsAndWrites(localFunc, node.Syntax, writes: true);
            }

+            if (MethodEnsuresTrueWhenExits(method) && arguments.Length > 0)
+            {
+                VisitTrueWhenExits(condition: arguments[0]);


The beginning of VisitCall has a check for method.CallsAreOmitted with a comment to skip the visitor in the future when the [Conditional] attribute is not satisfied. But for methods such as Debug.Assert (and similar methods), it seems we should visit the condition argument regardless of whether we visit other arguments. Consider having one test with both TestOptions.DebugDll and TestOptions.ReleaseDll to ensure that. #Closed

That said, visiting the condition regardless of whether the call is skipped may set the state incorrectly. For instance, the following should probably generate a warning in RELEASE. Admittedly this is an edge case.

object? o = null; Debug.Assert((o = new object()) != null); o.ToString(); // may be null

#Resolved

Added test EnsuresNotNull_ConditionalMethodInReleaseMode

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

sharwell · 2018-05-05T21:22:54Z

@AArnott has been dealing with this in the vs-threading analyzers. The additional file content that would be used in this case would be the following:

[System.Diagnostics.Debug]::Assert
[System.Diagnostics.Trace]::Assert

#Resolved

sharwell · 2018-05-05T21:25:18Z

Debug.Assert

This one is interesting. Do you have examples of where it impacts code? #Resolved

AArnott · 2018-05-05T22:35:08Z

The syntax I use was inspired by powershell, incidentally. #Resolved

cston · 2018-05-07T15:04:50Z

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

@@ -11,6 +11,7 @@
 using System.Diagnostics;
 using System.Linq;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
+using Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE;


Perhaps move ExternalAnnotations to Microsoft.CodeAnalysis.CSharp.Symbols. #Resolved

cston · 2018-05-07T15:07:11Z

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

@@ -1577,6 +1606,97 @@ public override BoundNode VisitCall(BoundCall node)
            return null;
        }

+        private BoundExpression TryGetFirstArgument(MethodSymbol method, ImmutableArray<BoundExpression> arguments)


static #Closed

AlekseyTs · 2018-05-14T12:11:51Z

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

+    internal enum AttributeAnnotations
+    {
+        None = 0,
+        NotNullWhenFalse,


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

Please use explicit values for ````[Flags]``` enums #Closed

Done already in follow-up PR https://github.com/dotnet/roslyn/pull/26828/files#diff-d02e2d6d70e84697ca87520824236d04R15 #Resolved

AlekseyTs · 2018-05-14T12:44:42Z

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

+        /// index 0 is used for return type
+        /// other parameters follow
+        /// </summary>
+        internal static (bool hasAny, AttributeAnnotations annotations) GetExtraAttributes(string key, int parameterIndex)


bool hasAny [](start = 25, length = 11)

I am not sure what extra information do we get from hasAny flag. Would it be sufficient to simply check ````AttributeAnnotations``` for 0? #Closed

Perhaps a clear comment might help

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

Added in follow-up PR https://github.com/dotnet/roslyn/pull/26828/files#diff-d02e2d6d70e84697ca87520824236d04R169 #Resolved

AlekseyTs · 2018-05-14T12:45:33Z

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

@@ -385,6 +385,22 @@ internal sealed override ObsoleteAttributeData ObsoleteAttributeData

        internal abstract bool IsCallerMemberName { get; }

+        internal abstract AttributeAnnotations FlowAnalysisAnnotations { get; }
+
+        protected (bool memberHasExtra, AttributeAnnotations annotations) TryGetExtraAttributeAnnotations()


memberHasExtra [](start = 24, length = 14)

The value of AttributeAnnotations member is not obvious to me. #Closed

Perhaps a clear comment might help

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

AlekseyTs · 2018-05-14T12:56:55Z

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

@@ -151,6 +151,11 @@ internal override bool IsCallerMemberName
            get { return _originalParam.IsCallerMemberName; }
        }

+        internal override AttributeAnnotations FlowAnalysisAnnotations
+        {
+            get { return _originalParam.FlowAnalysisAnnotations; }


_originalParam.FlowAnalysisAnnotations [](start = 25, length = 38)

Do we have a test covering this code path? #Closed

Added EnsuresNotNull_BeginInvoke and adjusted behavior (the attributes don't help nullability analysis on BeginInvoke/EndInvoke).

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

AlekseyTs · 2018-05-14T12:58:18Z

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

+                (bool memberHasAny, AttributeAnnotations annotations) = TryGetExtraAttributeAnnotations();
+                if (memberHasAny)
+                {
+                    return annotations;


return annotations [](start = 20, length = 18)

Do we have a test covering this code path? #Closed

I'll add in following PR. It's much easier to test after I add annotations for Debug.Assert method, which has two parameters. #Resolved

Debug.Assert is actually not in the follow-up PR, but the one after that. Added a tracking comment in the meantime: https://github.com/dotnet/roslyn/pull/26828/files#diff-829bf456227af3ee271c7ae37b62ffc0R7056 #Resolved

AlekseyTs · 2018-05-14T12:58:30Z

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

+
+                CommonParameterWellKnownAttributeData attributeData = GetDecodedWellKnownAttributeData();
+
+                return AttributeAnnotations.None


return AttributeAnnotations.None [](start = 16, length = 32)

Do we have a test covering this code path? #Closed

EnsuresNotNull_Indexer among others. I think all nullability analysis tests that have complex parameters (ie. with attributes, or default value) come here. #Resolved

AlekseyTs · 2018-05-14T13:01:01Z

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

+            get
+            {
+                (_, AttributeAnnotations annotations) = TryGetExtraAttributeAnnotations();
+                return annotations;


return annotations; [](start = 16, length = 19)

Do we have tests covering this code path? #Closed

NotNullWhenFalse_String_IsNullOrEmpty_WithoutCorlib and NotNullWhenFalse_String_IsNullOrWhiteSpace_WithoutCorlib for case with annotations.
Most existing nullability tests for case with no annotations returned. #Resolved

AlekseyTs · 2018-05-14T13:06:49Z

src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedParameterSymbol.cs

@@ -153,7 +153,12 @@ internal override bool IsCallerMemberName
            get { return _underlyingParameter.IsCallerMemberName; }
        }

-        public override string GetDocumentationCommentXml(CultureInfo preferredCulture = null, bool expandIncludes = false, CancellationToken cancellationToken = default(CancellationToken))
+        internal override AttributeAnnotations FlowAnalysisAnnotations


internal override AttributeAnnotations FlowAnalysisAnnotations [](start = 8, length = 62)

In general, in symbols only leaf classes should provide implementation and each leaf class should be covered by targeted tests. This implementation is fine for the prototype, but, if we decide to go with this approach, we should follow up. For example, I will not be surprised if retargeting symbols would need special handling, etc. Please add a prototype comment. #Closed

AlekseyTs · 2018-05-14T13:10:56Z

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

@@ -306,19 +306,21 @@ public static SyntaxTree ParseWithRoundTripCheck(string text, CSharpParseOptions
            string ilSource,
            IEnumerable<MetadataReference> references = null,
            CSharpCompilationOptions options = null,
-            bool appendDefaultHeader = true) => CreateCompilationWithILAndMscorlib40(source, ilSource, TargetFramework.Standard, references, options, appendDefaultHeader);
+            CSharpParseOptions parseOptions = null,
+            bool appendDefaultHeader = true) => CreateCompilationWithILAndMscorlib40(source, ilSource, TargetFramework.Standard, references, options, parseOptions, appendDefaultHeader);


bool appendDefaultHeader = true) => CreateCompilationWithILAndMscorlib40(source, ilSource, TargetFramework.Standard, references, options, parseOptions, appendDefaultHeader); [](start = 12, length = 173)

Given a number of parameters for the method and the presence of optional ones, the expression body form is "unreadable". Consider converting this method to the regular body form. #Closed

AlekseyTs · 2018-05-14T13:17:17Z

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

+
+                // We'll ignore NotNullWhenFalse that is misused in metadata
+                if ((annotations & AttributeAnnotations.NotNullWhenFalse) != 0 &&
+                    parameter.ContainingSymbol.GetTypeOrReturnType().SpecialType != SpecialType.System_Boolean)


parameter [](start = 20, length = 9)

Should we use original definition for this check? Is generic case covered by tests? #Closed

Added NotNullWhenFalse_RequiresBoolReturn_OnGenericMethod. The behavior of checking the specialized definition, rather than the original definition, seem ok.

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

AlekseyTs · 2018-05-14T13:17:43Z

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

+
+                // We'll ignore EnsuresNotNull that is misused in metadata
+                if ((annotations & AttributeAnnotations.EnsuresNotNull) != 0 &&
+                    (parameter.Type?.IsValueType != false || parameter.IsParams))


parameter [](start = 21, length = 9)

Should we use original definition for this check? Is generic case covered by tests? #Closed

I'll add tests.
To do that more easily, I'll pull the change to remove the warning for misused attributes in this PR. That'll also address some other feedback. #Resolved

Added tests EnsuresNotNull_Generic_WithRefType, EnsuresNotNull_Generic_WithValueType and EnsuresNotNull_Generic_WithUnconstrainedGenericType.

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

AlekseyTs · 2018-05-14T13:18:56Z

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

+            }
+            else if (attribute.IsTargetAttribute(this, AttributeDescription.EnsuresNotNullAttribute))
+            {
+                if (this.Type.IsValueType)


this.Type.IsValueType [](start = 20, length = 21)

What if type is an unconstraint generic type parameter? #Closed

Unconstrained generic type gives IsValueType as false and no warning is produced.
That said, I'm removing those warnings in follow-up PR, from discussion with Neal and Chuck (those warning create compat issue once the attributes are released, if someone uses the attribute with C# 7 compiler then upgrades to C# 8 compiler).

Update: I've pulled the change (removing warnings for misused diagnostics) into this PR. #Resolved

AlekseyTs · 2018-05-14T13:19:06Z

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

@@ -605,6 +623,28 @@ internal override void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArgu
                // NullableAttribute should not be set explicitly.
                arguments.Diagnostics.Add(ErrorCode.ERR_ExplicitNullableAttribute, arguments.AttributeSyntaxOpt.Location);
            }
+            else if (attribute.IsTargetAttribute(this, AttributeDescription.NotNullWhenFalseAttribute))
+            {
+                if (this.ContainingSymbol.GetTypeOrReturnType().SpecialType == SpecialType.System_Boolean)


this.ContainingSymbol.GetTypeOrReturnType() [](start = 20, length = 43)

What if type is an unconstraint generic type parameter? #Closed

AlekseyTs · 2018-05-14T13:43:16Z

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

+                else if (notNullWhenFalse)
+                {
+                    // We'll use the WhenTrue/False states to represent whether the invocation returns true/false
+                    Split();


Split(); [](start = 20, length = 8)

I think think this might be a wrong place to do the Split, it should be done when we are visiting invocation, not the arguments. #Closed

I don't see a functional advantage to splitting early. Do you mean that would be better for clarity? #Resolved

AlekseyTs · 2018-05-14T13:58:48Z

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

@@ -3573,6 +3748,19 @@ public bool Reachable
                    return _knownNullState.Capacity > 0;
                }
            }
+
+            public override string ToString()


ToString [](start = 35, length = 8)

Is this for debugging purposes? Consider adding a specialized helper for that rather than to changing ToString. #Closed

I've changed that to use debugger display method instead, in follow-up PR https://github.com/dotnet/roslyn/pull/26828/files#diff-e3717bc6a837e218d79277c0e45f213aR3791 #Resolved

AlekseyTs · 2018-05-14T14:04:10Z

Done with review pass (iteration 13) #Closed

AlekseyTs · 2018-05-14T16:44:02Z

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

+                    }
+                    else
+                    {
+                        this.State[slot] = true;


this.State[slot] = true [](start = 24, length = 23)

It feels like in general case prior arguments should be able to take advantage of this information. #Closed

AlekseyTs · 2018-05-14T16:45:44Z

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

+                    }
+                    else
+                    {
+                        this.State[slot] = true;


this.State[slot] = true; [](start = 24, length = 24)

It feels like in general case this information should flow into type argument inference for the method, but it doesn't look like it does. #Closed

Added tests EnsuresNotNull_BackEffect, EnsuresNotNull_ForwardEffect(s), EnsuresNotNull_TypeInference

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

AlekseyTs · 2018-05-14T16:51:04Z

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

+{
+    public void Main(string? s)
+    {
+        if (MyIsNullOrEmpty(s))


MyIsNullOrEmpty(s) [](start = 12, length = 18)

Please add a test for conditions of the form MyIsNullOrEmpty(s) == true and MyIsNullOrEmpty(s) == false. And also with comparison operands in different order. #Closed

That is indeed broken. Thanks!
Add tests NotNullWhenFalse_ComparedToTrue and NotNullWhenFalse_ComparedToFalse

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

cston · 2018-05-14T22:18:06Z

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

+    {
+        if (false == MyIsNullOrEmpty(s))
+        {
+            s.ToString(); // warn


jcouv · 2018-05-15T04:52:40Z

@AlekseyTs This PR is ready for another look. Thanks

AlekseyTs

LGTM (iteration 18)

jcouv · 2018-05-15T18:25:02Z

test windows_release_unit64_prtest please

jcouv added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels May 5, 2018

jcouv added this to the 16.0 milestone May 5, 2018

jcouv self-assigned this May 5, 2018

jcouv requested a review from a team as a code owner May 5, 2018 17:51

jcouv force-pushed the nullable-assert branch 2 times, most recently from b6bb6a0 to ad53fbf Compare May 5, 2018 18:00

jcouv changed the title ~~Add well-known Debug.Assert method~~ Add well-known Debug.Assert and IsNullOrEmpty methods May 5, 2018

jcouv force-pushed the nullable-assert branch from 93e3984 to 4bdc40d Compare May 5, 2018 19:36

cston reviewed May 5, 2018

View reviewed changes

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

jcouv force-pushed the nullable-assert branch 2 times, most recently from d8a7a5d to 4e6e56a Compare May 6, 2018 17:42

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 6, 2018

jcouv mentioned this pull request May 6, 2018

[Work items] Nullable reference types #22152

Closed

jcouv force-pushed the nullable-assert branch from 4e6e56a to e66b11f Compare May 6, 2018 18:13

jcouv changed the title ~~Add well-known Debug.Assert and IsNullOrEmpty methods~~ Add extra attributes to Debug.Assert and IsNullOrEmpty for nullability analysis May 6, 2018

cston reviewed May 7, 2018

View reviewed changes

AlekseyTs reviewed May 14, 2018

View reviewed changes

jcouv added 2 commits May 14, 2018 08:45

WIP

7c7573d

Remove warning for misused attributes

2036a25

AlekseyTs reviewed May 14, 2018

View reviewed changes

jcouv added 2 commits May 14, 2018 14:55

Address PR feedback from Aleksey

aea98d8

Missed change

fa24db3

cston reviewed May 14, 2018

View reviewed changes

Fix comment and add test

266b24c

AlekseyTs approved these changes May 15, 2018

View reviewed changes

jcouv merged commit 4d28557 into dotnet:features/NullableReferenceTypes May 15, 2018

jcouv deleted the nullable-assert branch May 15, 2018 19:11


		CommonParameterWellKnownAttributeData attributeData = GetDecodedWellKnownAttributeData();

		return AttributeAnnotations.None

Use [NotNullWhenFalse] and [EnsuresNotNull] to drive nullability analysis #26656

Use [NotNullWhenFalse] and [EnsuresNotNull] to drive nullability analysis #26656

Conversation

jcouv commented May 5, 2018 • edited Loading

CyrusNajmabadi commented May 5, 2018 • edited by jcouv Loading

jcouv commented May 5, 2018 • edited Loading

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 5, 2018 • edited Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 5, 2018 • edited Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 5, 2018 • edited Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 9, 2018

Choose a reason for hiding this comment

sharwell commented May 5, 2018 • edited by jcouv Loading

sharwell commented May 5, 2018 • edited by jcouv Loading

AArnott commented May 5, 2018 • edited by jcouv Loading

cston May 7, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 7, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv May 14, 2018

Choose a reason for hiding this comment

AlekseyTs May 14, 2018 • edited Loading

jcouv commented May 5, 2018 •

edited

Loading

CyrusNajmabadi commented May 5, 2018 •

edited by jcouv

Loading

jcouv commented May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

jcouv May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

jcouv May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

jcouv May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

sharwell commented May 5, 2018 •

edited by jcouv

Loading

sharwell commented May 5, 2018 •

edited by jcouv

Loading

AArnott commented May 5, 2018 •

edited by jcouv

Loading

cston May 7, 2018 •

edited by jcouv

Loading

cston May 7, 2018 •

edited by jcouv

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs commented May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading

jcouv May 14, 2018 •

edited

Loading

AlekseyTs May 14, 2018 •

edited

Loading