Scala.js: Implement non-native JS classes. #9774

sjrd · 2020-09-11T16:29:28Z

This commit implements the entire specification of non-native JS classes, including nested ones, except the behavior of anonymous JS classes. Anonymous JS classes are not supposed to have their own class/prototype, but currently they still do.

The most important PRs to scala-js/scala-js that define what is implemented here were:

Fix #1795: Essentials of Scala.js-defined JS classes. scala-js/scala-js#1809 Essentials of Scala.js-defined classes (former name of non-native JS classes)
Fix #1811: Add support for secondary constructors in JS classes scala-js/scala-js#1982 Support for secondary constructors
Fix #2164: Support default args constructors in js.Any classes. scala-js/scala-js#2186 Support for default parameters in constructors
Fix #1997: Add support for js.Symbols in @JSName annotations. scala-js/scala-js#2659 Support for js.Symbols in @JSName annotations
Fix #2805 and #2398: Nested JavaScript classes. scala-js/scala-js#3161 Nested JS classes

However, this commit was written more based on the state of things at Scala.js v1.2.0 rather than as a port of the abovementioned PRs.

The support is spread over 4 components:

The ExplicitJSClasses phase, which reifies all nested JS classes, and has extensive documentation in the code.
The JSExportsGen component of the back-end, which creates dispatchers for run-time overloading, default parameters and variadic arguments (equivalent to GenJSExports in Scala 2).
The JSConstructorGen component, which massages the constructors of JS classes and their dispatcher into a unique JS constructor.
Bits and pieces in JSCodeGen, notably to generate the js.ClassDefs for non-native JS classes, orchestrate the two other back-end components, and to adapt calls to the members of non-native JS classes.

JSConstructorGen in particular is copied quasi-verbatim from pieces of GenJSCode in Scala 2, since it works on js.IRNodes, without knowledge of scalac/dotc data structures.

Edit: I've now also added the behavior of anonymous JS classes, whose equivalent in Scala 2 was scala-js/scala-js#2292

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

Separate subject from body with a blank line

When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:

Limit the subject line to 72 characters

Capitalize the subject line

Do not end the subject line with a period

Use the imperative mood in the subject line ("Add" instead of "Added")

Wrap the body at 80 characters

Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

sjrd · 2020-09-18T14:00:03Z

compiler/src/dotty/tools/backend/sjs/JSConstructorGen.scala

+   *  We are only manipulating IR trees and types.
+   *
+   *  The only difference is the two parameters `overloadIdent` and `reportError`,
+   *  which are added so that this entire file can be even more isolated.


Reviewers: Given the verbatim copy from Scala 2, it is probably completely useless to review this file.

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

sjrd · 2020-09-18T14:07:46Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+    def typeInfo: String = sym.info.toString
+  }
+
+  private sealed abstract class RTTypeTest


Reviewers: From here until the end of this file, the contents are basically copied verbatim from Scala 2.

sjrd · 2020-09-18T14:13:41Z

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

+     * have wrapped them in a `withContextualJSClassValue`, not knowing where
+     * they belong in the larger tree.
+     * We now unwrap those, canceling out that effect.
+     * TODO Is there a better way to do this?


@smarter said on gitter:

parents are transformed in a special context, that might help: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/transform/MegaPhase.scala#L360

Unfortunately, that special context is not so special. It's just built with a particular set of (owner, outer context, scope), but we cannot otherwise identify it. So that trail is a dead end.

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

sjrd · 2020-09-18T14:16:10Z

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

+  def fakeNew(cls: ClassSymbol, ctor: TermSymbol)(using Context): Tree = {
+    /* TODO This is not entirely good enough, as it break -Ycheck for generic
+     * classes. Erasure restores the consistency of the fake invocations.
+     * Improving this is left for later.


@smarter Any idea how to improve this?

My best idea so far is to leave holes at this phase, and add another phase between Erasure and LambdaLift that would create the fake New nodes. Indeed, after erasure, the problem of type parameters does not exist anymore, and we wouldn't break consistency.

I think the best option would be to tweak LambdaLift to make it do what you want. But failing that, I agree that adding the new after Erasure would be nicer than doing it here indeed.

Regarding making lambdalift do what I want, I've been trying to come up with exactly what I want. And in fact I cannot think of a better way to receive the information I want than the one I have now (at least for ways that maintain semantic meaning of the Trees we have in the compiler at the phases we have).

So the question remains how to produce that information, and AFAICT modifying lambdalift will not be easier than having a miniphase create those fake New nodes, so I'm more inclined to go that route.

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

gzm0

Not much to say about the PR in its current form (only minor things). However (as I have already mentioned offline), it seems that a lot here is working around the current functioning of dotc, rather than integrating with it. LMK how you would like to proceed in that regard (both short- and long-term) and I'm happy to give further input.

gzm0 · 2020-09-19T15:21:54Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+   *  dotc inserts statements before the super constructor call for param
+   *  accessor initializers (including val's and var's declared in the params).
+   *  We move those after the super constructor call, and are therefore
+   *  executed later than for a Scala class.


I take it this is user visible if I do something like this:

abstract class A extends js.Object { def a: Int println(a) } class B(val a: Int) extends A

OTOH, this only applies to JS classes, so its just a semantic difference (even though a subtle one).

Yes. That semantic difference already exists in Scala 2, and is unavoidable.

I have expanded the comment to make it clear that it is related to Scala/Scala.js as languages, rather than a workaround for any particular compiler.

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

gzm0 · 2020-09-19T16:09:59Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+         * those two methods as primitives to completely eliminate them.
+         *
+         * Hopefully this will become unnecessary when/if we manage to
+         * reinterpret js.| as a true Dotty union type.


It seems we're amassing quite a bit of tech debt that can be fixed dotty internally. That is OK, especially if it is to bootstrap into something usable we can improve on.
My recommendation would be to track these in some form (can be issue tracker, doesn't have to be) so we can keep a grasp on them.

I intend to submit an issue for this one, but I haven't spent time on minimizing it yet. I'd like to find a minimization that is independent of Scala.js, since this is a typechecker issue.

gzm0 · 2020-09-19T16:28:35Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+  }
+
+  def genJSConstructorDispatch(alts: List[Symbol]): (Option[List[js.ParamDef]], js.JSMethodDef) = {
+    val exporteds = alts.map(Exported)


This imports quite a bit of tech debt. (Exported as a class IMO does not make sense anymore, I have tried removing it, but it requires control flow changes, so I haven't gotten around to it yet).

Maybe this is also OK, just want to point it out.

I'm not sure what's wrong, here. Exported gathers the information about how a method behaves in the context of the run-time overloading dispatch that JSExportsGen has to build. Since constructing that information is non-obvious, and is then reused several times over the run-time overloading dispatch generation algorithm, it makes sense to me to store that info somewhere, and it seems to me that a class is perfectly applicable for that.

What is strange IMO is that we calculate things inside the class itself (rather than in the flow of export generation). An Exported has no more semantic meaning than a Symbol (it used to, but not anymore). So IMO a better thing would be something more semantically meaningful.

So ... IIUC, it would be better to

Move the computations in a separate method that then instantiates the Exported with all the already computed data (rather than put everything in the constructor), and

Rename it to something more meaningful, perhaps SymbolExportInfo?

Would that address your concerns, or am I missing something?

Yes. My feeling is that with a better control flow, some of this data becomes more local. But it's just a gut feeling. I haven't tried.

gzm0 · 2020-09-23T11:16:26Z

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

+  private def isApplicableOwner(sym: Symbol)(using Context): Boolean = {
+    !sym.isStaticOwner || (
+        sym.is(ModuleClass) &&
+        sym.hasAnnotation(jsdefn.JSTypeAnnot) &&


How is this different form sym.isJSType? (also below)

gzm0 · 2020-09-23T12:50:30Z

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

+          else
+            ref1
+        } else {
+          // JS traits never have an initializer, no matter what dotc thinks


Can we make the rest of dotc more JS aware to not "think" that?

Perhaps the choice of words is not appropriate, here.
In a way, adding the NoInits flag here precisely makes dotty aware that a JS trait does not have an initializer. It's just that the initial computation of whether or not to put NoInits does not add the flag to JS traits, since it is not directly aware of the specificity of JS traits. That initial computation happens in Namer. Maybe I could try to add that directly in Namer, but then I would also have to do something in Scala2Unpickler to fix up things coming from Scala 2 (there is already some logic in there to patch NoInits on some well-known Scala 2 traits, because they never have the NoInits flag in the pickles).

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

gzm0 · 2020-09-23T12:58:40Z

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

+      case typeRef: TypeRef => typeRef
+      case _ =>
+        // This should not have passed the checks in PrepJSInterop
+        report.error(i"class type required but found $tpe0", tree)


Assert instead? (if it is a compiler bug?)

compiler/src/dotty/tools/dotc/transform/sjs/ExplicitJSClasses.scala

gzm0 · 2020-09-23T13:08:40Z

Oh, seems a bunch of stuff has changed from my review snapshot :-/ I'll have a look at that as well.

sjrd

@gzm0 I've added a commit that I think addresses all your comments except the ones that are of the kind "can we change dotty to make this simpler". I need to go prepare dinner now; I'll come back for those later.

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

sjrd · 2020-09-23T14:23:37Z

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala

+         * those two methods as primitives to completely eliminate them.
+         *
+         * Hopefully this will become unnecessary when/if we manage to
+         * reinterpret js.| as a true Dotty union type.


I intend to submit an issue for this one, but I haven't spent time on minimizing it yet. I'd like to find a minimization that is independent of Scala.js, since this is a typechecker issue.

sjrd · 2020-09-23T14:28:28Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+  }
+
+  def genJSConstructorDispatch(alts: List[Symbol]): (Option[List[js.ParamDef]], js.JSMethodDef) = {
+    val exporteds = alts.map(Exported)


I'm not sure what's wrong, here. Exported gathers the information about how a method behaves in the context of the run-time overloading dispatch that JSExportsGen has to build. Since constructing that information is non-obvious, and is then reused several times over the run-time overloading dispatch generation algorithm, it makes sense to me to store that info somewhere, and it seems to me that a class is perfectly applicable for that.

sjrd · 2020-09-23T15:17:43Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+
+    val hasVarArg = alt.hasRepeatedParam
+    val maxArgc = if (hasVarArg) paramsSize - 1 else paramsSize
+    val needsRestParam = maxArgc != minArgc || hasVarArg


Indeed. I have significantly reworked this area of the code now to address that and other comments below. The logic to determine the required args is now in genExportMethod.

sjrd · 2020-09-23T15:19:34Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+    val hasVarArg = alt.hasRepeatedParam
+    val maxArgc = if (hasVarArg) paramsSize - 1 else paramsSize
+    val needsRestParam = maxArgc != minArgc || hasVarArg
+    val formalArgsRegistry = new FormalArgsRegistry(minArgc, needsRestParam)


I would rather keep FormalArgsRegistry as is since it is 100% in common with Scala 2. I'm not really sure how to improve it anyway.

sjrd · 2020-09-23T15:43:12Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+      js.Assign(js.JSSuperSelect(superClass, receiver, nameTree),
+          allArgs.head.asInstanceOf[js.Tree])
+    } else {
+      js.JSSuperMethodCall(superClass, receiver, nameTree, allArgs)


No, we're passing exactly the arguments that we received (the rest param being passed with a spread to re-expand it).

sjrd · 2020-09-23T15:51:15Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+          allArgs.head.asInstanceOf[js.Tree])
+    } else {
+      js.JSSuperMethodCall(superClass, receiver, nameTree, allArgs)
+    }


That was actually very tempting, but it doesn't work because genApplyJSMethodGeneric unboxes the result, and here we must keep it boxed.

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

sjrd · 2020-09-23T16:16:43Z

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

+      val verifiedOrDefault = if (param.hasDefault) {
+        js.If(js.BinaryOp(js.BinaryOp.===, jsArg, js.Undefined()), {
+          genCallDefaultGetter(exported.sym, i, param.sym.sourcePos, static) {
+            prevArgsCount => result.take(prevArgsCount).toList.map(_.ref)


I tried to do as you suggest, but it gets uglier than expected because we then also need to compute the tpes in advance (not just the idents), and to compute the tpes we need to also look at whether it's repeated param or not (and if it is, it's a Scala Seq type, so it's annoying to produce), etc. In the end it's harder to reason about than the mutable state, IMO.

compiler/src/dotty/tools/backend/sjs/JSExportsGen.scala

gzm0 · 2020-09-24T14:05:07Z

I have looked at the new commits, nothing to add.

sjrd · 2020-09-24T15:49:59Z

Regarding adding dedicated Tree nodes instead of the magic methods createInnerJSClass, createLocalJSClass and withContextualJSClassValue.

First, this generally goes against the current in dotc. The current has been in favor of less kinds of Trees, and instead represent things as methods where possible. A clear example is the absence of a Throw node, replaced by an internal magic method throw(ex: Throwable). Throughout the pipeline, it navigates as an Apply, and is identified as a primitive in the back-end.

One concern, which went in favor of reducing the number of tree kinds, is that every additional kind of Tree degrades ever so slightly the performance of the entire compiler, for all programs, because the type test chains in the pattern match get a bit longer. If some kind of tree is not used pervasively (like If) and can go through most if not all of the pipeline as a magic method, the dotc philosophy is that it is better to choose the magic method. Here, we already know that these "tree nodes" can proceed through the pipeline as method calls.

For two of the methods, namely createInnerJSClass and createLocalJSClass, we can even argue that they truly are methods, semantically speaking. They are proper terms, that have some genuine term arguments (the super class values), and that will be compiled into actual terms in the back-end (with IR nodes that no other method produces, but that's the case for all the primitive methods). The only shadow in that landscape is that some of their arguments are not proper terms: the classOf[C] is only there to carry a ClassSymbol until the back-end, and the fake New nodes will be deconstructed by the back-end. Could we improve on that? Perhaps. For the classOf, we could make createInnerJSClass and createLocalJSClass (and constructorOf) more like isInstanceOf and asInstanceOf: they would be special-cased to retain their type argument even after erasure, which would remove the deconstruct-the-classOf aspect.

That has to be balanced with an entirely different concern. If we create special things that appear in the compiler only in the JS pipeline, we create a big risk regarding compiler plugins. If a compiler plugin does not actively take steps and tests to support Scala.js-related features, they can end up breaking on Scala.js code. If instead, they simply see methods and process them like any other, and it works like that, it's a win.

Finally, I'll talk a bit more about withContextualJSClass. That one is probably the less nice of the three, because it does not even compile to a term at all. Instead, it is meant to carry an additional term that should be attached to the Apply nodes surrounding News and Supers. Should we instead make that first-class in the compiler? Again, it's a trade-off. For performance reasons, we cannot add a field to every Apply, New or Super node in the programs, as that would impact every piece of every program, even on the JVM. So should we use alternative JSNew and JSSuper nodes? That would make the job of the back-end nicer, but at the cost of more tree nodes (performance concerns) and the fact that compiler plugins need to be aware of them (correctness of integration concerns).

In conclusion, although there are potential gains to reap from having dedicated Tree nodes, there are also downsides. The current approach has the merit of being well established and known to work in Scala 2. We could try a different approach, but tht requires some more exploration, and it is not clear to me that it will be overall better.

gzm0 · 2020-09-24T16:27:14Z

First, this generally goes against the current in dotc. [...]

I never thought about it that way. That's actually a very neat way of simplifying things.

For two of the methods, namely createInnerJSClass and createLocalJSClass, we can even argue that they truly are methods, semantically speaking.

Yep. Sold.

and the fake New nodes

Could we make them lambdas so they are less hacky?

they would be special-cased to retain their type argument even after erasure

Seems like there are multiple constructs in the compiler (in general that need this). This might be a generalization that is worth it.

So should we use alternative JSNew and JSSuper nodes?

Could we add methods instead (jsNewClass)? (I'm not sure this is even feasible and sane, just putting it out there).

This commit implements the entire specification of non-native JS classes, including nested ones, except the behavior of *anonymous* JS classes. Anonymous JS classes are not supposed to have their own class/prototype, but currently they still do. The most important PRs to scala-js/scala-js that define what is implemented here were: 1. scala-js/scala-js#1809 Essentials of Scala.js-defined classes (former name of non-native JS classes) 2. scala-js/scala-js#1982 Support for secondary constructors 3. scala-js/scala-js#2186 Support for default parameters in constructors 4. scala-js/scala-js#2659 Support for `js.Symbol`s in `@JSName` annotations 5. scala-js/scala-js#3161 Nested JS classes However, this commit was written more based on the state of things at Scala.js v1.2.0 rather than as a port of the abovementioned PRs. The support is spread over 4 components: * The `ExplicitJSClasses` phase, which reifies all nested JS classes, and has extensive documentation in the code. * The `JSExportsGen` component of the back-end, which creates dispatchers for run-time overloading, default parameters and variadic arguments (equivalent to `GenJSExports` in Scala 2). * The `JSConstructorGen` component, which massages the constructors of JS classes and their dispatcher into a unique JS constructor. * Bits and pieces in `JSCodeGen`, notably to generate the `js.ClassDef`s for non-native JS classes, orchestrate the two other back-end components, and to adapt calls to the members of non-native JS classes. `JSConstructorGen` in particular is copied quasi-verbatim from pieces of `GenJSCode` in Scala 2, since it works on `js.IRNode`s, without knowledge of scalac/dotc data structures.

This was in fact much easier than I thought. Unlike in scalac, where we forcefully reattach captures as normal constructor arguments, here we keep them as captures. This is much more natural, and yields a simpler implementation.

We don't use `paramSymss` anymore. We now only work with `paramNamess` and `paramInfoss`, which are reliable. We also clean up a bunch of things by making things "more general", requiring less explanation comments. In particular, we make no difference between the outer param and other capture params.

We enable Ycheck for our phases in the test suite to make sure that we do not regress in the future.

sjrd · 2020-10-02T07:41:09Z

Rebased.

and the fake New nodes

Could we make them lambdas so they are less hacky?

Hum, I don't understand. Do you mean putting the News inside Closure nodes, and have an array of Closure(..., rhs = Apply(New(_), ...)) instead of an array of Apply(New(_), ...)? That would only make it harder to recover in the back-end, I think.

they would be special-cased to retain their type argument even after erasure

Seems like there are multiple constructs in the compiler (in general that need this). This might be a generalization that is worth it.

So should we use alternative JSNew and JSSuper nodes?

Could we add methods instead (jsNewClass)? (I'm not sure this is even feasible and sane, just putting it out there).

I believe those are good things we could explore, but at this point I suggest to leave them for future work. They would be internal improvements in the compiler, but I don't think they will directly improve things for users. I'd rather validate this port PR and give it to users before researching further.

gzm0 · 2020-10-02T10:02:14Z

I believe those are good things we could explore, but at this point I suggest to leave them for future work.

Yep, sounds fair. Is there anything specific I still should look at?

sjrd · 2020-10-02T10:04:20Z

Is there anything specific I still should look at?

I don't think so.

It seems I'm only waiting for a review by @nicolasstucki at this point, mostly to make sure that I'm not doing anything dotty-stupid. :)

nicolasstucki · 2020-10-02T10:17:15Z

I will review it this afternoon

sjrd · 2020-10-02T11:30:21Z

Thanks a lot, @gzm0 @nicolasstucki and @smarter for your reviews on this complex PR! ❤️

dottybot reviewed Sep 11, 2020

View reviewed changes

sjrd force-pushed the sjs-non-native-js-classes branch 9 times, most recently from 9f9df05 to 3744aec Compare September 18, 2020 13:38

sjrd changed the title ~~WiP Scala.js: Non-native JS classes~~ Scala.js: Implement non-native JS classes. Sep 18, 2020

sjrd requested review from gzm0, smarter and nicolasstucki September 18, 2020 13:40

sjrd assigned smarter, gzm0 and nicolasstucki Sep 18, 2020

sjrd commented Sep 18, 2020

View reviewed changes

sjrd marked this pull request as ready for review September 18, 2020 14:18

sjrd force-pushed the sjs-non-native-js-classes branch from 1eba262 to ffbd08c Compare September 18, 2020 17:38

gzm0 reviewed Sep 23, 2020

View reviewed changes

sjrd commented Sep 23, 2020

View reviewed changes

sjrd force-pushed the sjs-non-native-js-classes branch from 3f940aa to 7b638aa Compare September 24, 2020 14:41

sjrd added 3 commits October 2, 2020 09:28

Better implementation for isFullyApplied.

d31dddf

Use pattern matching in transformTemplate.

534a5b2

sjrd added 8 commits October 2, 2020 09:28

Implement HashSet.-= in terms of remove, not the other way around.

fc52452

Address my self-review.

7dbea35

Implement the behavior of anonymous JS classes.

7c645b0

This was in fact much easier than I thought. Unlike in scalac, where we forcefully reattach captures as normal constructor arguments, here we keep them as captures. This is much more natural, and yields a simpler implementation.

Better extraction of the super type constructor.

bd03e71

Make sure that tests that compile and link keep doing so.

046f01f

Move the insertion of fake New nodes after erasure.

5bade1d

We enable Ycheck for our phases in the test suite to make sure that we do not regress in the future.

Address a number of review comments.

a91beae

sjrd force-pushed the sjs-non-native-js-classes branch from 7b638aa to a91beae Compare October 2, 2020 07:35

gzm0 approved these changes Oct 2, 2020

View reviewed changes

nicolasstucki approved these changes Oct 2, 2020

View reviewed changes

sjrd merged commit b7256bb into scala:master Oct 2, 2020

sjrd deleted the sjs-non-native-js-classes branch October 2, 2020 11:29

Scala.js: Implement non-native JS classes. #9774

Scala.js: Implement non-native JS classes. #9774

Conversation

sjrd commented Sep 11, 2020 • edited

dottybot left a comment

Choose a reason for hiding this comment

Commit Messages

sjrd Sep 18, 2020 • 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

gzm0 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

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

gzm0 commented Sep 23, 2020

sjrd 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

gzm0 commented Sep 24, 2020

sjrd commented Sep 24, 2020

gzm0 commented Sep 24, 2020

sjrd commented Oct 2, 2020

gzm0 commented Oct 2, 2020

sjrd commented Oct 2, 2020

nicolasstucki commented Oct 2, 2020

sjrd commented Oct 2, 2020

sjrd commented Sep 11, 2020 •

edited

sjrd Sep 18, 2020 •

edited