Allow dispose pattern to be implemented via extension method #29307

chsienki · 2018-08-14T23:28:21Z

Add option to FindPatternMethod to allow searching for extension methods
Generate correct call to dispose when it is an extension method
Add various tests

jcouv · 2018-08-14T23:58:45Z

build/scripts/build-utils.ps1

@@ -148,6 +148,7 @@ function Ensure-DotnetSdk() {
        Create-Directory $cliDir
        Create-Directory $toolsDir
        $destFile = Join-Path $toolsDir "dotnet-install.ps1"
+        [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12


[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 [](start = 8, length = 79)

Not sure what happened in this file. Should be reverted.

Update: it looks like there is a TLS 1.2 commit that doesn't belong in your PR.

I pulled this in from master, as the build doesn't work without it

This is a necessary change. The servers that we get a lot of our bootstrapping tools from recently changed to require Tls12 which is not the default. Every branch has needed to cherry-pick this fix in order to pass CI.

I think we should just perform a merge from master into this branch. I'll queue up a PR to add it to the automerge list as well.

jaredpar · 2018-08-15T16:51:48Z

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

                                                SyntaxTree syntaxTree, MessageID messageID)
        {
            Debug.Assert(lookupResult.IsClear);
-
+
+            //prototype:


Nit: use PROTOTYPE. Stands out more.

jaredpar · 2018-08-15T16:54:14Z

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

                }
            }

-            MethodSymbol patternMethod = PerformPatternOverloadResolution(patternType, candidateMethods, syntaxExpr, warningsOnly, diagnostics, syntaxTree, messageID);
-
+            patternMethod = PerformPatternOverloadResolution(patternType, candidateMethods, syntaxExpr, warningsOnly, diagnostics, syntaxTree, messageID);
            candidateMethods.Free();


Double blank line below.

jaredpar · 2018-08-15T16:57:24Z

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

-
+
+            //prototype:
+            LookupOptions lookupOptions = (searchExtensionMethods) ? LookupOptions.MustBeInvocableIfMember : LookupOptions.Default;


MustBeInvocableIfMember allows for items like fields / properties where the type is dynamic or it's a delegate instance. Please add tests to ensure this doesn't work with the pattern based disposed feature.

jaredpar · 2018-08-15T16:58:02Z

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

                originalBinder: this,
                diagnose: false,
                useSiteDiagnostics: ref useSiteDiagnostics);

            diagnostics.Add(syntaxExpr, useSiteDiagnostics);

-            if (!lookupResult.IsMultiViable)
+            MethodSymbol patternMethod = null;


r [](start = 30, length = 1)

This declaration can be moved to the initialization on line 3252. It has no usage before then.

jaredpar · 2018-08-15T16:59:43Z

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

            candidateMethods.Free();

+
+            // if we failed to find a method via normal lookup, see if there is an extension method that satisifies it
+            if (patternMethod == null && searchExtensionMethods)


Use pattern is null instead of == null if you want to determine if the reference has the value null.

jaredpar · 2018-08-15T17:01:18Z

                    ReportPatternWarning(diagnostics, patternType, member, syntaxExpr, messageID);

Won't this warn when there is both a field (via dynamic) and method candidate? I would assume in that case we would prefer the method candidate in the case it was more applicable.

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs:3234 in 2abef17. [](commit_id = 2abef17, deletion_comment = False)

jaredpar · 2018-08-15T17:01:57Z

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

+            if (patternMethod == null && searchExtensionMethods)
+            {
+                var extLookupResult = LookupResult.GetInstance();
+                this.LookupExtensionMethods(extLookupResult, methodName, 0, lookupOptions, ref useSiteDiagnostics);


Please used a named argument for 0 here to give it meaning at the call site.

jaredpar · 2018-08-15T17:02:53Z

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

+
+                extLookupResult.Free();
+
+                if (patternMethod == null)


Use is null here as well.

agocke · 2018-08-15T17:20:15Z

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

@@ -702,7 +702,7 @@ internal MethodSymbol TryFindDisposePatternMethod(TypeSymbol exprType, SyntaxNod
        {
            LookupResult lookupResult = LookupResult.GetInstance();

-            MethodSymbol disposeMethod = FindPatternMethod(exprType, WellKnownMemberNames.DisposeMethodName, lookupResult, syntaxNode, warningsOnly: true, diagnostics, syntaxNode.SyntaxTree, MessageID.IDS_Disposable);
+            MethodSymbol disposeMethod = FindPatternMethod(exprType, WellKnownMemberNames.DisposeMethodName, lookupResult, syntaxNode, searchExtensionMethods: true, warningsOnly: true, diagnostics, syntaxNode.SyntaxTree, MessageID.IDS_Disposable);


Could you reformat this like:

exprType, WellKnownMemberNames.DisposeMethodName, lookupResult, syntaxNode, searchExtensionMethods: true, warningsOnly: true, diagnostics, syntaxNode.SyntaxTree, MessageID.IDS_Disposable)

The line is so long right now that it's pretty unreadable.

agocke · 2018-08-15T17:22:50Z

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

@@ -3184,15 +3184,19 @@ internal virtual ImmutableArray<LabelSymbol> Labels
        /// <param name="patternType">Type to search.</param>
        /// <param name="methodName">Method to search for.</param>
        /// <param name="lookupResult">Passed in for reusability.</param>
+        /// <param name="searchExtensionMethods">True if the lookup should consider extension methods as viable candidates</param>
        /// <param name="warningsOnly">True if failures should result in warnings; false if they should result in errors.</param>
        /// <param name="diagnostics">Populated with binding diagnostics.</param>
        /// <returns>The desired method or null.</returns>
        internal MethodSymbol FindPatternMethod(TypeSymbol patternType, string methodName, LookupResult lookupResult,


I think we'll end up needing to split this out to handle different patterns differently. For instance, the GetEnumerator lookup will be like

Search on type

Look for conversion to IEnumerable

Look for extension method

while Dispose will be like

Look for conversion

Look for instance method

Look for extension method

I'm not sure we can really share much between these two.

agocke · 2018-08-15T17:24:52Z

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

+
+            //prototype:
+            LookupOptions lookupOptions = (searchExtensionMethods) ? LookupOptions.MustBeInvocableIfMember : LookupOptions.Default;
+
            // Not using LookupOptions.MustBeInvocableMember because we don't want the corresponding lookup error.


This comment seems relevant. It sounds like there may be an unwanted diagnostic added.

agocke · 2018-08-15T17:27:40Z

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

-            if (!lookupResult.IsMultiViable)
+            MethodSymbol patternMethod = null;
+
+            if (!lookupResult.IsMultiViable && !searchExtensionMethods)
            {
                ReportPatternMemberLookupDiagnostics(lookupResult, patternType, methodName, syntaxExpr, warningsOnly, diagnostics, messageID);


The return null on line 3231 looks suspicious -- I don't follow why the existence of a property with the Dispose name should disallow calling an identical Dispose method. Maybe this is a question we need to take to LDM

agocke · 2018-08-15T17:34:21Z

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

+                // if there are multiple extension methods found it is ambiguous which one to use 
+                if (extLookupResult.IsMultiViable && extLookupResult.Symbols.Count > 1)
+                {
+                    diagnostics.Add(ErrorCode.WRN_PatternIsAmbiguous, syntaxExpr.Location, patternType, messageID.Localize(),


Seems like the instance lookup just tries to find something with no parameters. Why can't we do that here?

This will probably break for people using both a Dispose() and a Dispose(bool) method, which is a pretty common pattern.

agocke · 2018-08-15T17:36:15Z

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

@@ -768,8 +768,8 @@ private bool SatisfiesGetEnumeratorPattern(ref ForEachEnumeratorInfo.Builder bui
        {
            LookupResult lookupResult = LookupResult.GetInstance();
            MethodSymbol getEnumeratorMethod = FindPatternMethod(collectionExprType, GetEnumeratorMethodName, lookupResult,
-                                                                 _syntax.Expression, warningsOnly: true, diagnostics: diagnostics,
-                                                                 _syntax.SyntaxTree, MessageID.IDS_Collection);
+                                                     _syntax.Expression, searchExtensionMethods:false, warningsOnly: true, diagnostics: diagnostics,


Parameters on newlines should either be indented once into the new line or aligned with the first character after the opening parenthesis.

agocke · 2018-08-15T17:36:26Z

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

@@ -872,8 +872,7 @@ private bool SatisfiesForEachPattern(ref ForEachEnumeratorInfo.Builder builder,
                lookupResult.Clear(); // Reuse the same LookupResult

                MethodSymbol moveNextMethodCandidate = FindPatternMethod(enumeratorType, MoveNextMethodName, lookupResult, _syntax.Expression,
-                                                                        warningsOnly: false, diagnostics, _syntax.SyntaxTree, MessageID.IDS_Collection);
-
+                                                                        searchExtensionMethods:false, warningsOnly: false, diagnostics, _syntax.SyntaxTree, MessageID.IDS_Collection);


Same thing about parameter alignment.

agocke · 2018-08-15T17:38:02Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs

@@ -293,7 +293,14 @@ private BoundStatement RewriteUsingStatementTryFinally(SyntaxNode syntax, BoundB

            if ((!(methodOpt is null)) || Binder.TryGetSpecialTypeMember(_compilation, SpecialMember.System_IDisposable__Dispose, syntax, _diagnostics, out methodOpt))
            {
-                disposeCall = BoundCall.Synthesized(syntax, disposedExpression, methodOpt);
+                if (methodOpt.IsExtensionMethod)


Consider using a ? : operator here. If it extends to multiple lines, our formatting rule is each token on a new line e.g.,

var expr = cond ? true-consequence : false-consequence;

cston · 2018-08-15T21:21:12Z

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

+            if (patternMethod == null && searchExtensionMethods)
+            {
+                var extLookupResult = LookupResult.GetInstance();
+                this.LookupExtensionMethods(extLookupResult, methodName, 0, lookupOptions, ref useSiteDiagnostics);


LookupExtensionMethods [](start = 21, length = 22)

Extension method lookup should stop at the first scope that contains an appropriate overload, so we should call LookupExtensionMethodsInSingleBinder for each scope, stopping at the first match.

See Binder.BindExtensionMethod().

cston · 2018-08-15T21:24:02Z

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

+                    diagnostics.Add(ErrorCode.WRN_PatternIsAmbiguous, syntaxExpr.Location, patternType, messageID.Localize(),
+                      (MethodSymbol)extLookupResult.Symbols[0], (MethodSymbol)extLookupResult.Symbols[1]);
+                }
+                else if (extLookupResult.IsSingleViable && extLookupResult.SingleSymbolOrDefault.GetParameters().Length == 1 && extLookupResult.SingleSymbolOrDefault is MethodSymbol method)


extLookupResult.SingleSymbolOrDefault is MethodSymbol method [](start = 128, length = 60)

Consider moving this expression before the previous expression so that method can be used in that expression.

And use method.Parameters rather than GetParameters().

What is the difference between the two?

GetParameters() is an extension method on Symbol which calls MethodSymbol.Parameters if this is a method. If we already know we have a method, we should use Parameters directly.

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

cston · 2018-08-15T21:40:03Z

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

+                if (extLookupResult.IsMultiViable && extLookupResult.Symbols.Count > 1)
+                {
+                    diagnostics.Add(ErrorCode.WRN_PatternIsAmbiguous, syntaxExpr.Location, patternType, messageID.Localize(),
+                      (MethodSymbol)extLookupResult.Symbols[0], (MethodSymbol)extLookupResult.Symbols[1]);


(MethodSymbol) [](start = 22, length = 14)

Casts are not needed.

chsienki · 2018-09-14T17:37:56Z

@dotnet/roslyn-compiler Ready for second pass review, please

- Add option to FindPatternMethod to allow searching for extension methods - Generate correct call to dispose when it is an extenion method - Add various tests

- Rename lookup pattern to strict, for the previous behavior still used by GetEnumerator/MoveNext - Add a relaxed lookup with more relaxed semantics that will keep searching, even if it finds an invalid candidate - Add a lookup extension method that can be used to resolve patterns via extension methods - Update TryFindDisposePattern to search first via relaxed lookup, then extension, handling the diagnostics appropriately - Address previous PR comments

cston · 2018-10-01T15:35:06Z

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

+                        }
+                    }
+
+                    if (candidateMethods.Count > 1)


if (candidateMethods.Count > 1) [](start = 20, length = 31)

Are we checking the this argument type? It seems reasonable to have multiple Dispose extension methods in the same scope, with different this arguments.

Hmm, that's a great point. We're not, and the case you suggested fails. I've added the check that it's a valid receiver method and some tests to ensure we correctly handle that behaviour.

…o cover.

cston · 2018-10-01T20:29:22Z

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

+                    foreach (var symbol in lookupResult.Symbols)
+                    {
+                        //PROTOTYPE: for now, just check for the single this param. We probably want to also  allow e.g. dispose(this, int a = 4, params object[] args)
+                        if (symbol is MethodSymbol method && method.ParameterCount == 1 && !(method.ReduceExtensionMethod(patternType) is null))


ReduceExtensionMethod [](start = 100, length = 21)

It looks like this relies on ReducedExtensionMethodSymbol.Create. Consider replicating two interesting calls from that method here. Specifically:

method = method.InferExtensionMethodTypeArguments( patternType, Compilation, ref useSiteDiagnostics); if ((object)method != null && Conversions.ConvertExtensionMethodThis( method.Parameters[0].Type, patternType, ref useSiteDiagnostics)) { ... }

There are a couple of reasons: 1. We probably want to return the constructed method rather than the original definition if the method is generic. 2. We should include the method in the candidate set even if there are use-site errors.

cston · 2018-10-01T20:38:26Z

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

+        }
+
+        [Fact]
+        public void UsingPatternWithLessDerivedTarget()


UsingPatternWithLessDerivedTarget [](start = 20, length = 33)

Please add a test with Dispose extension methods for both C1 and C2 to verify overload resolution when calling with an instance of each.

Please also test a mix of generic and non-generic methods:

static void Dispose(this C1 c) { } static void Dispose<T>(this T t) where T : C2 { }

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

Other variants:

static void Dispose(in SomeStruct) static void Dispose(ref SomeStruct)

In reply to: 221754221 [](ancestors = 221754221,221748751)

AlekseyTs · 2018-10-01T21:50:04Z

It feels like this change requires changes to IOpertion and Control Flow Graph generation

- Remove the custom lookup code, and just attempt to bind using BindExtensionMethod - Add more tests

chsienki · 2018-10-02T23:08:23Z

I've updated the logic in FindPatternExtensionMethod to use BindExtensionMethod: this gives us all the wanted behaviour without needing to re-implement it a second time.; we do something similar during deconstruct pattern lookup.

This branch has been trying to share the lookup logic of the dispose pattern with the GetEnumerator pattern, but they're actually fairly different. I think it would make sense to undo that, and instead base the Dispose lookup on the Deconstruct pattern. Rather than trying to do that here, I'd like to get this PR in, and we can re-factor out in a separate second pass.

cston · 2018-10-03T15:43:28Z

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

+        /// <param name="diagnostics">Populated with binding diagnostics.</param>
+        /// <param name="messageID">The ID of the message to use when reporting diagnostics</param>
+        /// <returns>The desired method or null.</returns>
+        private MethodSymbol FindPatternExtensionMethod(BoundExpression typeExpr, string methodName, LookupResult lookupResult,


LookupResult lookupResult [](start = 101, length = 25)

Not used.

cston · 2018-10-03T15:47:22Z

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

+    }
+}";
+            var compilation = CreateCompilation(source).VerifyDiagnostics();
+        }


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

Consider testing Dispose(this object o, params object[] args)

cston · 2018-10-03T15:48:59Z

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

+    }
+}";
+            var compilation = CreateCompilation(source).VerifyDiagnostics();
+        }


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

Consider checking IL of Main or some verification of overload resolution.

- Remove unused parameter - Verify IL of overloads to ensure we're calling the correct methods - Add some extra tests of interesting behavior

cston · 2018-10-04T17:42:51Z

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

+            ImmutableArray<TypeSymbol> typeArguments = default;
+            MethodSymbol patternMethod = null;
+
+            var resolution = BindExtensionMethod(syntaxExpr, WellKnownMemberNames.DisposeMethodName, arguments, typeExpr, typeArguments, isMethodGroupConversion: false, returnRefKind: RefKind.None, returnType: null);


WellKnownMemberNames.DisposeMethodName [](start = 61, length = 38)

methodName

cston · 2018-10-04T17:43:23Z

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

+            ImmutableArray<TypeSymbol> typeArguments = default;
+            MethodSymbol patternMethod = null;
+
+            var resolution = BindExtensionMethod(syntaxExpr, WellKnownMemberNames.DisposeMethodName, arguments, typeExpr, typeArguments, isMethodGroupConversion: false, returnRefKind: RefKind.None, returnType: null);


typeArguments [](start = 122, length = 13)

Consider using typeArguments: default rather than creating a local.

cston · 2018-10-04T17:45:01Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs

@@ -293,7 +293,9 @@ private BoundStatement RewriteUsingStatementTryFinally(SyntaxNode syntax, BoundB

            if ((!(methodOpt is null)) || Binder.TryGetSpecialTypeMember(_compilation, SpecialMember.System_IDisposable__Dispose, syntax, _diagnostics, out methodOpt))
            {
-                disposeCall = BoundCall.Synthesized(syntax, disposedExpression, methodOpt);
+                disposeCall = methodOpt.IsExtensionMethod
+                    ? BoundCall.Synthesized(syntax, disposedExpression, methodOpt, local)


disposedExpression [](start = 52, length = 18)

null

cston · 2018-10-04T17:49:05Z

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

+                //         using (C2 c = new C2())
+                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "C2 c = new C2()").WithArguments("C2").WithLocation(22, 16)
+                );
+        }


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

Consider testing with inaccessible instance method and accessible extension method.

- Don't pass receiver to synthesized bound node in extension case - Don't hard code dispose method name in find pattern extension method - Add test for inaccesible instance method & public extension method

cston · 2018-10-04T18:24:43Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingStatementTests.cs

+        }
+    }
+}";
+            CompileAndVerify(source).VerifyIL("C3.Main()", @"


CompileAndVerify [](start = 12, length = 16)

Please include expectedOutput: "..." and include Console.WriteLine(...) in Dispose, to verify execution.

- ExpectedOutput tests the same thing in a clearer way - CodeGenTests are testing the correct IL is emitted

…29307) * Allow dispose pattern to be implemented via extension method: - Split pattern lookups into strict, relaxed and extension paths - Use relaxed, followed by extension for dispose lookup - Generate correct call to dispose when it is an extension method - Add tests

chsienki requested review from a team as code owners August 14, 2018 23:28

jcouv reviewed Aug 14, 2018

View reviewed changes

jcouv added Area-Compilers New Language Feature - enhanced using Using pattern and declaration labels Aug 14, 2018

jcouv added this to the 16.0 milestone Aug 15, 2018

jaredpar reviewed Aug 15, 2018

View reviewed changes

agocke reviewed Aug 15, 2018

View reviewed changes

cston reviewed Aug 15, 2018

View reviewed changes

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

chsienki force-pushed the dev/chsienki/dispose_pattern_ext_method branch from 8d309d5 to b9e6443 Compare September 20, 2018 22:05

chsienki requested review from a team as code owners September 20, 2018 22:05

chsienki force-pushed the dev/chsienki/dispose_pattern_ext_method branch from b9e6443 to 5c1f5fc Compare September 24, 2018 18:24

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

chsienki added 4 commits September 28, 2018 11:57

Allow dispose pattern to be implemented via extension method:

846659f

- Add option to FindPatternMethod to allow searching for extension methods - Generate correct call to dispose when it is an extenion method - Add various tests

Undo whitespace changes

5a9a77b

Add doc comment

f7cad9b

cston reviewed Oct 1, 2018

View reviewed changes

chsienki added 3 commits October 1, 2018 13:03

Add test to check extension method scopes

2ea1717

Test the receiver type of any possible extension methods. Add tests t…

a58662c

…o cover.

Fix typo

3716db9

cston reviewed Oct 1, 2018

View reviewed changes

Change extension pattern binding to use BindExtensionMethod:

33e348e

- Remove the custom lookup code, and just attempt to bind using BindExtensionMethod - Add more tests

Fix whitespace

2eba78b

cston reviewed Oct 3, 2018

View reviewed changes

cston approved these changes Oct 3, 2018

View reviewed changes

chsienki added 2 commits October 3, 2018 12:07

PR Feedback:

f078df1

- Remove unused parameter - Verify IL of overloads to ensure we're calling the correct methods - Add some extra tests of interesting behavior

Add extra test with mix of generic and more derived overloads

7971f2f

jaredpar approved these changes Oct 3, 2018

View reviewed changes

Undo whitespace change

5148bba

cston reviewed Oct 4, 2018

View reviewed changes

PR Feedback:

dab7f44

- Don't pass receiver to synthesized bound node in extension case - Don't hard code dispose method name in find pattern extension method - Add test for inaccesible instance method & public extension method

cston reviewed Oct 4, 2018

View reviewed changes

chsienki added 2 commits October 4, 2018 12:23

Print output during tests, and verify as expected

837e126

Drop ILVerify from using statement tests

cc99131

- ExpectedOutput tests the same thing in a clearer way - CodeGenTests are testing the correct IL is emitted

cston approved these changes Oct 4, 2018

View reviewed changes

chsienki merged commit 244e01c into dotnet:features/enhanced-using Oct 4, 2018

chsienki deleted the dev/chsienki/dispose_pattern_ext_method branch October 4, 2018 21:28



		//prototype:
		LookupOptions lookupOptions = (searchExtensionMethods) ? LookupOptions.MustBeInvocableIfMember : LookupOptions.Default;

Allow dispose pattern to be implemented via extension method #29307

Allow dispose pattern to be implemented via extension method #29307

Conversation

chsienki commented Aug 14, 2018

jcouv Aug 14, 2018 • edited

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

jaredpar commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Aug 15, 2018 • edited

Choose a reason for hiding this comment

agocke Aug 15, 2018 • edited

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

cston Aug 15, 2018 • edited

Choose a reason for hiding this comment

cston Aug 15, 2018 • edited

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

chsienki commented Sep 14, 2018

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

AlekseyTs commented Oct 1, 2018

chsienki commented Oct 2, 2018

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 Aug 14, 2018 •

edited

agocke Aug 15, 2018 •

edited

agocke Aug 15, 2018 •

edited

cston Aug 15, 2018 •

edited

cston Aug 15, 2018 •

edited