Stabilize receiver of extension method application so that implicits accessible via the prefix can be candidate implicit arguments #5999

milessabin · 2017-07-19T14:39:40Z

This fixes scala/bug#4225 and scala/bug#5946 by introducing a synthetic val for the unstable LHS of an implicit method application where the method type depends on the LHS.

As applied to this example, taken from scala/bug#4225,

class Foo {
  class Bar
  object Bar {
    implicit def mkBar: Bar = new Bar
  }
}

object f extends Foo

implicit class Ops[F <: Foo](val f0: F) {
  def op(i: Int)(implicit b: f0.Bar): f0.Bar = b
}

f.op1(23)

this transformation produces,

{
  val fresh = Ops[f.type](f)
  fresh.op1(23)
}

Because fresh is stable we retain the singleton type F and the implicit argument of type f0.Bar can be resolved against the implicit scope of f.type.

A real world example of this is the macro API issue reported in scala/bug#5946 ... following this PR the usage below compiles as expected,

class Ops(val g: scala.reflect.api.JavaUniverse) {
  def op[T: g.TypeTag] = ()
}
implicit def Ops(g: scala.reflect.api.JavaUniverse): Ops = new Ops(g)
scala.reflect.runtime.universe.op[Int]

In principle this transformation could also be applied for explicit applications, however there would no gain: to be able the write the explicit argument the user would already have had to explicitly introduce the necessary val.

retronym · 2017-07-20T06:52:39Z

Minor admin note: could you please retitle the commit/PR with a description of the change, rather than the ticket reference? We've been leaving the ticket reference as a Fixes scala/...#NNNN in the PR description and commit message.

retronym · 2017-07-20T06:57:38Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                def stabilizeQual(tree: Tree): Option[(Tree, ValDef)] = tree match {
+                  case Select(qual, name) if qual.tpe.isStable => None
+                  case Select(qual, name) =>
+                    val vsym = context.owner.newValue(unit.freshTermName(), context.owner.pos, SYNTHETIC)


I think the qual.pos.focus will be more precise than context.owner.pos for the synthetic tree.

retronym · 2017-07-20T06:59:23Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                    val newQual = Ident(vsym).setType(qual.tpe)
+                    val newSelect = makeAccessible(treeCopy.Select(tree, newQual, name), tree.symbol, singleType(NoPrefix, vsym), newQual)._1
+                    if(newSelect.tpe.contains(vsym))
+                      Some((newSelect, vdef))


I don't see any atPos or typedPos that positions the synthetic trees, but I guess we can discuss the required positioning cleanups after we discuss the actual functional change.

retronym · 2017-07-20T07:04:10Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                  case _ => None
+                }
+
+                stabilizeQual(tree).map { case (newLhs, vdef) =>


I wonder if we can use mt.isDependentMethodType to drive whether the old or new logic is used. I still don't have a clear picture of the problem or solution, so just brainstorming a little...

Thanks for the pointer ... I'll investigate.

That won't help here. The method type contains the prefix, but it's not a dependent method in the sense which isDependentMethodType is testing for.

retronym · 2017-07-20T07:13:18Z

test/files/pos/t4225.scala

+    }
+  }
+
+  implicit class Ops[F <: Foo](val f0: F) {


A test variant with a by-name implicit f0 here might show up a problem with evaluation order by hoisting out the val. We deal with the same sort of issue in the named/default args transform, we would need to share some of that implementation in this case.

I've added some test variants which cover by name arguments (for the ops class and for the ops methods) and (by name) right associative ops and they don't cause any problems. Do you have a particular scenario in mind which you think might be problematic?

lrytz · 2017-07-20T08:24:13Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                    }
+                  case _ => None
+                }
+


Could you add some comment "rewrites expr to fxpr because ..., see scala/bug#X"? It will save time for people driving by here in the future.

Will do ...

milessabin · 2017-07-27T17:01:52Z

@retronym PR title and description tweaked as requested.

soronpo · 2017-07-27T19:21:12Z

@milessabin Forgive me if this is a stupid question, but is it possible to implement the same mechanism of synthetic val to resolve milessabin/shapeless#738 with ValueOf in TLS ?

adriaanm · 2017-09-26T16:38:31Z

The commit messages are still empty. Please start with a prescriptive/imperative title (to keep it short, don't mention tickets here), then a body with the description. End with references to tickets.

lrytz

The example here scala/bug#4225 (comment) still doesn't compile

class Foo {
  class Bar
  object Bar {
    implicit def fromString(a: String) = new Bar
  }
  def andThen(b : Bar) = b
}

object Test {
  (new Foo) andThen ("Bar")
}

gives

Test.scala:10: error: type mismatch;
 found   : String("Bar")
 required: _1.Bar where val _1: Foo
  (new Foo) andThen ("Bar")
                     ^
one error found

lrytz · 2017-11-03T12:42:07Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -880,8 +880,50 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
                }
              }
            )
-            else
-              typer1.typed(typer1.applyImplicitArgs(tree), mode, pt)
+            else {


This is the else expr of if (original != EmptyTree && pt != WildcardType). The if expr is not obvious to me - do you understand what it does and why we need the two branches? And why / if your fix is not needed in the if case?

lrytz · 2017-11-03T12:43:57Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

-            else
-              typer1.typed(typer1.applyImplicitArgs(tree), mode, pt)
+            else {
+              if(tree.symbol != null && !tree.symbol.isConstructor) {


I guess you observed the null case, maybe under erroneous trees? A comment would help.

lrytz · 2017-11-03T13:31:39Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

-              typer1.typed(typer1.applyImplicitArgs(tree), mode, pt)
+            else {
+              if(tree.symbol != null && !tree.symbol.isConstructor) {
+                def stabilizeQual(tree: Tree): Option[(Tree, ValDef)] = tree match {


Could the transformation be done using this?

case treeInfo.Applied(fun @ Select(qual, name), targs, argss) => ...

I also wonder if assigning the types manually to the Select / Apply / TypeApply nodes is necessary. Could we just build the trees and run the typer? Probably the new synthetic value would have to be entered in the typer's scope. Also, the select's symbol might need to be set manually to prevent going into overloading resolution. Similar case for named/default args: https://github.com/scala/scala/blob/v2.13.0-M2/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala#L173-L189. See also #558.

If we have the types available to assign why would we run the typer? Wouldn't that be more expensive?

It seems to me that inlining pieces of typedSelect and typedApply makes the code unnecessarily long / harder to maintain. I don't think we have a performance issue here, we'd just run through a few methods of the typer.

lrytz · 2017-11-03T13:33:16Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                  case _ => None
+                }
+
+                // Rewrites "qual.name ..." to "{ val lhs = qual ; lhs.name ... }" in cases where


Add a reference to "scala/bug#5946"

lrytz · 2017-11-03T13:53:10Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                    val newQual = Ident(vsym).setType(qual.tpe)
+                    val newSelect0 = atPos(tree.pos)(treeCopy.Select(tree, newQual, name))
+                    val newSelect = makeAccessible(newSelect0, tree.symbol, singleType(NoPrefix, vsym), newQual)._1
+                    if(newSelect.tpe.contains(vsym))


So this is to identify whether the transformation is necessary, right? Do you see a way to check this beforehand?

milessabin · 2017-11-11T14:15:42Z

@lrytz you're right that this didn't in fact fix SI-4225, although it did fix some related issues.

I've now completely reworked the fix and it now solves both the issues this PR originally addressed and SI-4225 proper. The earlier commits are obsolete ... I should probably just squash this.

I think the implementation is now a lot simpler ... the problematic cases are detected directly in typedSelect (in effect beforehand, as you suggested) and then we walk up the context tree to find the appropriate place to insert the stabilizing val. The duplication of typedSelect and typedApply that you were unhappy about have also now gone.

milessabin · 2017-11-11T14:19:24Z

The broadened scope of this fix means that one existing neg test has moved to pos: test/files/pos/t3995.scala ... the test is in fact recording an instance of 4255. Also a couple of neg check files have had to be updated because error messages now relate to a synthetic stabilizing val rather than an existential.

lrytz

Nice, this is definitely much nicer than the previous solution!

lrytz · 2017-11-15T21:54:07Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                val vsym = insertionContext.owner.newValue(unit.freshTermName(nme.STABILIZER_PREFIX), qual.pos.focus, SYNTHETIC | ARTIFACT | STABLE)
+                vsym.setInfo(qual.tpe)
+                insertionContext.scope enter vsym
+                val vdef = ValDef(vsym, qual)


I think we need a changeOwner here, for symbols defined within the qual, similar to named / default arguments: https://github.com/scala/scala/blob/v2.13.0-M2/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala#L173-L189

lrytz · 2017-11-15T21:55:16Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                vsym.setInfo(qual.tpe)
+                insertionContext.scope enter vsym
+                val vdef = ValDef(vsym, qual)
+                vdef.pos.makeTransparent


It seems to me that this just computes a new Position value and discards it.

Hmm ... you're right.

lrytz · 2017-11-15T22:15:04Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+
+              if(insertionContext != NoContext) {
+                val vsym = insertionContext.owner.newValue(unit.freshTermName(nme.STABILIZER_PREFIX), qual.pos.focus, SYNTHETIC | ARTIFACT | STABLE)
+                vsym.setInfo(qual.tpe)


I think we might have the same issue as scala/bug#7694 here, fixed in named / default args with the uncheckedBounds annotation (https://github.com/scala/scala/blob/v2.13.0-M2/src/compiler/scala/tools/nsc/typechecker/NamesDefaults.scala#L173-L189)

scala> :pa -raw // Entering paste mode (ctrl-D to finish) trait A trait B class Foo[A2, B2 <: A2] { class Bar object Bar { implicit def fromString(a: String) = new Bar } def andThen(b : Bar) = b } object Test { def lub = if (true) (null: Foo[A, A]) else (null: Foo[B, B]) (lub) andThen ("Bar") } // Exiting paste mode, now interpreting. [[syntax trees at end of typer]] // <pastie> def lub: Foo[_ >: A with B <: Object, _ >: A with B <: Object] = if (true) (null: Foo[A,A]) else (null: Foo[B,B]); { <synthetic> <stable> <artifact> val stabilizer$1: Foo[_1,_2] = Test.this.lub; stabilizer$1.andThen(stabilizer$1.Bar.fromString("Bar")) } (lub) andThen ("Bar") ^ <pastie>:14: error: type arguments [_1,_2] do not conform to class Foo's type parameter bounds [A2,B2 <: A2] There were compilation errors!

Thanks for the pointer.

adriaanm · 2017-11-29T00:52:58Z

sjrd makes some good points at scala/bug#10619, which we should keep in mind while reasoning about this change. Also, does it interact ok with the various rewrites we do to method invocations? (by-name, prefix, defaults)

I'm a little uneasy with this change, as we're basically adding a pretty ad-hoc form of wildcard capture (by giving the unstable prefix a name). Maybe that's ok, but we should think about this carefully in those terms. The Java type system and its rules around wildcard capture are pretty hard to follow.

sjrd · 2017-11-30T10:10:46Z

@adriaanm I don't think the points I make in scala/bug#10619 port over to this PR. There is one fundamental difference between this PR and the suggestions in the other issue: in this PR, we are only applying term rewritings that preserve evaluation order. In the other issues, the comments suggest to take a path from a type (where no evaluation order is even defined) and move it to a term position, thereby inventing some evaluation context for the term.

I do not see any problem with this PR as far as semantics are concerned.

retronym · 2017-11-30T10:32:23Z

test/files/pos/t4225.scala

+    f.op1(23)
+    f.op2(23)(true)
+    f.op3[Int]
+    f.op4(23)


Could we introduce a run test to show how by-name params interact with this lifting? I'd imagine that we'd need to lift a def or a val with a Function0 to preserve eval semantics.

If the qualifier is passed as a by-name arg to the implicit conversion, it doesn't seem suitably stable for a path (or equivalently, to be liftable to a val)

Agreed. That would have to be excluded.

It's not actually the argument to the implicit conversion which must be strict, it's the argument of the extension method class. That ought to be prohibited as it stands, but it looks as though it's falling foul of scala/bug#10619. The following is a small reproduction for 2.12.4 and 2.13.x (without this PR) that's relevant here,

scala> class Foo { class Bar } defined class Foo scala> class Unsound[F <: Foo](f: => F) { def quux(a: f.Bar): f.Bar = a } defined class Unsound

The two occurrences of f.Bar in quux should not be treated as having identical prefixes.

That's orthogonal to this PR though, and I can't add a neg test for the scenario you're worried about yet because it would be frustrated by scala/bug#10619.

adriaanm · 2017-11-30T18:40:41Z

I agree the intent preserves eval order, but, if I understood correctly, retronym identified one counter-example / needed exception. I'd like to see tests for all variations listed in my previous message (by-name, prefix, as well as defaults, just in case). For robustness, how about weird unstable things like a block that contains a definition? { class Foo ; qual }.m?

The other aspect of the change is that we are offering a limited form of wildcard capture with this rewrite. What if you have a long chain of method invocations that now results in a bunch of extra local vals that you don't want but can't disable? How about the recursion involved with rewriting chained calls?

milessabin · 2017-12-09T22:38:34Z

/rebuild

lrytz · 2018-01-10T10:25:50Z

There are some open comments: #5999 (review) and here #5999 (comment)

milessabin · 2018-01-12T16:13:09Z

Also rebased.

milessabin · 2018-01-12T16:17:54Z

@adriaanm

The other aspect of the change is that we are offering a limited form of wildcard capture with this rewrite. What if you have a long chain of method invocations that now results in a bunch of extra local vals that you don't want but can't disable? How about the recursion involved with rewriting chained calls?

A stabilizing val is only inserted if necessary to make the term type check, so I don't think this is an issue. Prior to this PR the long chain you've described wouldn't compile without you manually inserting vals. Post this PR the long chain compiles, and there's nothing to prevent you from splitting it up as you would have been forced to previously.

milessabin · 2018-01-12T19:15:46Z

Added some additional tests intended to address @retronym and @adriaanm's concerns.

lrytz

LGTM. Could you please:

squash the changes into a single commit
add a right-associative operator to the test for evaluation order, like:

scala> class Foo {
     |   class Bar
     |   object Bar {
     |     implicit def fromString(a: String) = new Bar
     |   }
     |   def andThen_:(b : Bar) = b
     | }

scala> { println("bar"); "Bar" } andThen_: { println("foo"); new Foo }
bar
foo
res0: Foo#Bar = Foo$Bar@769bd849

scala> { println("foo"); new Foo }.andThen_:({ println("bar"); "Bar" })
foo
bar
res1: Foo#Bar = Foo$Bar@2e71240b

lrytz · 2018-01-17T21:23:34Z

Oh, and also add a version with andThen_: by-name; it works thanks to #5969.

milessabin · 2018-01-18T20:28:53Z

@lrytz done. Note that your last request revealed a bug in byname arguments of right associative operators: scala/bug#10693.

milessabin · 2018-01-18T20:31:43Z

Gah ... forgot the check file ...

lrytz · 2018-01-19T10:25:23Z

Could you update the commit message to add a short title line? https://github.com/scala/scala/blob/2.13.x/CONTRIBUTING.md#clean-commits-clean-history

This fixes scala/bug#4225 and scala/bug#5946 by introducing a synthetic val for the unstable LHS of an implicit method application where the method type depends on the LHS. As applied to this example, taken from scala/bug#4225, ```scala class Foo { class Bar object Bar { implicit def mkBar: Bar = new Bar } } object f extends Foo implicit class Ops[F <: Foo](val f0: F) { def op(i: Int)(implicit b: f0.Bar): f0.Bar = b } f.op1(23) ``` this transformation produces, ```scala { val stabilizer$1 = Ops[f.type](f) stabilizer$1.op1(23) } ``` Because `stabilizer$1` is stable we retain the singleton type `F` and the implicit argument of type `f0.Bar` can be resolved against the implicit scope of `f.type`. A real world example of this is the macro API issue reported in scala/bug#5946 ... following this PR the usage below compiles as expected, ```scala class Ops(val g: scala.reflect.api.JavaUniverse) { def op[T: g.TypeTag] = () } implicit def Ops(g: scala.reflect.api.JavaUniverse): Ops = new Ops(g) scala.reflect.runtime.universe.op[Int] ``` In principle this transformation could also be applied for explicit applications, however there would no gain: to be able to write the explicit argument the user would already have had to explicitly introduce the necessary val.

milessabin · 2018-01-19T13:10:32Z

Done. Sorry about that ... I must have lost the title when I combined the messages.

lrytz

Thank you!

scala#5999 introduces a synthetic val for the unstable LHS of an implicit method application where the method type depends on the LHS. Unfortunately this only handles the case where an expression requires a _single_ stabilizing val: if the expression involves chained method applications, each requiring a stabilizing val, then only the last of these is emitted resulting in a failure during bytecode generation, object Test { class Foo { type Out } object Foo { implicit def foo: Foo { type Out = Bar } = ??? } class Bar { type Baz = Foo def foo(implicit foo: Baz): foo.Out = ??? } (new Bar).foo.foo } results in a NoSuchElementException, java.util.NoSuchElementException: value stabilizer$1 at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:425) at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:424) at scala.collection.mutable.AnyRefMap.apply(AnyRefMap.scala:180) at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:392) ... Prior to this commit only a single stabilizer is recorded on the expression tree which is overwritten by later ones. This is fixed here by allowing multiple stabilizers to be recorded and emitted.

scala#5999 introduces a synthetic val for the unstable LHS of an implicit method application where the method type depends on the LHS. Unfortunately this only handles the case where an expression requires a _single_ stabilizing val: if the expression involves chained method applications, each requiring a stabilizing val, then only the last of these is emitted resulting in a failure during bytecode generation, object Test { class Bar { class Foo { type Out } object Foo { implicit def foo: Foo { type Out = Bar } = ??? } def foo(implicit foo: Foo): foo.Out = ??? } (new Bar).foo.foo } results in a NoSuchElementException, java.util.NoSuchElementException: value stabilizer$1 at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:425) at scala.collection.mutable.AnyRefMap$ExceptionDefault.apply(AnyRefMap.scala:424) at scala.collection.mutable.AnyRefMap.apply(AnyRefMap.scala:180) at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder$locals$.load(BCodeSkelBuilder.scala:392) ... Prior to this commit only a single stabilizer is recorded on the expression tree which is overwritten by later ones. This is fixed here by allowing multiple stabilizers to be recorded and emitted. In addition to that, previously spurious dependencies were picked up due to insufficient dealiasing. The following, object Test { class Bar { class Foo { type Out } object Foo { implicit def foo: Foo { type Out = Bar } = ??? } def foo(implicit foo: Foo): foo.Out = ??? } (new Bar).foo.foo } triggered the preceding error despite not having a true dependency which would require a stabilizer in the first place. The test for a dependent argument now dealiases to eliminate these spurious dependencies. Finally we avoid retyping stabilizing definitions when they are emitted, fixing scala/bug#10736.

scala-jenkins added this to the 2.13.0-M3 milestone Jul 19, 2017

This was referenced Jul 19, 2017

Implicit Lookup fails for implicit view on nested type. scala/bug#4225

Closed

tagOfType crasher with universe as class parameter scala/bug#5946

Closed

retronym reviewed Jul 20, 2017

View reviewed changes

lrytz reviewed Jul 20, 2017

View reviewed changes

milessabin force-pushed the topic/si-4225 branch from 996c771 to b392aa7 Compare July 27, 2017 16:40

milessabin changed the title ~~Fix for SI-4225 and SI-5946.~~ Stabilize receiver of extension method application so that implicits accessible via the prefix can be candidate implicit arguments Jul 27, 2017

lrytz reviewed Nov 3, 2017

View reviewed changes

milessabin mentioned this pull request Nov 6, 2017

Fix for scala/bug#5946. #5992

Closed

milessabin force-pushed the topic/si-4225 branch from b392aa7 to b3b2b4c Compare November 11, 2017 14:06

lrytz reviewed Nov 15, 2017

View reviewed changes

milessabin mentioned this pull request Nov 21, 2017

2.12.4 regression: the compiler fails to reject unstable paths in types scala/bug#10619

Closed

adriaanm self-requested a review November 29, 2017 00:25

retronym reviewed Nov 30, 2017

View reviewed changes

milessabin force-pushed the topic/si-4225 branch from b3b2b4c to f3ccbbb Compare December 8, 2017 17:42

lrytz self-assigned this Jan 10, 2018

milessabin force-pushed the topic/si-4225 branch from f3ccbbb to 0a1c62c Compare January 12, 2018 16:12

lrytz requested changes Jan 17, 2018

View reviewed changes

milessabin force-pushed the topic/si-4225 branch from 7c96009 to 084d1ac Compare January 18, 2018 20:25

milessabin force-pushed the topic/si-4225 branch from 084d1ac to 99c7778 Compare January 18, 2018 20:31

milessabin force-pushed the topic/si-4225 branch from 99c7778 to bcbe993 Compare January 19, 2018 13:09

lrytz approved these changes Jan 19, 2018

View reviewed changes

lrytz merged commit f025f9d into scala:2.13.x Jan 19, 2018

hrhino mentioned this pull request Feb 2, 2018

2.13.0-M3 compiler crash with "Synthetic tree ... contains nonsynthetic tree" scala/bug#10706

Closed

This was referenced Feb 6, 2018

Release Scala 2.13.0-M3 scala/scala-dev#431

Closed

Only one stabilizing definition is emitted even when more than one required scala/bug#10714

Closed

milessabin mentioned this pull request Feb 6, 2018

Stabilizing definitions fixes #6312

Merged

julienrf mentioned this pull request Feb 19, 2018

Regression in implicit resolution between 2.12 and 2.13 scala/bug#10736

Closed

lrytz mentioned this pull request Jun 4, 2019

crash in lambdalift, perhaps interaction with extension method receiver stabilizer scala/bug#11556

Closed

retronym mentioned this pull request Jul 21, 2022

new "cyclic aliasing or subtype" error in 2.13 scala/bug#12622

Closed

lrytz mentioned this pull request Apr 23, 2024

Stabilizer causes type error in combination with type member scala/bug#12987

Closed

Stabilize receiver of extension method application so that implicits accessible via the prefix can be candidate implicit arguments #5999

Stabilize receiver of extension method application so that implicits accessible via the prefix can be candidate implicit arguments #5999

Conversation

milessabin commented Jul 19, 2017 • edited Loading

retronym commented Jul 20, 2017

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

milessabin commented Jul 27, 2017

soronpo commented Jul 27, 2017 • edited Loading

adriaanm commented Sep 26, 2017

lrytz left a 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

milessabin commented Nov 11, 2017

milessabin commented Nov 11, 2017 • edited Loading

lrytz left a 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

adriaanm commented Nov 29, 2017

sjrd commented Nov 30, 2017

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

adriaanm commented Nov 30, 2017

milessabin commented Dec 9, 2017

lrytz commented Jan 10, 2018

milessabin commented Jan 12, 2018

milessabin commented Jan 12, 2018

milessabin commented Jan 12, 2018

lrytz left a comment

Choose a reason for hiding this comment

lrytz commented Jan 17, 2018 • edited Loading

milessabin commented Jan 18, 2018

milessabin commented Jan 18, 2018

lrytz commented Jan 19, 2018

milessabin commented Jan 19, 2018

lrytz left a comment

Choose a reason for hiding this comment

milessabin commented Jul 19, 2017 •

edited

Loading

soronpo commented Jul 27, 2017 •

edited

Loading

milessabin commented Nov 11, 2017 •

edited

Loading

lrytz commented Jan 17, 2018 •

edited

Loading