Refactor declaration emitter into declaration transformer #21930

weswigham · 2018-02-14T03:45:03Z

This is some prerequisite work we wanted to get done before tackling some of our more complex declaration emit requests (ie, declaration implementation maps). And y'know what? It's an overall reduction in the number of SLOC in the codebase 😉

Notable nonfunctional changes:

No excess whitespace for global augmentations, empty objects, or empty interfaces (due to using the unified printer, decided it wasn't worth adding settings to emulate the old undesirable behavior)
Destructuring patterns are now printed with surrounding whitespace (same as above)
No trailing commas on enums (same as above)
Optional parameter properties now show their optionality on the generated property, in addition to the parameter (I think this is good, but am unsure - can easily change if need be)
Erroneous mixed-mode projects (ie, a module: commonjs + outFile) now emit a triple-slash reference in their declaration output, instead of no reference to the other mode file (while valid ones continue to embed a module declaration) - the reason being that in this scenario the modules aren't in the bundled emit list, so aren't in the list of files to remove triple-slash references for (this is potentially more correct, and now a global augmentation in the module referenced would get picked up, assuming the module was emitted unbundled... In any case, it's an error, so our exact behavior there today isn't terribly important)

Also, some small functional changes:

Errors due to aliasing are less obviously stateful (bugfix)
- This should prevent emit more, but does not because of how we calculate diagnostics.
any is more aggressively appended to user supplied types which would otherwise be an error under noImplicitAny mode- fixes Certain types can produce implicit 'any' declaration output #21614
Private methods now print as a private member with no signature/types/type arguments, as was arguably expected, but not what was done - fixes Private methods in d.ts files should be declared with rest parameters to make them more stable #10438

It's worth stating, since some people have mentioned that they think this is the way to allowing declaration emit for JS files: While the transform does work on JS (though it filters js out right now to retain old behavior), it does not handle JS-specific patterns our binder picks up, like IIFEs, psuedo-namespaces, non-es6 classes, and more. It is quite decisively a TS-pattern declaration transformer. It's highly likely that the correct way to go about creating a declaration file for a JS file is to synthesize the entire declaration file directly from the shape of the exports object (instead of the original declarations), in order to smooth over all the odd patterns we recognize in JS but not in a .d.ts, as a separate transform (naturally, they'd need to call into one another for bundled emit).

weswigham · 2018-02-14T03:49:05Z

src/compiler/factory.ts

@@ -3182,8 +3258,7 @@ namespace ts {
                    finally { if (e) throw e.error; }
                }
                return ar;
-            };
-        `
+            };`


Curious why all these ending backticks got moved up a line? Simple reason. I updated guessIndentation to infer whitespace from whitespace-only lines (but not empty lines), as when converting comments into synthetic ones (as on accessors), once you ignore the first line you can often be left with just whitespace! In any case, this brought to my attention the fact that some of our helpers had their trailing backtick on a new line tabbed one less indentation level than the rest of the lines in the helper. We were a bit inconsistent about this, so I just updated the helpers that did not to have the backtick after the last nonwhitespace character.

weswigham · 2018-02-14T22:28:17Z

For the curious, I ran some perf baselines:

While it was strangely high-variance for a 50(!) iteration run that I ran overnight, for the projects which have emit, it looks like, as is, this change doesn't affect either the emit time or the overall time much at all (which likely means there's room to gain some performance, since this wasn't authored with maximum perf as a priority, but instead focuses on replicating old behavior).

alfaproject · 2018-02-16T20:46:24Z

Trailing commas are good, tho?

rbuckton · 2018-02-17T01:55:02Z

Windows performance numbers are always erratic for emit since it incorporates file I/O and for some reason file I/O via libuv on Windows never has stable numbers compared to other OSes.

DanielRosenwasser · 2018-02-21T00:36:35Z

Optional parameter properties now show their optionality on the generated property, in addition to the parameter (I think this is good, but am unsure - can easily change if need be)

This is strictly a bug fix IMO.

DanielRosenwasser · 2018-02-20T23:42:35Z

src/compiler/comments.ts

@@ -260,7 +260,21 @@ namespace ts {
            }
        }

+        function isJSDocLikeText(text: string, start: number) {


Deduplicate this with isJsDocStart

Can't. They behave meaningfully differently. isJSDocLikeText matches anything that starts with ** and isn't an empty comment. isJsDocStart only matches comments starting with ** but not *** and doesn't filter empty comments at all.

Why does isJsDocStart check for /***? If we don't consider /*** to be JSDoc there, then we shouldn't allow it here either.

Assuming the last condition in isJsDocStart is not a typo (e.g. checking for * instead of /), it seems like both of these could be the same function:

function isJsDocStart(content: string, start: number) { return content.charCodeAt(start) === CharacterCodes.slash && content.charCodeAt(start + 1) === CharacterCodes.asterisk && content.charCodeAt(start + 2) === CharacterCodes.asterisk && content.charCodeAt(start + 3) !== CharacterCodes.asterisk && content.charCodeAt(start + 3) !== CharacterCodes.slash; }

Why does isJsDocStart check for /***

Honestly no idea. It just seems as if we decided at some point that extra stars (like if, for example, you put line-length star guards around your comment) don't count as jsdoc as far as the parser is concerned. However I definitely think it's still correct to preserve such a comment in declaration emit, hence the difference.

I've unified them both under the previous isJSDocLikeText implementation, since it seems like the parser definition has just sat around since its introduction three years ago, and the only justification for why /*** @type {Foo} */ is any less jsdoc than /** @type {Foo} */ I've found is a remark on usejsdoc.org; but we're typically more lenient than it says (plus, closure compiler allows it (you can see the type annotation still getting applied under the advanced mode error)) cc @DanielRosenwasser it makes sense to be more lenient here, right?

DanielRosenwasser · 2018-02-20T23:46:09Z

src/compiler/emitter.ts

                if (result) {
                    return result;
                }
            }
        }
    }

+    /*@internal*/
+    export function getOutputPathsFor(sourceFile: SourceFile | Bundle, host: EmitHost, forceDtsPaths?: boolean) {


Don't mark forceDtsPaths as optional.

DanielRosenwasser · 2018-02-20T23:47:21Z

src/compiler/emitter.ts

+        let declarationPrinter: Printer;
+        if (emitOnlyDtsFiles || compilerOptions.declaration) {
+            const nonJsFiles = filter(sourceFiles, isSourceFileNotJavaScript);
+            declarationTransform = transformNodes(resolver, host, compilerOptions, (compilerOptions.outFile || compilerOptions.out) ? [createBundle(nonJsFiles)] : nonJsFiles, [transformDeclarations], /*allowDtsFiles*/ false);


nit: break (compilerOptions.outFile || compilerOptions.out) ? [createBundle(nonJsFiles)] : nonJsFiles into its own variable

DanielRosenwasser · 2018-02-21T00:07:23Z

src/compiler/transformers/declarations.ts

+
+            switch (input.kind) {
+                case SyntaxKind.ExportDeclaration: {
+                    if (!resultHasExternalModuleIndicator && isSourceFile(input.parent)) {


Technically I don't think you need to check resultHasExternalModuleIndicator before setting it to true but I guess it can save some work in the cases where you need to check modifiers.

DanielRosenwasser · 2018-02-21T00:10:23Z

src/compiler/transformers/declarations.ts

+            return emittedImports;
+        }
+
+        function visitDeclarationComponents(input: Node): VisitResult<Node> {


The name here is tripping me up. What is a declaration component? Are we just lacking a better name for a fuzzy concept?

I mean, the things at the top level are declaration statements. What's a good name for everything else?

Would topLevelDeclaration/topLevelDeclarationStatement and subDeclaration/subDeclarationStatement make sense?

The sub-parts aren't themselves guaranteed to be either declarations or statements.

While I don't have an issue with visitDeclarationComponents, perhaps visitDeclarationSubtree might be clearer?

Isn't subtree just about as overloaded a term as components with respect to an ast transformer? Tbqh, neither term gives you a great idea of what it operates over.

DanielRosenwasser · 2018-02-21T00:16:13Z

src/compiler/transformers/declarations.ts

+            }
+        }
+
+        function collectReferences(sourceFile: SourceFile, ret = createMap<SourceFile>()) {


I'd rather just avoid the default parameter since it's only used in two ways.

DanielRosenwasser · 2018-02-21T00:41:40Z

src/compiler/types.ts

@@ -688,6 +689,7 @@ namespace ts {
        Loop = 2,                           // Automatically generated identifier with a preference for '_i'.
        Unique = 3,                         // Unique name based on the 'text' property.
        Node = 4,                           // Unique name based on the node in the 'original' property.
+        OptimisticUnique = 5,               // Unique name based on the 'text' property, first instance won't use '_#' if there's no conflict


YES. But is there any case where we don't want to just use OptimisticUnique? Maybe we should make this the behavior of Unique after this PR.

@aozgaa has a reasonable point that you potentially want the Unique behavior in some cases (not here) to indicate that the output was machine-generated to some extent.

DanielRosenwasser · 2018-02-21T00:55:27Z

src/compiler/sourcemap.ts

        };

+        function setDisabled(state: boolean) {


https://ariya.io/2011/08/hall-of-api-shame-boolean-trap#double-negative

…Script into declaration-emit-refactor

rbuckton

In addition to my other review comments, I noticed when reviewing the baselines that there are a large number of differences due to the same general changes that seem unnecessary or inconsistent:

Some imports and exports are often in a different order.
No trailing comma on enum members. (while this is mentioned above, we don't emit enum members to JS, so we should preserve our existing emit)
No newline in empty module blocks. (while this is mentioned above, we don't emit modules to JS, so we should preserve our existing emit)
We sometimes (but not always) add : any type annotations (or : any[] for rest parameters) where we didn't before. (while this is mentioned above, we still have inconsistencies)
Incorrect indentation of multi-line parameter lists.

A number of other baselines differ only in whitespace around binding patterns, which I'm ok with as they align with our JS emit.

rbuckton · 2018-03-15T00:50:03Z

src/compiler/comments.ts

@@ -260,7 +260,21 @@ namespace ts {
            }
        }

+        function isJSDocLikeText(text: string, start: number) {


Why does isJsDocStart check for /***? If we don't consider /*** to be JSDoc there, then we shouldn't allow it here either.

Assuming the last condition in isJsDocStart is not a typo (e.g. checking for * instead of /), it seems like both of these could be the same function:

function isJsDocStart(content: string, start: number) { return content.charCodeAt(start) === CharacterCodes.slash && content.charCodeAt(start + 1) === CharacterCodes.asterisk && content.charCodeAt(start + 2) === CharacterCodes.asterisk && content.charCodeAt(start + 3) !== CharacterCodes.asterisk && content.charCodeAt(start + 3) !== CharacterCodes.slash; }

rbuckton · 2018-03-15T01:00:03Z

src/compiler/emitter.ts

        // Emit each output file
        performance.mark("beforePrint");
        forEachEmittedFile(host, emitSourceFileOrBundle, transform.transformed, emitOnlyDtsFiles);
        performance.measure("printTime", "beforePrint");

        // Clean up emit nodes on parse tree
        transform.dispose();
+        if (declarationTransform) declarationTransform.dispose();


You're reusing the parse tree across two different transformation pipelines at the same time. This could be problematic if either the declaration transforms or javascript transforms modify the emitNode on a parse tree node in a way that would affect the other. Even if we aren't doing anything now, it would be very easy to run into this in the future.

To be safe, we might need two separate passes over the files: one full pass (transform, print, and dispose) for javascript emit and another full pass (transform, print, and dispose) for declaration emit.

hmmm I implemented it that way initially, but it changes the order we emit files in (instead of all outputs for the same file being one after another, dts outputs come after everything else), which affects almost every baseline, as it happens. It would be better to make it "safe" to run multiple transforms at the same time, since nothing can enforce the invariant that you don't do so.

Making it "safe" might necessitate additional indirection for a lot of utility functions that interact with emitNode. Perhaps it would be better to transform and print each source file one at a time for multi-file emit. That could also cut back on the amount of memory we need to hold onto between each file as well, but adds additional overhead to reinitialize the printer and transformer between each file.

How we enforce this invariant is that one of first things we do in transformNodes is to clean up all emitNode entries on the parse tree. Its highly possible that we already have a bug in these new changes due to the fact we call transformNodes first for .js output and then for .d.ts output, erasing any emitNode entries we need for JS emit before we ever get to the point of printing. Its likely that we just don't have a test case that covers this scenario currently, and that we might see breaks in our RWC tests due to this.

I don't think it's possible to have a situation where it matters, unless a transformer sets an emit node on a node in a different file.

As I said, we probably already have situations where this matters since the transformNodes call for the declaration emit basically already disposes the transform result for the javascript emit. We just probably don't have a test for that case. The most likely causes for a parse tree node having an emitNode revolve around custom comment or source map positions associated with identifiers. Even if we don't have an issue today, its such an obscure interaction that when we do have an issue it will be very hard to track down.

I'd be tempted to change the line in transformNodes that ensures the parse tree is disposed to instead perform a Debug.assert if there are undisposed parse tree nodes so that we can catch these errors more readily. That would enforce that you .dispose() a transform result before you reuse the parse tree in another call to transformNodes.

Yeah, that just seems like a bit of an obscure API with a bunch of implicit invariants that can't be enforced in the typesystem. It's probably better if we can just keep emit nodes in a side table associated with each transform/transform result; especially since it's super easy just to accidentally call dispose before printing, for example, which would have silently messed up your print. (And the table already exists globally fir the implementation of dispose in the first place, iirc) If they're stored in the transform result, then dispose is just no longer necessary. (And we'd no longer be making parse tree nodes polymorphic on the presence of emit nodes, which is probably good and has potential to outweigh the extra indirection, just like inverting the checker's cache structure experimentally did)

We could use a side table only for the parse tree nodes, since we wouldn't want to hold onto intermediate nodes created by transforms that end up elided by a later transform as there's no point in holding onto information we don't need. The problem is that we would have to have a significant breaking change to the public API for transforms to support that (since right now those functions just look at .emitNode). Its much easier and less likely to break the API if we just ensure that we finish the JS transformation before we start the declaration one. For single file emit that's not a problem because we work at the Bundle level anyways, but for multi-file output we would have to transform and emit a single file at a time. That is a much more palatable change (and one that I've looked at once or twice in the past when analyzing performance), since we won't need the emitNode data between multiple files for multi-file output.

rbuckton · 2018-03-15T01:03:24Z

src/compiler/emitter.ts

+                        if (n.kind === SyntaxKind.Bundle) {
+                            return sourceFileOrBundle.kind === SyntaxKind.Bundle;
+                        }
+                        return getOriginalNode(n) === getOriginalNode(sourceFileOrBundle);


No sense in repeatedly calling getOriginalNode on sourceFileOrBundle. I would refactor this into a variable outside of the call to find.

rbuckton · 2018-03-15T01:10:48Z

src/compiler/factory.ts

@@ -214,6 +223,68 @@ namespace ts {
        return <BooleanLiteral & Token<SyntaxKind.FalseKeyword>>createSynthesizedNode(SyntaxKind.FalseKeyword);
    }

+    // Modifiers
+
+    export function createAbstractModifier() {


Do we need all of these, rather than just calling createToken directly? They'll be inlined anyways and all this does is just add more code to the compiler that needs to be parsed, compiled, and held in memory. The reason we have functions for createSuper, createThis, etc. is that they are for the most part reused repetitively throughout the codebase. Other than createDeclareModifier, all of the other createXModifier functions seem to only be used once.

Public API. It's very odd to have one but not the others.

Maybe we should add these to services since they don't need to be in tsc?

I don't think we've usually inlined a single-use sensible function (and we have a lot of internal single-use functions) and copied it into services for a tiny possible benefit to tsc elsewhere. It doesn't really make sense to split up the factory functions like that, from an organizational standpoint. Plus as you said, they'll almost certainly get inlined by the JIT if it matters, and function bodies are parsed lazily in the engine anyway, so any tiny parse overhead is deferred anyway. Tbqh, if you were into that I'd say we should have a tool for automatically inlining things based on a known heuristic (like the maximum bytecode size before the containing function can be inlined, for example), rather than speculating on it, and making the API harder to maintain in the process.

Heck, we should probably just be running a dead code elimination tool on tsc.

I just don't feel that these are worth it. If we are purely adding them as a convenience then we should have one for every token you can create that we emit, and we don't do that for things like QuestionToken, EqualsGreaterThanToken, etc.

Those have token in the name, which leads one to believe createToken is the correct way to go (and should appear in quickinfo if you're searching blindly, which, given our lack of docs, is what will probably be done), while these show up in quick info when you search with modifier, which is the name of the type where you'd use them. These also have an implied correct location in the AST (modifier positions), while a "question token" is a bit ambiguous - there's no obvious question token locations (there're all called out as .questionToken on a parent node) and there's little in common between where they appear.

Pretty much just that while Modifiers are effectively just tokens in the implementation, they carry some extra semantic baggage that makes it more obvious how to get a hold of one of them if they actually have individual public functions.

I've merged them all into one method

rbuckton · 2018-03-15T01:14:01Z

src/compiler/factory.ts

            const updated = <SourceFile>createSynthesizedNode(SyntaxKind.SourceFile);
            updated.flags |= node.flags;
            updated.statements = createNodeArray(statements);
            updated.endOfFileToken = node.endOfFileToken;
            updated.fileName = node.fileName;
            updated.path = node.path;
            updated.text = node.text;
+            updated.isDeclarationFile = isDeclarationFile === undefined ? node.isDeclarationFile : isDeclarationFile;


When I wrote this originally I made an effort to evaluate the property assignments in the same way (including order and optionality) that they are assigned by the parser to avoid excess polymorphism.

They are very much not in the same order right now; this is the initial ordering from the parser:

sourceFile.text = sourceText; sourceFile.bindDiagnostics = []; sourceFile.languageVersion = languageVersion; sourceFile.fileName = normalizePath(fileName); sourceFile.languageVariant = getLanguageVariant(scriptKind); sourceFile.isDeclarationFile = isDeclarationFile; sourceFile.scriptKind = scriptKind;

if this is important we can correct it in a separate PR.

rbuckton · 2018-03-15T02:24:37Z

tests/baselines/reference/enumClassification.js

 }
 declare enum E12 {
    A = 1,
    B = 2,
-    C = 4,
+    C = 4


There are a lot of these differences due to trailing commas in enums. Would it make sense to always emit the trailing comma as we have in the past?

We're reusing the original nodes in a bunch of these cases, which did not have trailing commas.

rbuckton · 2018-03-15T02:25:26Z

tests/baselines/reference/es6ImportDefaultBindingFollowedWithNamedImportDts.js

 import { x, a12 as y } from "./server";
+import { x11 as z } from "./server";


Again, this is a seemingly unnecessary reordering of imports and exports.

IIRC, These are the aliases that aren't actually marked as visible until some time after they are traversed. (If I remember, that ties into the "actually collects declaration errors during the pass instead of after" part of the change) Inserting them in-order would require a bit more work to put them back into the location they were elided from (essentially a n-ary walk just to reinsert imports painted as required during the prior walks) - in the old declaration emitter this was done by tracking the absolute position in the ouput string it was elided from and splicing it in at the end of the walk if it was visible. I guess the similar could be done here (tracking the position in the statement list each import was elided from and the list it was elided from, then rather than inserting at the top, inserting near where the recorded original location was; or maybe substuting an associated transformer-only statement that's been flagged?), but it seems like a lot of extra work for little gain? We don't check TDZ or anything in declaration files, right?

Given one of the reasons for .d.ts files is to document the API, it seems like it makes more sense to try to keep the declaration where it originally would have occurred in the file as it is more likely to be logically associated with the export that needed it, resulting in clearer documentation. If .d.ts were ever only intended to be machine readable, I'd be less concerned. I think it's worth preserving the old behavior in this case.

Order should now be preserved

rbuckton · 2018-03-15T02:27:08Z

tests/baselines/reference/internalAliasClassInsideTopLevelModuleWithoutExport.js

@@ -32,10 +32,10 @@ var cReturnVal = exports.cProp.foo(10);


 //// [internalAliasClassInsideTopLevelModuleWithoutExport.d.ts]
+import xc = x.c;


Another case of odd reordering, especially since if the source were written this way it would have failed at runtime.

rbuckton · 2018-03-15T02:27:53Z

tests/baselines/reference/moduleAugmentationImportsAndExports2.js

-
-
-//// [f1.d.ts]
-export declare class A {


~~What happened here? Why are these declarations now missing?~~

Never mind, I reread the description of the PR, so this is ok.

rbuckton · 2018-03-15T02:29:54Z

...ines/reference/project/mapRootAbsolutePathMixedSubfolderSpecifyOutputFile/node/bin/test.d.ts

@@ -1,3 +1,4 @@
+/// <reference path="../ref/m2.d.ts" />


This could cause breaks in dependent packages if you ship a .d.ts that references build inputs that aren't present in the published package.

IIRC, This code is broken to begin with, as the compiler options in use are an error to use together - see OP. This is arguably more correct.

I take it the conflicting options diagnostics aren't preventing declaration emit to begin with?

Yeah, options diagnostics aren't directly checked for blocking declaration emit, since they're only generated by the program.

…tained

weswigham · 2018-03-15T23:14:28Z

@rbuckton I believe I've either spoken to you about or mitigated each of your comments now?

Refactor declaration emitter into declaration transformer

62ee91d

weswigham commented Feb 14, 2018

View reviewed changes

weswigham requested review from rbuckton, RyanCavanaugh and mhegazy February 14, 2018 03:52

Merge branch 'master' into declaration-emit-refactor

9f42eb8

weswigham mentioned this pull request Feb 16, 2018

Enable Go to Implementation in Typescript Editors #6209

Closed

DanielRosenwasser requested changes Feb 21, 2018

View reviewed changes

Merge branch 'master' into declaration-emit-refactor

4c639b8

weswigham force-pushed the declaration-emit-refactor branch from 44f08d3 to 53571a7 Compare February 21, 2018 01:41

Slight cleanup from code review feedback

3adcbc7

weswigham force-pushed the declaration-emit-refactor branch from 53571a7 to 3adcbc7 Compare February 21, 2018 01:48

weswigham added 2 commits February 26, 2018 14:32

Merge branch 'master' into declaration-emit-refactor

9e34a3f

Merge branch 'master' into declaration-emit-refactor

db8e239

weswigham force-pushed the declaration-emit-refactor branch from 52c9f0a to db8e239 Compare February 27, 2018 00:28

weswigham mentioned this pull request Mar 1, 2018

allowJs:true + declaration:true #21455

Closed

9 tasks

weswigham and others added 3 commits March 1, 2018 14:46

Merge branch 'master' into declaration-emit-refactor

7844547

Merge branch 'master' into declaration-emit-refactor

d1359ee

Incorporate fix for new test

165e6bb

mhegazy mentioned this pull request Mar 8, 2018

Comments get truncated in generated declaration files #22037

Closed

weswigham added 3 commits March 8, 2018 15:44

Merge branch 'master' into declaration-emit-refactor

b84cbe4

Merge branch 'declaration-emit-refactor' of github.com:weswigham/Type…

63923e2

…Script into declaration-emit-refactor

Merge branch 'master' into declaration-emit-refactor

e8a64bd

rbuckton requested changes Mar 15, 2018

View reviewed changes

weswigham added 2 commits March 15, 2018 10:39

Merge branch 'master' into declaration-emit-refactor

d67ddc1

Swaths of PR feedback

a2f1a8a

weswigham added 7 commits March 15, 2018 12:50

Merge public methods

036c65a

Per-file output

289e3e5

Preserve input import ordering more often

08ffcc8

Unify jsdoc comment start detection under more lenient rule

cb69963

Merge branch 'per-file-output' into declaration-emit-refactor

1a699b8

Move to per-file transformations to reduce the memory that msut be re…

f83bc81

…tained

Fix typo

6d729cf

rbuckton approved these changes Mar 16, 2018

View reviewed changes

weswigham merged commit 19ec83f into microsoft:master Mar 16, 2018

ghost mentioned this pull request Mar 16, 2018

gulp runtests --t API fails #22647

Closed

microsoft locked and limited conversation to collaborators Jul 25, 2018

weswigham deleted the declaration-emit-refactor branch September 12, 2018 23:42

		import { x, a12 as y } from "./server";
		import { x11 as z } from "./server";

		@@ -32,10 +32,10 @@ var cReturnVal = exports.cProp.foo(10);


		//// [internalAliasClassInsideTopLevelModuleWithoutExport.d.ts]
		import xc = x.c;

Refactor declaration emitter into declaration transformer #21930

Refactor declaration emitter into declaration transformer #21930

Conversation

weswigham commented Feb 14, 2018 • edited Loading

weswigham Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

weswigham commented Feb 14, 2018

alfaproject commented Feb 16, 2018

rbuckton commented Feb 17, 2018

DanielRosenwasser commented Feb 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbuckton Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

rbuckton Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

rbuckton Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Mar 15, 2018

weswigham commented Feb 14, 2018 •

edited

Loading

weswigham Feb 14, 2018 •

edited

Loading

weswigham Feb 21, 2018 •

edited

Loading

rbuckton left a comment •

edited

Loading

rbuckton Mar 15, 2018 •

edited

Loading

rbuckton Mar 15, 2018 •

edited

Loading

rbuckton Mar 15, 2018 •

edited

Loading

rbuckton Mar 15, 2018 •

edited

Loading