(2/4) Override Stream ReadAsync/WriteAsync Analyzer #4726

NewellClark · 2021-01-21T18:54:49Z

I ran the analyzer against dotnet/runtime and found several violations:

DeflateStream, ChunkedMemoryStream, RequestStream, and NetworkStreamWrapper were all trivial, and have been dealt with in this PR.
CryptoStream was non-trivial, and is being worked on in this PR.
DeflateManagedStream is internal, and calls into the IFileFormatReader interface, which is a dead interface and is being removed in this PR. Once that gets merged, I'll memorify DeflateManagedStream, which will have no extra copies. Edit: I've memorified DeflateManagedStream (PR).
WebSocketHttpListenerDuplexStream is also internal, and from what I could find, ReadAsync/WriteAsync are only ever called from WebSocketBase, which only calls the array-based override. Unfortunately, WebSocketHttpListenerDuplexStream also calls back into WebSocket, and it passes the array it was given into multiple different methods on WebSocket, and some methods on WebSocketBuffer. Providing memory-based overrides would require almost a complete rewrite of WebSocketBuffer as well as changes to code that consumes it. Since nothing currently calls the memory-based overrides on WebSocketHttpListenerDuplexStream, I think we should strongly consider suppressing the warning.

Note that I'm using CA1840 as the diagnostic ID because I used CA1839 in another analyzer that has yet to be merged in. Not sure how that type of situation is typically handled.

Fix issue #33789

…ssue-33789

NewellClark · 2021-01-21T18:58:46Z

@mavasani Sorry about that. I've retargeted this PR to the correct branch.

codecov · 2021-01-21T19:48:53Z

Codecov Report

Merging #4726 (4d7967b) into release/6.0.1xx (0f43809) will increase coverage by 0.01%.
The diff coverage is 98.43%.

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #4726      +/-   ##
===================================================
+ Coverage            95.56%   95.57%   +0.01%     
===================================================
  Files                 1210     1212       +2     
  Lines               276014   277225    +1211     
  Branches             16724    16737      +13     
===================================================
+ Hits                263785   264971    +1186     
- Misses               10007    10020      +13     
- Partials              2222     2234      +12

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

Youssef1313 · 2021-01-22T09:25:48Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+            return containingType.GetMembers(methodName)
+                .SingleOrDefault(symbol => symbol is IMethodSymbol m && IsMatch(m, argumentTypes)) as IMethodSymbol;
+
+            static bool IsMatch(IMethodSymbol method, ITypeSymbol[] argumentTypes)


@Evangelink Was there a helper method that does this?

The helper method requires instances of ParameterInfo, which themselves require INamedTypeSymbol instances. It was significantly shorter to write it out my way, which allows any ITypeSymbol to be used. I did originally write it out using the helper method, but it was significantly longer and more verbose.

…ideStreamMemoryBasedAsyncOverrides.cs Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>

If there are multiple definitions of the same overridden method, the analyzer can crash due to uses of `SingleOrDefault` instead of `FirstOrDefault`. To fix, I refactored `GetOverload` and `GetSymbolForOverridingMethod` into immutable-array-returning methods, and simply used `FirstOrDefault` or `SingleOrDefault` at each call-site as appropriate.

Evangelink · 2021-01-23T15:48:00Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -1510,4 +1510,14 @@
    <value>and all other platforms</value>
    <comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment>
  </data>
+  <data name="ProvideStreamMemoryBasedAsyncOverridesDescription" xml:space="preserve">
+    <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving your implementation to '{2}' and have '{1}' delegate to '{2}'.</value>


Seems like you missed the {0} in this declaration.

I used the {0} in the message field, but not in the description.
Edit: I've changed the description so it includes the {0} argument.

Evangelink · 2021-01-23T15:50:17Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+
+            context.RegisterSymbolAction(AnalyzeNamedType, SymbolKind.NamedType);
+
+            void AnalyzeNamedType(SymbolAnalysisContext context)


Except if I have missed something, this local func could be marked as static.

It uses the symbols struct produced by the call to TryGetRequiredSymbols(...) on line 55.

Evangelink · 2021-01-23T15:51:58Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+                return Diagnostic.Create(
+                    Rule,
+                    violatingType.Locations[0],
+                    violatingType.Locations.Skip(1),


Please use the CreateDiagnostic helper method.

Suggested change

return Diagnostic.Create(

Rule,

violatingType.Locations[0],

violatingType.Locations.Skip(1),

return violatingType.CreateDiagnostic(

Rule,

I've fixed this in the latest commit.

Evangelink · 2021-01-23T15:54:32Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+
+            // We know that there are no compiler errors on streamType, so we use 'SingleOrDefault'.
+
+            var readAsyncArrayMethod = GetOverloads(streamType, ReadAsyncName, byteArrayType, int32Type, int32Type, cancellationTokenType).SingleOrDefault();


I would still avoid the SingleOrDefault here as it makes it harder to debug if you end up with actually more than one item.

What's your view on this @mavasani ?

I figured SingleOrDefault since we're looking up methods on framework types that are in metadata, and therefore could never contain compiler errors. Now that I think about it though, if this analyzer is running against System.Private.CoreLib, these framework types could actually contain compiler errors, and we definitely don't want the analyzer to crash if that happens. I suppose we could detect whether streamType is in source code or metadata, but it's probably not worth the hassle. I'll change this to FirstOrDefault for now.

Edit: I've switched to FirstOrDefault

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

Evangelink · 2021-01-23T15:57:46Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs

+{
+    public class ProvideStreamMemoryBasedAsyncOverridesTests
+    {
+        #region Reports Diagnostic


[cosmetic] @mavasani Shall we keep introducing these regions?

Evangelink · 2021-01-23T16:00:26Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs

+            string code = $@"
+{CSUsings}
+namespace Testopolis
+{{
+    public class {{|#0:FooStream|}} : Stream
+    {{
+        {CSAbstractMembers}
+        {CSReadAsyncArray}
+    }}
+}}";


[cosmetic] I am fine if you keep it this way but I find the double curly braces hard to read.

Suggested change

string code = $@"

{CSUsings}

namespace Testopolis

{{

public class {{|#0:FooStream|}} : Stream

{{

{CSAbstractMembers}

{CSReadAsyncArray}

}}

}}";

string code = @"

" + CSUsings + @"

namespace Testopolis

{

public class {|#0:FooStream|} : Stream

{

" + CSAbstractMembers + @"

" + CSReadAsyncArray + @"

}

}";

Going forward, I will do it your way.

Let's wait for @sharwell or @mavasani opinion here.

NewellClark · 2021-01-24T15:23:37Z

This should be linked to issue #33789.

Youssef1313 · 2021-01-24T15:35:18Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+            var int32Type = compilation.GetSpecialType(SpecialType.System_Int32);
+            var byteType = compilation.GetSpecialType(SpecialType.System_Byte);


@mavasani Can these be null under any case?

Yes, they can be null for broken compilation references. We should have a null check for these types as well.

@mavasani Fixed. Sorry I missed this earlier.

Youssef1313 · 2021-01-24T15:42:29Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs

+                    VerifyVB.Diagnostic(Rule)
+                        .WithLocation(0)
+                        .WithLocation(1)
+                        .WithArguments("River", displayArrayMethod, displayMemoryMethod)


This is very confusing. Is this supposed to be two diagnostics?
I believe that only one diagnostic is passed to the test.

The location that is underlined when this diagnostic is reported is the name of the class in the class declaration. If the class is a partial class, all partial declarations need to be underlined. That is what this test is checking: that when you have multiple partial declarations in the same file, all partial declarations will be underlined for a single violation.

@NewellClark If I understand correctly, you're passing only one diagnostic. When you chain WithLocation calls, only the latest one takes effect.

I believe if you removed the chain and kept the latest WithLocation call only, the test will still pass. Could you confirm that locally on your machine?

Just tested it. The test fails if we remove any of the WithLocation calls, and the error message reports the diagnostics at the removed locations as the unexpected diagnostics.

@NewellClark Good to know! Thanks.

@Youssef1313 by the way, would it be possible to have this pull request linked to issue 33789?

@NewellClark The issue won't be auto-closed when the PR is merged. But a Microsoft employee will close it when he merges your PR.

Youssef1313 · 2021-01-24T15:43:28Z

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs

+                        .WithLocation(0)
+                        .WithLocation(1)
+                        .WithLocation(2)


Same comment here.

Same as my previous comment, but this time we are checking whether all locations are reported when the partials are in separate files.

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs

mavasani · 2021-04-13T15:57:43Z

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx

@@ -1510,4 +1510,14 @@
    <value>and all other platforms</value>
    <comment>This call site is reachable on: 'windows' 10.0.2000 and later, and all other platforms</comment>
  </data>
+  <data name="ProvideStreamMemoryBasedAsyncOverridesDescription" xml:space="preserve">
+    <value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value>


Suggested change

<value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value>

<value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns 'Task' instead of 'ValueTask'. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value>

Actually, you cannot have format arguments {0}, {1}, etc. in description string, it is a static description per-descriptor, not something instantiated per-diagnostic. You should instead follow what @carlossanlop did in a similar analyzer and use more generic description string that is applicable for all violations.

mavasani · 2021-04-13T16:05:46Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+            context.RegisterSymbolAction(AnalyzeNamedType, SymbolKind.NamedType);
+
+            void AnalyzeNamedType(SymbolAnalysisContext context)


Suggested change

context.RegisterSymbolAction(AnalyzeNamedType, SymbolKind.NamedType);

void AnalyzeNamedType(SymbolAnalysisContext context)

context.RegisterSymbolAction(AnalyzeNamedType, SymbolKind.NamedType);

return;

// Local functions

void AnalyzeNamedType(SymbolAnalysisContext context)

mavasani · 2021-04-13T16:11:55Z

src/NetAnalyzers/RulesMissingDocumentation.md

@@ -2,3 +2,4 @@

 Rule ID | Missing Help Link | Title |
 --------|-------------------|-------|
+CA1840 | <https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1840> | Provide memory-based overrides of async methods when subclassing 'Stream' |


ID has already been used, please choose a new one.

Changed ID to ca1844

mavasani

Overall looks great. Please retarget to preview4 branch.

- Change ID to CA1842

mavasani · 2021-05-17T07:46:26Z

@NewellClark Sorry for not getting back to this PR review sooner, but unfortunately even preview4 is now locked. Can you please retarget to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx? Promise this one is the last branch retarget request. Again, very sorry for this additional work.

Can you also address the remaining review comments? I should be able to merge once both of these are taken care of.

Fix possible crash when primitive types can't be loaded

NewellClark · 2021-05-17T15:47:31Z

@NewellClark Sorry for not getting back to this PR review sooner, but unfortunately even preview4 is now locked. Can you please retarget to https://github.com/dotnet/roslyn-analyzers/tree/release/6.0.1xx? Promise this one is the last branch retarget request. Again, very sorry for this additional work.

Can you also address the remaining review comments? I should be able to merge once both of these are taken care of.

@mavasani Fixed. Should I retarget for all my other analyzer PRs that are targeted to preview4?

mavasani · 2021-05-17T15:58:21Z

Should I retarget for all my other analyzer PRs that are targeted to preview4?

Yes please. preview4 branch is now locked.

mavasani · 2021-05-18T05:49:05Z

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs

+
+                return violatingType.CreateDiagnostic(
+                    Rule,
+                    violatingType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat),


We normally use just the type/method .Name in diagnostic messages. MinimallyQualifiedFormat actually produces a qualified name, which lead to very long names.

@mavasani Fixed. I've changed the message so it makes sense with simple names (the message was originally worded with the assumption that the arguments of each overload would be displayed).

mavasani

LGTM.

I can merge once you change the diagnostic messages to use simple names instead of ToDisplayString.

- Remove usages of `MinimallyQualifiedFormat` - Change message so it makes sense with simple names

mavasani

Thanks!

mavasani · 2021-05-18T16:21:09Z

@NewellClark Can you please help us in creating documentation PRs for all the new .NET6 analyzers that you have contributed? Please see the item 4. in https://github.com/dotnet/roslyn-analyzers/blob/main/GuidelinesForNewRules.md.

Also tagging @gewarren as we now have 8 new undocumented CA rules which have been implemented in the release\6.0.1xx branch for .NET6: https://github.com/dotnet/roslyn-analyzers/blob/release/6.0.1xx/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md.

NewellClark · 2021-05-18T17:50:54Z

@NewellClark Can you please help us in creating documentation PRs for all the new .NET6 analyzers that you have contributed? Please see the item 4. in https://github.com/dotnet/roslyn-analyzers/blob/main/GuidelinesForNewRules.md.

Also tagging @gewarren as we now have 8 new undocumented CA rules which have been implemented in the release\6.0.1xx branch for .NET6: https://github.com/dotnet/roslyn-analyzers/blob/release/6.0.1xx/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md.

Absolutely. I'm on it.

NewellClark added 4 commits January 15, 2021 12:48

Implement analyzer with basic tests

a67832e

Added more tests

fbc706d

Add ProvideStreamMemoryBasedAsyncOverrides analyzer

8b9cf92

Fix issue #33789

Merge remote-tracking branch 'dotnet/release/6.0.1xx-preview1' into i…

b6604e7

…ssue-33789

NewellClark requested a review from a team as a code owner January 21, 2021 18:54

mavasani requested a review from carlossanlop January 21, 2021 19:00

mavasani assigned carlossanlop Jan 21, 2021

Forgot to MSBuild

fe1d3b9

Youssef1313 reviewed Jan 22, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 22, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 22, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs Show resolved Hide resolved

Youssef1313 reviewed Jan 22, 2021

View reviewed changes

...Analyzers/Core/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryBasedAsyncOverrides.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 22, 2021

View reviewed changes

NewellClark and others added 3 commits January 22, 2021 10:29

Update src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/Prov…

4f667a7

…ideStreamMemoryBasedAsyncOverrides.cs Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>

Rerun CI

3f64c5e

Evangelink reviewed Jan 23, 2021

View reviewed changes

NewellClark added 3 commits January 23, 2021 12:47

Apply suggested changes

f91a7f0

Rerun CI

f072328

Forgot to msbuild /t:pack

faac3a8

Youssef1313 reviewed Jan 24, 2021

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jan 24, 2021

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs Show resolved Hide resolved

Youssef1313 reviewed Jan 24, 2021

View reviewed changes

...zers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/ProvideStreamMemoryAsyncOverridesTests.cs Show resolved Hide resolved

NewellClark mentioned this pull request Jan 24, 2021

Override Stream.ReadAsync/WriteAsync dotnet/runtime#33789

Closed

mavasani reviewed Apr 13, 2021

View reviewed changes

Apply requested changes

3925794

- Change ID to CA1842

NewellClark changed the base branch from release/6.0.1xx-preview1 to release/6.0.1xx-preview4 April 20, 2021 14:34

NewellClark added 8 commits April 20, 2021 10:43

Merge branch 'release/6.0.1xx-preview4' into issue-33789

761578e

Add newline to end of resx file

a4e6374

Add newline character

f481548

Fix garbled unshipped.md markup

b2dc2cc

Merge branch 'release/6.0.1xx-preview4' into issue-33789

516e775

Change ID to CA1844

31b78e9

Merge branch 'release/6.0.1xx-preview4' into issue-33789

a320e17

Fix failing CA

81eda27

mavasani mentioned this pull request May 17, 2021

(3/4) Use Span-Based String Concat Analyzer and Fixer #4764

Merged

NewellClark changed the base branch from release/6.0.1xx-preview4 to release/6.0.1xx May 17, 2021 15:34

Apply suggested changes

f70c6d8

Fix possible crash when primitive types can't be loaded

mavasani reviewed May 18, 2021

View reviewed changes

mavasani approved these changes May 18, 2021

View reviewed changes

NewellClark added 2 commits May 18, 2021 09:33

Use simple names in diagnostic messages

94ee770

- Remove usages of `MinimallyQualifiedFormat` - Change message so it makes sense with simple names

Merge branch 'release/6.0.1xx' into issue-33789

4d7967b

mavasani approved these changes May 18, 2021

View reviewed changes

mavasani merged commit 4bda814 into dotnet:release/6.0.1xx May 18, 2021

Youssef1313 mentioned this pull request May 18, 2021

Tracking issue for missing documentation of .NET6 CAxxxx rules dotnet/docs#23900

Closed

22 tasks

NewellClark mentioned this pull request May 18, 2021

CA1844 dotnet/docs#24278

Merged


		context.RegisterSymbolAction(AnalyzeNamedType, SymbolKind.NamedType);

		void AnalyzeNamedType(SymbolAnalysisContext context)


		// We know that there are no compiler errors on streamType, so we use 'SingleOrDefault'.

		var readAsyncArrayMethod = GetOverloads(streamType, ReadAsyncName, byteArrayType, int32Type, int32Type, cancellationTokenType).SingleOrDefault();

		var int32Type = compilation.GetSpecialType(SpecialType.System_Int32);
		var byteType = compilation.GetSpecialType(SpecialType.System_Byte);

	<value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns Task instead of ValueTask. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value>
	<value>The default implementation of '{2}' delegates to '{1}', which is innefficient as it requires an extra copy. It also results in extra allocations as it returns 'Task' instead of 'ValueTask'. Consider moving {0}'s implementation to '{2}' and have '{1}' delegate to '{2}'.</value>

(2/4) Override Stream ReadAsync/WriteAsync Analyzer #4726

(2/4) Override Stream ReadAsync/WriteAsync Analyzer #4726

Conversation

NewellClark commented Jan 21, 2021 • edited Loading

NewellClark commented Jan 21, 2021

codecov bot commented Jan 21, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

NewellClark Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark Jan 23, 2021 • 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

NewellClark commented Jan 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NewellClark Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mavasani commented May 17, 2021

NewellClark commented May 17, 2021

mavasani commented May 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mavasani left a comment

Choose a reason for hiding this comment

mavasani commented May 18, 2021

NewellClark commented May 18, 2021

NewellClark commented Jan 21, 2021 •

edited

Loading

codecov bot commented Jan 21, 2021 •

edited

Loading

NewellClark Jan 22, 2021 •

edited

Loading

NewellClark Jan 23, 2021 •

edited

Loading

NewellClark Jan 23, 2021 •

edited

Loading

NewellClark commented Jan 24, 2021 •

edited

Loading

NewellClark Apr 20, 2021 •

edited

Loading