.chalk file syntax writer #430

super-tuple · 2020-05-03T03:30:53Z

This adds rendering lowered chalk data structure to the .chalk file syntax.

See discussion in Zulip: https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/.2Echalk.20file.20writer

Work in progress, the remaining tasks are:

nikomatsakis

Gave a quick skim -- one thing I was expecting to see which I didn't yet see is something like

/// Wraps another `RustIrDatabase` (`DB`) and monitors the calls that occur.
/// When the `LoggingRustIrDatabase` is dropped, it writes a serialized
/// version of Rust program to `W`.
struct LoggingRustIrDatabase<I, W, DB>
where DB: RustIrDatabase<I>, W: Write, I: Interner
{

}

I guess we probably want to wrap the solver too, ultimately, so we can track the goals...

nikomatsakis · 2020-05-04T18:12:59Z

chalk-solve/src/split.rs

+    fn split_associated_ty_parameters<'p, P>(
+        &self,
+        parameters: &'p [P],
+        associated_ty_datum: &AssociatedTyDatum<I>,


Maybe split_projection should call this function? Seems like some duplication.

Thanks for catching that!

nikomatsakis · 2020-05-04T18:14:31Z

chalk-integration/src/program_writer.rs

+}
+
+#[cfg(test)]
+mod test {


These should probably live in the tests directory

Thanks! Right now this file is mostly just a stopgap so we could write tests before figuring out the tests directory structure.

nikomatsakis · 2020-05-04T18:14:45Z

chalk-integration/src/program_writer.rs

+
+use crate::program::Program;
+
+pub trait WriteProgram {


I'm not really clear on what the role of this trait is

It's purely for the tests, and should be removed as we create the logging database

daboross · 2020-05-10T03:39:51Z

Thank you for the quick review! As you could probably tell, there are quite a few things left in the codebase that are holdovers from WIP code. I've replied to the individual notes.

Gave a quick skim -- one thing I was expecting to see which I didn't yet see is something like

/// Wraps another `RustIrDatabase` (`DB`) and monitors the calls that occur.
/// When the `LoggingRustIrDatabase` is dropped, it writes a serialized
/// version of Rust program to `W`.
struct LoggingRustIrDatabase<I, W, DB>
where DB: RustIrDatabase<I>, W: Write, I: Interner
{

}

This is on our todo list - we started by implementing & testing the display code separately, but this is the end goal. But thank you for the definition!

If all goes well, we should be pushing up a first implementation tonight.

Based on feedback from niko matsakis: rust-lang#430 (comment) `assocated_ty_parameters` re-implemented some logic in `split_projection`. This deletes the duplicated in `split_projection` and instead calls `split_associated_ty_parameters`.

super-tuple · 2020-05-17T08:06:30Z

We are mostly complete, but there are a few outstanding design decisions, which we have described on zulip:

RustIrDatabase::name_* methods: https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/.2Echalk.20file.20writer/near/197021015
Wrapping ChalkDatabase: https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/.2Echalk.20file.20writer/near/197826479

Thanks for all the assistance, we are really excited to contribute!

Based on feedback from niko matsakis: rust-lang#430 (comment) `assocated_ty_parameters` re-implemented some logic in `split_projection`. This deletes the duplicated in `split_projection` and instead calls `split_associated_ty_parameters`. Co-authored-by: David Ross <[email protected]>

daboross · 2020-05-24T08:09:40Z

I think we've made good progress on this today, and with this commit we've fixed all but two of the outstanding issues - the remaining two being *_name methods, and a bug in the LoggerIrDatabase (more details in chalk).

There are still features missing, mostly because of the backlog of dealing with new features added to chalk. But I think these will continue to pile up, and our strategy for implementing them shouldn't be dramatically different from the existing code.

Besides those issues, though, this should be ready. If either of you have time this week, do you think you could look over it?

jackh726

Just skimmed through. A couple comments, but nothing really catches my eye. This is well done and has great test coverage.

jackh726 · 2020-05-27T06:01:39Z

tests/test_util/display.rs

+    };
+}
+
+pub fn write_program(program: &Program) -> String {


This looks pretty similar to the write_program function in chalk-solve/display?

It is similar - good catch, we missed that one. One operates on the interface provided by Program, the other RustIrLoggingDb. I think we can write a function that transforms the data in a Program into a list of RecordedItemIds similar to what we have in the logging db, and then provide that to chalk-solve::display::write_program, eliminating the duplication of the writing logic. I'll add this to the pull request task list.

jackh726 · 2020-05-27T06:03:08Z

tests/test_util/display.rs

+    out
+}
+
+fn program_diff(original: &impl Debug, produced: &impl Debug) -> String {


Wait are we actually diffing the program text? I don't think this is really helpful. Moreso, we just want the goals to be solvable (or fail) in the same way.

It's diffing the Debug output of Program! I'll add some comments to it at least. This particular function is just nice output if we fail - it isn't a test. The actual test is a old_lowered_program == new_lowered_program test, though.

We decided to use an exact comparison between Programs because it allowed us to really easily write a bunch of different tests without having to specify goals for every one, and that let us cover a lot more ground with less test code. With this, we can just test that a struct with fields outputs the same struct with fields, without having to figure out what exactly in chalk uses fields and to rely on that code as well.

It seems reasonable that the end goal is to keep goals solveable the same way. To that end, I'd consider these as unit tests. They test the exact behavior of this specific display code, and then the logging_db tests can assume that we render items correctly, and can focus on ensuring we get everything necessary for goals to pass (or fail).

Does that seem reasonable? We could also revise this testing strategy, but it would mean rewriting most of the display tests so that they have reasonable goals to follow.

Yeah, honestly the test coverage is really good. And it seems pretty reasonable to just check that old_lowered_program == new_lowered_program. I was originally skeptical about this, because I thought it would end up being dificult. But it doesn't seem so.

jackh726 · 2020-05-27T06:04:27Z

tests/test/mod.rs

-        });
-    }
-}
+pub use super::test_util::{test::*, *};


Why move these? I'm not entirely against this change, but I think this PR is big enough that if you want to do this, splitting it into a separate PR would be helpful.

Right - I think we wanted to be able to use the internals from test! in logging_db_output_sufficient! defined test_util/logging_db.rs, and I think there were problems using macros from tests/test/mod.rs in tests/test_util/logging_db.rs with test/ already using macros from test_util.rs.

This change is a separate commit, so making a separate PR wouldn't be hard at all. We could also revert it and work around this some less invasive way - like putting the macro for logging_db in tests/logging_db/mod.rs and just letting the macro dependency chain be logging_db -> test -> test_util.

We've removed this change, and can make another PR to do the misc. fixing later.

jackh726 · 2020-05-27T06:06:17Z

tests/test_util/logging_db.rs

+    }};
+}
+
+pub fn logging_db_output_sufficient(


A little bit of a verbose function body, but it gets the job done.😂

jackh726 · 2020-05-27T06:09:32Z

tests/logging_db/mod.rs

@@ -0,0 +1,164 @@
+#[test]


Other tests I would like to see:

Coverage of all the builtin types (e.g. scalars, str, never, etc.)

Coverage of functions (fn ptr and fn def)

It might be useful to "wrap" the main tests locally just to have a broader test set, to see if we missed anything.

Welp I just scrolled up and saw these are coveraged. Well done :)

It might be useful to "wrap" the main tests locally just to have a broader test set, to see if we missed anything.

We tried to do this at first, but found difficulty hooking into the existing test function which used ChalkDatabase to run everything - we talked a bit about this on zulip. It'd be possible to write this in solve_goal, and use some of the suggestions (like maybe putting a logging flag in ChalkDatabase). However, I thought that it would lead to diminishing returns compared to tests directed at LoggingIrDatabase, and the huge amount of extra test harness code wouldn't be worth it.

My biggest objection was that running it on the existing test suite wouldn't hit too many extra edge cases, since we'd be running it on tests which weren't designed to test LoggingIrDatabase edge cases. If the cost is too great, like changing things in ChalkDatabase, then it'd be a bust.

But thinking about this again, there might be a reasonable strategy. We could just run our test harness at the end of solve_goal, rather than trying to integrate our tests into solve_goal. To run this on all tests, we would need to essentially fully duplicate solve_goal in our test code. But our current code still partially duplicates it, and the only reason it isn't full duplication is to stay minimal.

If it's OK that we fully duplicate solve_goal's functionality, then we'll do that.

Nah, don't worry about it.

jackh726 · 2020-05-27T06:10:33Z

tests/display/where_clauses.rs

+                        dyn Bar: Baz<Foo>;
+            }
+        }
+        produces {


Theoretically, if we do want to do this, why not just check that the program outputs itself?

The problem here is that going through the loop always adds additional clauses to an AliasEq where clause. Lowering always transforms Foo: Bax<T, BaxT=T> into Foo: Bax<T, BaxT=T>, Foo: Bax<T>, and trying to undo that seemed like more work than it'd be worth.

Even if we included the output program as the source, it'd just add another Foo: Bax<T> clause and we'd be in the same place.

I'll add a comment to that to the test code, though. That's useful information that we didn't include!

jackh726 · 2020-05-27T06:15:35Z

chalk-solve/src/logging_db.rs

+///
+/// Uses a separate type, `P`, for the database stored inside to account for
+/// `Arc` or wrapping other storage mediums.
+pub struct WriteOnDropRustIrDatabase<I, W, DB, P = DB>


Why do we need this? I think it's okay to just make the user output before the LoggingDatabase is dropped manually.

In a previous comment @nikomatsakis suggested we make the logging database write on drop. We opted to refactor this behavior out into a separate wrapper since we felt that it was distinct, and there are likely use-cases for just logging. Do you think it's worth having?

Hmm, I guess it's simple enough to keep.

jackh726 · 2020-05-27T06:16:51Z

chalk-solve/src/lib.rs

@@ -101,6 +103,19 @@ pub trait RustIrDatabase<I: Interner>: Debug {

    /// Check if a trait is object safe
    fn is_object_safe(&self, trait_id: TraitId<I>) -> bool;
+
+    /// Retrieves a trait's original name. No uniqueness guarantees.
+    /// TODO: remove this, use only interner debug methods


I think this is probably fine to stay. At least for this PR. (Maybe we can have a followup PR that makes these optional and adds fallback support)

I still think this is fine to stay for now. But I would like to see a followup PR to use methods on Interner.

Based on feedback from niko matsakis: rust-lang#430 (comment) `assocated_ty_parameters` re-implemented some logic in `split_projection`. This deletes the duplicated in `split_projection` and instead calls `split_associated_ty_parameters`. Co-authored-by: David Ross <[email protected]>

daboross · 2020-05-31T06:19:08Z

By the way, thanks for the review! Didn't say that earlier. I realize this has gotten to be quite a large PR in terms of lines of code.

We made some progress today, but we spent more time thinking than fixing the code. We rebased and added support for additional constructs, but didn't implement the fixes for the two big issues mentioned above: LoggingRustIrDatabase not including everything referenced, and missing support for const values.

With the design guidance from the last zulip meeting, I think we now have everything we need to fix those two issues. We'll focus directly on implementing them next time we work on this, and that will either be next Saturday or the Saturday after that.

Based on feedback from niko matsakis: rust-lang#430 (comment) `assocated_ty_parameters` re-implemented some logic in `split_projection`. This deletes the duplicated in `split_projection` and instead calls `split_associated_ty_parameters`. Co-authored-by: David Ross <[email protected]>

daboross · 2020-06-21T09:30:33Z

We've finally gotten time to work on this this weekend, and have started implementing the fix for logging_db missing items! Mainly, finding IDs we're missing, and outputting stub items.

We also removed the big unnecessary tests/test/mod.rs -> tests/test_util/test.rs refactor from this PR.

I think we discussed merging this without fully supporting everything - if that's alright, then I think the only remaining blockers are:

one known bug in the stub items code: It doesn't pick up trait associated item bounds
a general lack of tests for the logging db

The display code is well tested, but the logging_db gathering framework is not. We barely have 2 working tests... It should at least have one for each top level item it can record, and some covering the intricacies of stub items.

The PR is also now re-rebased onto master.

jackh726 · 2020-06-21T13:37:28Z

Indeed I do think this is worth merging, even for partial functionality. @daboross how would you feel if I just merged this and opened issue(s) for followup? I have to go through the code again, but last time I looked, it was fairly complete.

I'm mostly just worried about this bitrotting. Especially given that Chalk is very much a moving target right now in terms of RustIrDatabase stability.

(Also, go ahead and switch this to not be a draft)

Based on feedback from niko matsakis: rust-lang#430 (comment) `assocated_ty_parameters` re-implemented some logic in `split_projection`. This deletes the duplicated in `split_projection` and instead calls `split_associated_ty_parameters`. Co-authored-by: David Ross <[email protected]>

daboross · 2020-06-28T10:39:03Z

Sorry I didn't see and respond to this earlier! Had a busy week. I'll try to be on more this coming work week.

Since we didn't realize, we've got another round of fixes from today.

If you've got time to review it (or start to review it, at least) we're good with that. Next week, we could also limit our changes to rebase + minimal fixes for that if you want to start a review & it isn't completely compiling when the review is done.

Added required trait methods to the logging db wrappers so they behave correctly. Also added a todo macro in the rendering code for Closures. Co-authored-by: David Ross <[email protected]>

For a function that takes u32 as an arg and returns a unit, it outputs: fn(u32) -> () The inputs and outputs are inside a Binders. We are not totally sure why this is, but we had increment the debrujin index as a result. Co-authored-by: David Ross <[email protected]>

There were serveral places where we had code like: ``` .collect<Vec<String>>().join(", ") ``` This is not optimal as it allocates twice, and is quite verbose. We replaced it with Itertools::format, which returns something implementing display, and will lazily write to the formatter without allocating. Co-authored-by: David Ross <[email protected]>

The current chalk syntax mandates that every dyn be follow by a liftime bound: ``` dyn Foo + 'a ``` This updates the rendering code and tests to support this. Co-authored-by: David Ross <[email protected]>

This implements the suggested fix where if LoggingRustIrDatabase finds an name in the output which it isn't already outputting, it'll prepend a "stub" version of that thing - it'll have the right generics, but no where bounds and no content. Structs are empty, traits have no methods, etc. This doesn't entirely work - mainly, there are some bugs with associated type bounds not being visited to find missing IDs, and we don't have enough tests - but the framework is there! Co-authored-by: David Ross <[email protected]>

The logging db that wraps RustIrDatabase was not collecting the ids for items referenced in associated types. We extended the id collector to convert an associated type's inline bounds to a where clause, which is then visited using the existing where clause visitor to pickup the referenced ids. Co-authored-by: David Ross <[email protected]>

Fix needed after rebasing program-writer branch on latest master since `LoggingRustIrDatabase` implements `RustIrDatabase`. Co-authored-by: David Ross <[email protected]>

The logging db that wraps RustIrDatabase was not collecting the ids for items referenced in assocated type values. The visitor we are repurposing from the solver does not cross identifier boundaries. The `AssocTyDatum` stores the values as a set of ids, so they aren't visited. We extended the id collector to resolve the assoc ty value ids in an impl block, and then visit them. Co-authored-by: David Ross <[email protected]>

Refactor the flag macro out of the TraitDatum impl, since it is also needed for struct flags. Additionally, we altered the macro to leverage struct destructuring exhaustiveness to error if any flags are missing. Co-authored-by: David Ross <[email protected]>

Adds the phantom_data, upstream, and fundamental flags: ``` struct Bar<T> {} ``` Co-authored-by: David Ross <[email protected]>

Adds rendering for the `#[upstream]` flag on impls. Co-authored-by: David Ross <[email protected]>

Added documentation to describe the purpose and scope of each of the display sub modules. Co-authored-by: David Ross <[email protected]>

This supports #[repr(C)] and #[repr(Rust)] on adts. Co-authored-by: David Ross <[email protected]>

Added rendering for well known trait markers, such as: ``` trait Sized { } ``` Co-authored-by: David Ross <[email protected]>

The object_safe flag is handled differently to the other flags. It is accessed by calling a seperate method, `RustIrDatabase::is_object_safe` instead of being included in the `TraitFlags` data structure. Co-authored-by: David Ross <[email protected]>

The following improvements were made: * Added a documentation comment for tests/logging_db/mod.rs * Renamed some methods for clarity * Updated outdated documentation comment * Re-enabled tests that are now working Co-authored-by: David Ross <[email protected]>

This adds rendering for using an opaque type as a type, for instance, OpaqueTy<Arg>. Co-authored-by: David Ross <[email protected]>

Added code to collect the identifiers of opaque ty values + test. Co-authored-by: David Ross <[email protected]>

We were stubbing collected ids (ids that were not directly captured by the logging db but were needed for name resolution to work) by cloning top level item data structures, removing bounds and other references to addtional types, then feeding them into the display code in a method called `write_stub_items`. This approach was not successful, because we were unable to remove certain bounds, like those on associated types, using this method because that data is read from the DB by the display code, rather than getting passed down, making it untouchable. To work around this, we have created yet another RustIrDatabase wrapper that will return data structures with bounds etc. removed. Co-authored-by: David Ross <[email protected]>

Collects ids of opaque_tys used as tys in logged items. For isntance, if bar is logged, we will collect Baq's id: ``` opaque type Baq: Foo + Fuu = Baz; opaque type Bar: Foo + Fuu = Baq; ^^^ ^^^ | logged | collected ``` Co-authored-by: David Ross <[email protected]>

jackh726

Ok I've left a couple comments. But this is a lot, so I probably wasn't completely thorough. However, since all of this (except of the extra functions in RustIrDatabase) are completely optional, I'm good with merging this now, and I'll file an issue to work on the comments here.

Additionally, I think I might like to see this under a feature flag.

jackh726 · 2020-06-29T17:34:43Z

chalk-solve/src/lib.rs

@@ -101,6 +103,19 @@ pub trait RustIrDatabase<I: Interner>: Debug {

    /// Check if a trait is object safe
    fn is_object_safe(&self, trait_id: TraitId<I>) -> bool;
+
+    /// Retrieves a trait's original name. No uniqueness guarantees.
+    /// TODO: remove this, use only interner debug methods


I still think this is fine to stay for now. But I would like to see a followup PR to use methods on Interner.

jackh726 · 2020-06-29T17:40:53Z

chalk-solve/src/display.rs

+
+use self::utils::as_display;
+
+pub fn write_item<F, I, T>(f: &mut F, ws: &WriterState<'_, I>, v: &T) -> Result


This probably doesn't have to be pub.

jackh726 · 2020-06-29T18:10:20Z

chalk-solve/src/display/bounds.rs

+    }
+}
+
+/// This implementation correct inside where clauses.


This doesn't make sense?

jackh726 · 2020-06-29T18:10:33Z

chalk-solve/src/display/bounds.rs

+    }
+}
+
+/// This implementation correct inside where clauses.


jackh726 · 2020-06-29T18:24:02Z

chalk-solve/src/display/items.rs

+            impl_param_names_in_impl_env,
+        );
+
+        // let params = s


This comment can be removed

jackh726 · 2020-06-29T18:49:46Z

tests/display/assoc_ty.rs

@@ -0,0 +1,305 @@
+#[test]


Just my general thoughts on these tests: great coverage, but I think I would like these to be combined when possible. For example, the test below could just merge the two programs.

Addtionally, comments for what each test/program is for would be nice.

Removed and re-wrote comments based on feedback from rust-lang#430 Co-authored-by: David Ross <[email protected]>

nikomatsakis reviewed May 4, 2020

View reviewed changes

super-tuple force-pushed the program-writer branch from 3cb6e6e to f340e49 Compare May 9, 2020 19:12

super-tuple marked this pull request as draft May 10, 2020 01:49

super-tuple force-pushed the program-writer branch from f340e49 to 355778f Compare May 10, 2020 03:53

super-tuple force-pushed the program-writer branch from 6112afd to 1e7e116 Compare May 17, 2020 03:45

super-tuple force-pushed the program-writer branch 4 times, most recently from eab5500 to 3f667a9 Compare May 24, 2020 07:29

jackh726 reviewed May 27, 2020

View reviewed changes

super-tuple force-pushed the program-writer branch from 42d8b01 to a5c4463 Compare May 31, 2020 06:06

daboross force-pushed the program-writer branch from a5c4463 to 6982886 Compare June 21, 2020 05:32

super-tuple force-pushed the program-writer branch from 6982886 to a5c4463 Compare June 21, 2020 06:01

daboross force-pushed the program-writer branch from a5c4463 to f751141 Compare June 21, 2020 06:43

super-tuple force-pushed the program-writer branch from f751141 to 382da88 Compare June 21, 2020 09:23

jackh726 marked this pull request as ready for review June 23, 2020 17:21

daboross force-pushed the program-writer branch from 382da88 to c1e793d Compare June 28, 2020 10:27

super-tuple and others added 20 commits June 28, 2020 03:41

Add todo in LoggingDB for closures

427b948

Added required trait methods to the logging db wrappers so they behave correctly. Also added a todo macro in the rendering code for Closures. Co-authored-by: David Ross <[email protected]>

Add dyn trait lifetime bounds rendering

1858d6a

The current chalk syntax mandates that every dyn be follow by a liftime bound: ``` dyn Foo + 'a ``` This updates the rendering code and tests to support this. Co-authored-by: David Ross <[email protected]>

Add adt_repr to Logging DB

fa55728

Fix needed after rebasing program-writer branch on latest master since `LoggingRustIrDatabase` implements `RustIrDatabase`. Co-authored-by: David Ross <[email protected]>

Add struct flag rendering

83304fa

Adds the phantom_data, upstream, and fundamental flags: ``` struct Bar<T> {} ``` Co-authored-by: David Ross <[email protected]>

Add impl type flag rendering

949c75a

Adds rendering for the `#[upstream]` flag on impls. Co-authored-by: David Ross <[email protected]>

Document display sub modules

06d5e4b

Added documentation to describe the purpose and scope of each of the display sub modules. Co-authored-by: David Ross <[email protected]>

Add struct repr rendering

e656484

This supports #[repr(C)] and #[repr(Rust)] on adts. Co-authored-by: David Ross <[email protected]>

Implement rendering for well known traits

750733f

Added rendering for well known trait markers, such as: ``` trait Sized { } ``` Co-authored-by: David Ross <[email protected]>

Add opaque type Ty rendering

024a8a4

This adds rendering for using an opaque type as a type, for instance, OpaqueTy<Arg>. Co-authored-by: David Ross <[email protected]>

Fix missing opaque ty stubs

e0f325e

Added code to collect the identifiers of opaque ty values + test. Co-authored-by: David Ross <[email protected]>

daboross force-pushed the program-writer branch from c1e793d to 232823b Compare June 28, 2020 10:46

super-tuple changed the title ~~[WIP] .chalk file syntax writer~~ .chalk file syntax writer Jun 28, 2020

jackh726 approved these changes Jun 29, 2020

View reviewed changes

jackh726 merged commit 77bcba2 into rust-lang:master Jun 29, 2020

jackh726 mentioned this pull request Jun 29, 2020

Add ability to output a .chalk file for reproducing bugs #365

Closed

daboross deleted the program-writer branch July 4, 2020 18:56

super-tuple added a commit to daboross/chalk that referenced this pull request Jul 4, 2020

Cleanup comments in syntax writer

1b57421

Removed and re-wrote comments based on feedback from rust-lang#430 Co-authored-by: David Ross <[email protected]>

super-tuple mentioned this pull request Jul 4, 2020

Cleanup comments in syntax writer #558

Merged

daboross mentioned this pull request Jul 11, 2020

Cleanup & comment .chalk file writer tests #570

Merged

daboross mentioned this pull request Jul 18, 2020

Add default RustIrDatabase::*_name impls #575

Merged


		use self::utils::as_display;

		pub fn write_item<F, I, T>(f: &mut F, ws: &WriterState<'_, I>, v: &T) -> Result

.chalk file syntax writer #430

.chalk file syntax writer #430

Conversation

super-tuple commented May 3, 2020 • edited Loading

nikomatsakis 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

daboross commented May 10, 2020 • edited Loading

super-tuple commented May 17, 2020

daboross commented May 24, 2020

jackh726 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

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

daboross commented May 31, 2020

daboross commented Jun 21, 2020

jackh726 commented Jun 21, 2020 • edited Loading

daboross commented Jun 28, 2020

jackh726 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

super-tuple commented May 3, 2020 •

edited

Loading

daboross commented May 10, 2020 •

edited

Loading

jackh726 commented Jun 21, 2020 •

edited

Loading