[CS2] Restore bound class methods via runtime check to avoid premature calling of bound method before binding #4561

helixbass · 2017-06-06T21:04:59Z

As discussed in coffeescript6/discuss#84 by @ryansolid @connec @GeoffreyBooth: restore bound methods with a runtime check to avoid silent breakage of code that under 2 would call the method before it was bound (ie a base class constructor calling a derived class bound method in a non-this-preserving way eg as a callback)

Much of the code and tests here are restored from pre-#4530, I’ll comment in the code on new stuff I’ve added

As I commented in coffeescript6/discuss#84, there seems to be a question of how to handle anonymous classes (with bound methods). If my suggestion (3) (give anonymous classes with bound methods a name) is deemed the best option, I can try and implement that behavior (instead of currently (2) (forgo the runtime check for anonymous classes))

helixbass · 2017-06-06T21:08:57Z

src/nodes.coffee

@@ -1362,6 +1365,9 @@ exports.Class = class Class extends Base
        @ctor = method
      else if method.isStatic and method.bound
        method.context = @name
+      else if method.bound
+        @boundMethods.push method.name
+        method.constructorName = @name


Attach the class name (so that it can be passed as an argument to the runtime check in the method body), not sure if this is already available to the method somehow?

Removed method.bound = false, as now it needs to know about its bound-ness when compiling itself

helixbass · 2017-06-06T21:10:32Z

src/nodes.coffee

@@ -1399,7 +1405,8 @@ exports.Class = class Class extends Base
      method.name = new (if methodName.shouldCache() then Index else Access) methodName
      method.name.updateLocationDataIfMissing methodName.locationData
      method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor'
-      method.error 'Methods cannot be bound functions' if method.bound
+      method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor
+      # method.error 'Cannot define a bound method in an anonymous class' if method.bound and not @name


For illustration, this would be option (1) (disallow bound methods in anonymous classes) of the three options I outlined for dealing with anonymous classes with bound methods

helixbass · 2017-06-06T21:38:23Z

src/nodes.coffee

@@ -2140,6 +2154,9 @@ exports.Code = class Code extends Base
    wasEmpty = @body.isEmpty()
    @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments
    @body.expressions.unshift exprs...
+    if @isMethod and @bound and not @isStatic and @constructorName


The addition of the actual runtime check (using new _boundMethodCheck helper). Currently deals with anonymous classes via option (2) (forgo the runtime check for anonymous classes) with and @constructorName

helixbass · 2017-06-06T21:41:53Z

src/nodes.coffee

@@ -3097,6 +3114,13 @@ exports.If = class If extends Base

 UTILITIES =
  modulo: -> 'function(a, b) { return (+a % (b = +b) + b) % b; }'
+  _boundMethodCheck: -> "


helper that throws runtime error if bound method called with a this that isn't an instance of its parent class

as discussed in coffeescript6/discuss#84, this results in a stack trace where the offending method is in the second line

Is there a reason this helper’s name starts with an underscore? Our other helpers’ names don’t.

@GeoffreyBooth I think @connec was mimicking babel's _classCallCheck. Updated to boundMethodCheck

helixbass · 2017-06-06T21:44:28Z

test/classes.coffee

@@ -1574,3 +1670,76 @@ test "CS6 Class extends a CS1 compiled class with super()", ->
  eq B.className(), 'ExtendedCS1'
  b = new B('three')
  eq b.make(), "making a cafe ole with caramel and three shots of espresso"
+
+test 'Bound method called as callback before binding throws runtime error', ->


all the new tests are here, including tests that the runtime error actually occurs for the edge case and that bound methods work ok in other situations. Quite possible there are some other scenarios that should be tested, if explained I'll add more tests

Checking for errors goes in error_messages.coffee. Not sure why, we should probably refactor them out of there into the relevant test files, but for now that’s where all such tests go.

helixbass · 2017-06-06T23:49:50Z

@GeoffreyBooth ok updated (based on your suggestion) to run the check against the parent class (and only run it for child class bound methods). Had to get a little fancy with making sure that bound methods can properly reference the base class (which could be an arbitrary Expression) so I'll go through and make some more code comments

helixbass · 2017-06-06T23:52:52Z

src/nodes.coffee

@@ -1362,6 +1380,10 @@ exports.Class = class Class extends Base
        @ctor = method
      else if method.isStatic and method.bound
        method.context = @name
+      else if method.bound
+        @boundMethods.push method.name
+        @setParentRef(o)


if we have any bound methods, see if the parent class expression should be cached to a variable ref and save the (possibly updated) parent on method so that it can be used in the runtime check

helixbass · 2017-06-07T00:00:26Z

src/nodes.coffee

    o.indent += TAB

    result = []
+    if @assignParentRef


if we have bound methods and the parent class expression should be cached (as determined by setParentRef()), turn the class expression into a (ref = parent.class().expression, class Child extends ref {...})-style expression

Whilst this is a nice way of doing (so I'm not suggesting changing it), but the .cache mechanism exists for this purpose, and could be used to generate code like:

class Child extends (ref = parent.class().expression) {}

helixbass · 2017-06-07T00:03:42Z

src/nodes.coffee

@@ -2140,6 +2169,9 @@ exports.Code = class Code extends Base
    wasEmpty = @body.isEmpty()
    @body.expressions.unshift thisAssignments... unless @expandCtorSuper thisAssignments
    @body.expressions.unshift exprs...
+    if @isMethod and @bound and not @isStatic and @parentClass


the runtime check, basically _boundMethodCheck(this, parentClassRef). Will only be added for child class bound methods

GeoffreyBooth · 2017-06-07T06:38:18Z

src/nodes.coffee

@@ -1399,7 +1421,7 @@ exports.Class = class Class extends Base
      method.name = new (if methodName.shouldCache() then Index else Access) methodName
      method.name.updateLocationDataIfMissing methodName.locationData
      method.ctor = (if @parent then 'derived' else 'base') if methodName.value is 'constructor'
-      method.error 'Methods cannot be bound functions' if method.bound
+      method.error 'Cannot define a constructor as a bound function' if method.bound and method.ctor


In error messages, let’s use the phrasing “bound (fat arrow) function”. MDN calls => functions “arrow functions,” so that’s what most people will probably know them as. I think we should be explicit about “fat” arrow to distinguish from ->.

GeoffreyBooth · 2017-06-07T06:39:05Z

src/nodes.coffee

+  proxyBoundMethods: (o) ->
+    @ctor.thisAssignments = for name in @boundMethods by -1
+      name = new Value(new ThisLiteral, [ name ]).compile o
+      new Literal "#{name} = #{name}.bind(this)"


Rather than new Literal, shouldn’t this be new Assign etc.?

GeoffreyBooth · 2017-06-07T06:41:34Z

src/nodes.coffee

+  boundMethodCheck: -> "
+    function(instance, Constructor) {
+      if (!(instance instanceof Constructor)) {
+        throw new Error('Bound instance method accessed before binding')


Add semicolon.

GeoffreyBooth · 2017-06-07T06:42:44Z

test/classes.coffee

@@ -1574,3 +1670,76 @@ test "CS6 Class extends a CS1 compiled class with super()", ->
  eq B.className(), 'ExtendedCS1'
  b = new B('three')
  eq b.make(), "making a cafe ole with caramel and three shots of espresso"
+
+test 'Bound method called as callback before binding throws runtime error', ->


Checking for errors goes in error_messages.coffee. Not sure why, we should probably refactor them out of there into the relevant test files, but for now that’s where all such tests go.

GeoffreyBooth · 2017-06-07T06:43:49Z

@connec and @ryansolid, do you mind reviewing?

helixbass · 2017-06-07T18:12:43Z

@GeoffreyBooth ok pushed changes based on your comments

GeoffreyBooth · 2017-06-08T20:01:07Z

@helixbass Please merge 2 into your branch and resolve conflicts.

helixbass · 2017-06-09T00:12:32Z

@GeoffreyBooth ok merged 2

GeoffreyBooth · 2017-06-09T05:02:34Z

@ryansolid, does this PR solve your issue?

@connec, do you mind reviewing? I think as soon as this gets merged in we can release a new beta, unless you think #4493 is close enough to be worth waiting for. All tests pass.

connec · 2017-06-09T09:30:40Z

@GeoffreyBooth I'll take a look this evening, all going well. I think it would be worth waiting for #4493, once the tests are in it should be pretty much there.

connec

This is a clean patch that LGTM!

I have a small concern, but I don't think it should hold up a merge since it does fix the problem.

Do we think of this change as setting guarantees for the bound method? If so, the guarantee that "you'll be called with a context that is, at least, an instanceof the parent class" isn't anywhere near as valuable as "you'll be called on an instanceof the class". I suspect we could deal with anonymous classes by assigning it to a generated variable and using that in the check.

If we don't think of it as a guarantee to the method, but instead, as was the purpose of the original issue, as a safeguard against accidentally invoking a bound method before binding, then maybe we shouldn't perform the instanceof check at all, and simply check this != null. That would still satisfy the tests, whilst only opening up the number of cases where you could .call/.apply/.bind the prototype method (and cutting out the need for the parentRef shenanigans).

I reckon if we felt it was the latter, we could change it in this PR. If we think it's the former, bringing the check back to the actual class could be a later iteration.

FWIW my current feeling is to remove the instanceof check, and revisit it later if for some reason people are writing code where they want the guarantee of the right instance.

GeoffreyBooth · 2017-06-09T23:27:24Z

@connec You raise a good point. Wouldn’t checking just for null also resolve the case of a grandchild class (a class that extends a class that itself extends a class)? Seems simpler and cleaner. If someone wanted to check specifically that they were calling via the correct instance, they can add that check explicitly.

I thought there is no anonymous class issue, since it’s not possible to extend an anonymous class?

connec · 2017-06-10T00:31:24Z

I thought there is no anonymous class issue, since it’s not possible to extend an anonymous class?

Honestly I didn't follow that discussion very well. My understanding was that was why the check switched to the parent class (because there's no usable reference for the Constructor)? I guess the issue is with code like:

class Base
  constructor: ->
    @boundMethod()

new class extends Base
  boundMethod: =>
    console.log 'bound!'

In that case there's no reference to use for the instanceof check, hence the decision to use the parent reference.

But yeh, checking for null removes the whole problem, whilst still fixing the original issue. As you say, adding an instanceof for tighter guarantees can be done explicitly.

GeoffreyBooth · 2017-06-10T00:34:42Z

Well switching to checking for null should solve that, right?

connec · 2017-06-10T00:56:45Z

Yup!

helixbass · 2017-06-10T01:13:07Z

@connec so if a not-yet-bound method is run in a callback style (in any environment), this will always be undefined? As opposed to how this is the window object if a normal function which references this is called (normally) in the browser?

If that's true, then your suggestion and thinking make sense to me, as the != null check would presumably be less expensive than an instanceof check, and as you say it'll simplify the implementation and sort of bypass this issue of "what kind of guarantee are we making about this?" (which is what I was talking about in my last point)

GeoffreyBooth · 2017-06-13T20:32:27Z

If people start depending on bound methods being uncallable from other contexts (for some reason), it might be worth making them consistent across the board.

This sounds very speculative. I think unless you can come up with a realistic example, we should probably limit it to the scenario in question. We can always expand it later.

jashkenas · 2017-06-13T20:36:13Z

Is your concern around the runtime check mostly the performance impact?

My concern is mostly about the inherent ugliness of the thing. If we had always had to insert such a check — we never would have added bound methods in the first place. CS2 is an opportunity to take them out. Leaving them in (most of the time) with a nasty runtime check to make sure that you're not running into a catch-22 seems like a half-assed implementation.

Seems like we can do better. But if this is the best option...

helixbass · 2017-06-13T20:59:19Z

If the runtime check is deemed inappropriate on philosophical/perf/... grounds, I'll argue again that leaving bound methods as-is (with documentation of the breaking change) is better than removing bound methods. I still think it's theoretically equivalent to the @-constructor args breaking change in 2, eg this breaks in 2 for basically the same reason:

class B
  constructor: ->
    @f()

class A extends B
  constructor: (@f) ->
    super()

a = new A -> console.log 'called'

So in theory I'd say just document it the same way as however you're documenting the @-constructor args breaking change

However I think the use case that started the discussion (things like Backbone initialize()) is much more realistic and has merited trying to find a graceful solution. But as @connec says there's no way it could be caught at compile-time - I'd be down to try and add some compile-time checks but think that actually most wouldn't be catchable if we'd basically be looking for normal calls made directly inside a parent class constructor that's defined in the same file as the child class

Removing bound methods would be a hugely-breaking change for the sake of a little elegance. I'll again invoke Python 3 as a very clear warning against doing this. While I respect the feeling that bound methods might've never been added in the first place had this been the initial situation, CS2 is not a new "initial situation", it bears the weight of all the code written under 1.x

So for bound methods that leaves me weighing the runtime check vs just warning about possibly subtly breaking code (hand-in-hand with a similar warning about @-constructor args). Personally I can easily stomach the runtime check from a perf and ugliness standpoint but I also deeply respect @jashkenas' staunch stance

connec · 2017-06-13T21:04:25Z

We can always expand it later.

Sorry for not being clear, @GeoffreyBooth, that was my suggestion all along 😅

I had another read of coffeescript6/discuss#84 and there was a proposal that would avoid runtime checks by compiling class Child extends Parent then method: => console.log @ to:

class Child extends Parent {
  constructor () {
    super(...arguments)
    this.method = () => {
      return console.log(this)
    }
  }

  method () {
    throw new Error('Bound instance method accessed before binding')
  }
}

It would possibly be acceptable even to drop the prototype method completely, effectively bring CS2's bound methods in-line with ES's Stage 2 public fields proposal.

GeoffreyBooth · 2017-06-13T21:07:36Z

That’s remarkably elegant. I feel like there must be a catch.

connec · 2017-06-13T21:10:01Z

there must be a catch

The fact that it doesn't work with super is a pretty big one.

connec · 2017-06-13T21:18:07Z

Yet another option could be to rename the prototype method (leaving it undefined when accessed before binding):

class Child extends Parent {
  constructor () {
    super(...arguments)
    this.method = this.method$.bind(this)
  }

  method$ () {
    return console.log(this)
  }
}

This is a simplified version of this proposal without the runtime check. This behaviour would be equivalent to that of @-params in the same circumstances.

GeoffreyBooth · 2017-06-13T21:23:54Z

Yet another option could be to rename the prototype method

That looks ridiculous. If I saw that as my output I would wonder what the hell had happened. I suppose instead of method$ you could rename it something like method__temporaryNameToFoilAttemptAtCallsBeforeBinding but I’m not sure that’s better than a runtime check. And either way, if I tried to call that method before binding, i.e. what the runtime check checks for, the runtime would throw an error because I tried to access an undefined variable. Which would be confusing, because in my original code, the variable was, in fact, defined. At least the runtime check tells me exactly what’s wrong.

Okay, so unless there’s anything else, can we merge this in? @jashkenas I’m sorry for the inelegance, but this appears to be the least offensive solution to the problem. If a better solution can be found, it can become a new PR. I don’t think backward compatibility should be sacrificed for the sake of elegance.

ryansolid · 2017-06-14T14:34:02Z

I'd add the method renaming does havok to trying to resolve prototype chains. That proposal only worked because the properly named method is on the prototype too and all it was accomplishing was removing calling the check once the method was bound. However once you call super on an inherited bound method it ends up calling that function up the prototype chain which essentially lands us at this PR. If we were ok breaking inheritance atleast super could work assuming you couldn't inherit bound methods. I'm sure there is a way for this check to disappear post binding for inheritance chains that are less than 3 deep but I doubt it's worth the complexity.

So while I'm in full support of this PR (or leaving it largely as was pre #4530), if there was a directional precedence that made that impossible, I think we should go with an approach that keeps bound methods but makes them not inheritable like Typescript does. But that would require having super atleast work in bound methods. But even that would still be a significant breaking change which we are trying to avoid. So obviously very happy to just merge this PR and deal with that separately if needed.

jashkenas · 2017-06-14T15:41:19Z

I think I agree with @helixbass and @ryansolid that just leaving bound methods in, and documenting the breakage is a bit better than a nasty runtime check — but I'll defer to @GeoffreyBooth and @connec's call on this.

xixixao · 2017-06-14T15:58:56Z

I'd add that ES6 compilation (whatever it's downsides) would seem like a better way to go for CS2 than this workaround.

CS1 > ES6 > this PR

GeoffreyBooth · 2017-06-14T16:05:41Z

@jashkenas The problem is that we’ve already received bug reports for the “just leave them in” approach, which was released in 2.0.0-beta1. Granted we hadn’t documented the pitfalls, but I think it’s much safer for the community at large if we add the check; I would rather get complaints about “what’s this random helper I didn’t want in my code” than “my classes don’t work in CS2.” I can’t imagine too many CoffeeScript users really understand the ins and outs of prototypes and class compilation, and the differences in how CS2 classes are generated versus CS1. We can always remove the check later if we feel confident that it’s not needed.

@xixixao I don’t understand your comment. In CS2 we are outputting ES classes. ES classes don’t support bound methods, hence the question of what to output in their place.

@connec Aside from philosophical debate, is this PR ready to merge in?

xixixao · 2017-06-14T16:10:06Z

@GeoffreyBooth I meant ESWhateverVersionThatSupportsThis, Babel:

var A = function A() {
  var _this = this;

  this.f = function () {
    return _this.x;
  };
};

Also you are still gonna keep getting "my classes don't work" tickets with this PR, it's just gonna be clear why they don't work. This PR doesn't change what you can and cannot do, it just makes the error obvious.

ryansolid · 2017-06-14T19:00:11Z

@xixixao There is an idiomatic approach of writing every non-constructor or non-static class method as fat arrow bound so that regardless of how it was used you could set and forget and have it always bound to your class. CS1 let you do this, and I'm sure it's a practice used religiously by atleast a few dev shops since it aligns with how most OOP languages work when it comes to class methods which was really the only thing to serve as basis if you go back a few years.

From my understanding that ES8 or maybe never proposal doesn't support both inheriting from bound methods or binding inherited methods and calling super. Both really change things. The first for library/framework writers since they would need to remove all bound methods from their inheritable classes. Any inherited class code that was expecting this behavior could then also be broken. The second definitely breaks any of that application code that wants to bind an inherited method. It's arguable whether most of these methods actually need to be bound. It's possible to wrap any inherited method inside these new bound methods, but they essentially become something different as they are no longer part of the prototype chain.

@jashkenas
The difficulty I have with the original issue that caused all this is twofold. Firstly the pattern that made it apparent I believe isn't really a class thinking forward. I have to assume methods like Backbone's initialize exists since there was no standard way of doing constructors. The code you put in initialize you could put after you call super in your child class constructor. It's such a small piece of the puzzle that we shouldn't restrict bound methods in anyway that you wouldn't restrict unbound methods, with the exception of them not being bound as expected. Unfortunately every compiler detection method mentioned doesn't seem to be so non-invasive.

Secondly, having it silently fail I can see being pretty bad. Not bad for those aware I suppose but because it spans over multiple files/classes people might not get what's happening. I mean all the control is really in the child inherited class arguably so it is definitely on the child class writer to do the right thing. But they won't know until it causes weird errors on execution.

connec · 2017-06-14T22:06:25Z

@GeoffreyBooth yes, the PR looks good to me.

GeoffreyBooth · 2017-06-14T22:13:59Z

Thanks @helixbass!

@connec, I think there’s just #4493 left before we release 2.0.0-beta3. That PR looks possibly ready.

GeoffreyBooth · 2017-06-14T22:14:32Z

@ryansolid Do you want to try compiling your project using the 2 branch?

ryansolid · 2017-06-15T03:31:18Z

Yeah just tried it with a couple sample projects, everything looks good from server through client. Even running through a tight loop rendering benchmark where the check method was present in a few locations the performance loss wasn't that bad about .15 ms per loop. So all in all I'm pretty content with this. Thank you @GeoffreyBooth, @connec, and @helixbass for making this happen.

helixbass · 2017-06-17T00:08:04Z

Report from the wild: I just had the bound method runtime check kick in on some (new) code where I was dereferencing a bound method off this in code called by the parent constructor. So just had to postpone dereferencing, definitely saved me some debugging headaches

@ryansolid no problem, thanks for working through the alternatives and coming up with a good one. If you're curious about the performance difference if the bound check was inlined, would want to turn off the bound method check for code that you feel sure doesn't need it (eg in production), ... it shouldn't be that hard to roll your own slightly modified build, I'd be happy to help make that happen

GeoffreyBooth · 2017-06-30T05:55:37Z

@helixbass I’m trying to write the documentation for this. This is the example I’m thinking of using, based on this test:

class Base
  constructor: ->
    @onClick()      # This works
    clickHandler = @onClick
    clickHandler()  # This throws a runtime error

class Component extends Base
  onClick: =>
    console.log 'Clicked!', @

I’m testing this via c = new Component(). Looking at this, I have to wonder: shouldn’t the @onClick() line also throw an error? Why would that line work and not clickHandler()?

helixbass · 2017-07-01T15:05:28Z

@GeoffreyBooth it works because it's being called normally (thus with expected this). It's only if calling a method "non-directly" eg as a callback (like clickHandler()) that it won't have the expected this and thus will fail the runtime check

GeoffreyBooth · 2017-07-01T16:54:21Z

Ah, okay. So is the documentation correct or would you change anything?

helixbass · 2017-07-01T21:50:18Z

@GeoffreyBooth ya that example illustrates the basic idea nicely and concisely

Goues · 2021-12-09T12:13:41Z

Hello, is there a way to turn this off? I am migrating an older codebase to version 2 of coffescript that is using a bare transpilation to have smaller output and then it gets compiled by google closure compiler so that no files are wrapped in extra functions. However, this means that boundMethodCheck exists in global space and fails for all classes except the last one. I don't want to wrap everything in a wrapper function, that's redundant, and I trust our code enough to let it crash instead of having this runtime check that crashes it anyway. Could there be a config option to skip injecting it?

GeoffreyBooth · 2021-12-09T15:33:27Z

No, sorry, there are no config options for changing the behavior of the compiler (other than the few we already have, like bare). I suggest you refactor the classes in question to avoid needing the helper.

helixbass added 4 commits June 6, 2017 14:19

bound method runtime check

a6165e5

restore bound method tests

e2ee016

bound method tests

66b7016

test bound method in prop-named class

cb762ce

GeoffreyBooth changed the title ~~bound method runtime check~~ [CS2] Restore bound class methods via runtime check to avoid premature calling of bound method before binding Jun 6, 2017

GeoffreyBooth added change priority labels Jun 6, 2017

GeoffreyBooth added this to the 2.0.0 milestone Jun 6, 2017

helixbass commented Jun 6, 2017

View reviewed changes

helixbass added 2 commits June 6, 2017 18:16

run check against parent class

f82c19d

dummy commit

d226da7

remove comment

65b009c

helixbass commented Jun 7, 2017

View reviewed changes

rename to boundMethodCheck

019d9fa

GeoffreyBooth requested changes Jun 7, 2017

View reviewed changes

fixes from code review

dba119c

merge 2

0c91411

connec approved these changes Jun 9, 2017

View reviewed changes

GeoffreyBooth merged commit 9a48566 into jashkenas:2 Jun 14, 2017

GeoffreyBooth mentioned this pull request Jun 30, 2017

CS2 Discussion: Project: Progress as of June 2017: 2.0.0-beta3! coffeescript6/discuss#86

Closed

GeoffreyBooth removed the priority label Aug 3, 2017

coffeescriptbot mentioned this pull request Feb 19, 2018

CS2 Discussion: Project: Progress as of June 2017: 2.0.0-beta3! #4979

Closed

Goues mentioned this pull request Dec 9, 2021

Introduce a --no-bound-method-check flag #5389

Closed

[CS2] Restore bound class methods via runtime check to avoid premature calling of bound method before binding #4561

[CS2] Restore bound class methods via runtime check to avoid premature calling of bound method before binding #4561

Conversation

helixbass commented Jun 6, 2017 • edited by GeoffreyBooth Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helixbass Jun 6, 2017 • 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

helixbass commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connec Jun 9, 2017 • 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

GeoffreyBooth commented Jun 7, 2017

helixbass commented Jun 7, 2017

GeoffreyBooth commented Jun 8, 2017

helixbass commented Jun 9, 2017

GeoffreyBooth commented Jun 9, 2017

connec commented Jun 9, 2017

connec left a comment • edited Loading

Choose a reason for hiding this comment

GeoffreyBooth commented Jun 9, 2017

connec commented Jun 10, 2017 • edited Loading

GeoffreyBooth commented Jun 10, 2017

connec commented Jun 10, 2017

helixbass commented Jun 10, 2017

GeoffreyBooth commented Jun 13, 2017

jashkenas commented Jun 13, 2017

helixbass commented Jun 13, 2017

connec commented Jun 13, 2017 • edited Loading

GeoffreyBooth commented Jun 13, 2017

connec commented Jun 13, 2017 • edited Loading

connec commented Jun 13, 2017

GeoffreyBooth commented Jun 13, 2017

ryansolid commented Jun 14, 2017 • edited Loading

jashkenas commented Jun 14, 2017 • edited Loading

xixixao commented Jun 14, 2017

GeoffreyBooth commented Jun 14, 2017

xixixao commented Jun 14, 2017

ryansolid commented Jun 14, 2017

connec commented Jun 14, 2017 via email • edited by GeoffreyBooth Loading

GeoffreyBooth commented Jun 14, 2017

GeoffreyBooth commented Jun 14, 2017

ryansolid commented Jun 15, 2017 • edited Loading

helixbass commented Jun 17, 2017

GeoffreyBooth commented Jun 30, 2017

helixbass commented Jul 1, 2017

GeoffreyBooth commented Jul 1, 2017

helixbass commented Jul 1, 2017

Goues commented Dec 9, 2021 • edited Loading

GeoffreyBooth commented Dec 9, 2021

helixbass commented Jun 6, 2017 •

edited by GeoffreyBooth

Loading

helixbass Jun 6, 2017 •

edited

Loading

connec Jun 9, 2017 •

edited

Loading

connec left a comment •

edited

Loading

connec commented Jun 10, 2017 •

edited

Loading

connec commented Jun 13, 2017 •

edited

Loading

connec commented Jun 13, 2017 •

edited

Loading

ryansolid commented Jun 14, 2017 •

edited

Loading

jashkenas commented Jun 14, 2017 •

edited

Loading

connec commented Jun 14, 2017 via email •

edited by GeoffreyBooth

Loading

ryansolid commented Jun 15, 2017 •

edited

Loading

Goues commented Dec 9, 2021 •

edited

Loading