[DeadCodeAnalysis] Add analyzer/fixer to flag and remove unused private members (fields/… by mavasani · Pull Request #29511 · dotnet/roslyn

mavasani · 2018-08-24T18:38:35Z

…methods/properties/events)

Analyzer flags two cases: members with no read/writes and members with only writes.

Members with no read or writes: Type '{0}' has an unused private member '{1}' which can be removed.
Only writes: Type '{0}' has a private member '{1}' which can be removed as the value assigned to it is never used.

Code fix removes the unused member declaration.
Fixes #24225

Open questions:

Current analyzer design uses a single code style option for all members and both the above kinds of unused members. We can potentially have multiple options, but this should probably be done based on feedback.
Should the analyzer use different diagnostic IDs for the above two kinds of unused members? This will mean that the FixAll experience will need multiple iterations for removing unused members.
Should we update the code fix (or have an additional code fix) that also updates the write references for the "only writes" case (2) above? Or is it better to leave the references to break the code so the user can analyze if the value being assigned can also be removed. The PR current chooses the latter approach as it is more conservative and unlikely to cause silent breaks.

…methods/properties/events) Analyzer flags two cases: members with no read/writes and members with only writes. 1. Members with no read or writes: `Type '{0}' has an unused private member '{1}' which can be removed.` 2. Only writes: `Type '{0}' has a private member '{1}' which can be removed as the value assigned to it is never used.` Code fix removes the unused member declaration. Fixes dotnet#24225 Open questions: 1. Current analyzer design uses a single code style option for all members and both the above kinds of unused members. We can potentially have multiple options, but this should probably be done based on feedback. 2. Should the analyzer use different diagnostic IDs for the above two kinds of unused members? This will mean that the FixAll experience will need multiple iterations for removing unused members. 3. Should we update the code fix (or have an additional code fix) that also updates the write references for the case (2) above? Or is it better to leave the references to break the code so the user can analyze if the value being assigned can also be removed. The PR current chooses the latter approach as it is more conservative and unlikely to cause silent breaks.

mavasani · 2018-08-24T18:43:12Z

                OperationBlockActionsCount > 0 ||
-                OperationBlockStartActionsCount > 0;
+                OperationBlockStartActionsCount > 0 ||
+                SymbolStartActionsCount > 0;


Single line change in compiler code: HasAnyExecutableCodeActions conservatively returns true for analyzers with SymbolStartActions as we cannot compute this upfront for an analyzer (need to actually execute the symbol start action for each symbol). #Closed

CyrusNajmabadi · 2018-08-24T19:09:27Z

+using Xunit;
+using Microsoft.CodeAnalysis.CSharp;
+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.AvoidUnusedMembers


the name of this feature does not match our names for other similar features. Should probably be RemoveUnusedMembers. Like RemoveUnusedImports RemoveUnusedVariable, etc. #Resolved

CyrusNajmabadi · 2018-08-24T19:11:56Z

+    <value>Type '{0}' has an unused private member '{1}' which can be removed.</value>
+  </data>
+  <data name="Avoid_Unused_Members_Title" xml:space="preserve">
+    <value>Avoid unused private members.</value>


IDE resource strings should match resource values. This should be "Avoid_unused_private_members".

do we generally have periods in these strings? I dont' think we do...
#Resolved

CyrusNajmabadi · 2018-08-24T19:13:03Z

+                // For instance fields, ensure that the instance reference is being initialized by the constructor.
+                var instanceFieldWrittenInCtor = isInConstructor &&
+                    fieldReference.Instance?.Kind == OperationKind.InstanceReference &&
+                    (!(fieldReference.Parent is IAssignmentOperation) || fieldReference.Parent.Parent?.Kind != OperationKind.ObjectOrCollectionInitializer);


this expression has gotten too complex to understand. Far too many &&s, ||s, and !s. #Resolved

CyrusNajmabadi · 2018-08-24T19:14:00Z

+            CancellationToken cancellationToken)
+        {
+            return FixWithEditorAsync(document, editor, diagnostics, cancellationToken);
+        }


i don't understand why we needed to forward this function to the function you wrote below... All you seemed to do was reorder hte args.. Why not just remove the below funciton and inline all the code into here... #Resolved

CyrusNajmabadi · 2018-08-24T19:14:24Z

+                var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+                var diagnosticSpan = diagnostic.Location.SourceSpan;
+                var symbolName = diagnostic.Properties[AvoidUnusedMembersDiagnosticAnalyzer.UnunsedMemberNameProperty];
+                var symbolKind = diagnostic.Properties[AvoidUnusedMembersDiagnosticAnalyzer.UnunsedMemberKindProperty];


mispelling: "Ununsed" #Resolved

CyrusNajmabadi · 2018-08-24T19:15:00Z

+                var symbolName = diagnostic.Properties[AvoidUnusedMembersDiagnosticAnalyzer.UnunsedMemberNameProperty];
+                var symbolKind = diagnostic.Properties[AvoidUnusedMembersDiagnosticAnalyzer.UnunsedMemberKindProperty];
+
+                var node = GetTopmostSyntaxNodeForSymbolDeclaration(root.FindNode(diagnosticSpan),


i believe we already have location.FindNode

you didn't pass getInnermostNodeForTie. could that cause a problem? #Resolved

CyrusNajmabadi · 2018-08-24T19:15:39Z

+                var symbolKind = diagnostic.Properties[AvoidUnusedMembersDiagnosticAnalyzer.UnunsedMemberKindProperty];
+
+                var node = GetTopmostSyntaxNodeForSymbolDeclaration(root.FindNode(diagnosticSpan),
+                    isSymbolDeclarationNode: n => n != null && semanticModel.GetDeclaredSymbol(n, cancellationToken)?.Name == symbolName);


i'm confused why this helper function would need to be provided. Are there other situations where you pass a different helper? If not, why not have GetTopmostSyntaxNodeForSymbolDeclaration contain this code internally? #Resolved

That method is a virtual function, which is overridden in VB (we need to map the StatementSyntax to BlockSyntax)

In reply to: 212728972 [](ancestors = 212728972)

in general, we don't use that pattern. Instead, we keep things abstract, making C# and VB have to decide what behavior they want. It's weird to have a defualt behavior that is then only used by one language, and then have the other language override it. In essence, this is saying "VB is overriding C#'s behavior". And instead we want to say: it's up to each language to decide what the correct behavior is.

Also see later note: we have a service that already does this work. So i don't think this extension point is needed in this feature. Look up ISymbolDeclarationService :) #Resolved

CyrusNajmabadi · 2018-08-24T19:15:54Z

+            if (fieldDeclarators.Count > 0)
+            {
+                AdjustDeclarators(fieldDeclarators, declarators);
+            }


needs explanatory comments. #Resolved

CyrusNajmabadi · 2018-08-24T19:16:15Z

+        protected virtual SyntaxNode GetTopmostSyntaxNodeForSymbolDeclaration(SyntaxNode syntaxNode, Func<SyntaxNode, bool> isSymbolDeclarationNode)
+        {
+            return syntaxNode.FirstAncestorOrSelf(isSymbolDeclarationNode);
+        }


this helper function seems unnecessary. you could just inline directly into the callsite. #Resolved

It is virtual, overridden in VB.

In reply to: 212729134 [](ancestors = 212729134)

See above:

in general, we don't use that pattern. Instead, we keep things abstract, making C# and VB have to decide what behavior they want. It's weird to have a defualt behavior that is then only used by one language, and then have the other language override it. In essence, this is saying "VB is overriding C#'s behavior". And instead we want to say: it's up to each language to decide what the correct behavior is.

Also see later note: we have a service that already does this work. So i don't think this extension point is needed in this feature. Look up ISymbolDeclarationService :)

#unbydesigned :) #Resolved

CyrusNajmabadi · 2018-08-24T19:16:38Z

+                declarators.Add(parentDeclaration);
+                declarators.RemoveAll(childDeclarators);
+            }
+        }


TODO(cyrusn). Have not reviewed this yet. waiting to get explanation/comments as to what this all for.

CyrusNajmabadi · 2018-08-24T19:17:25Z

+                UnnecessaryWithSuggestionDescriptor.IsEnabledByDefault,
+                UnnecessaryWithSuggestionDescriptor.Description,
+                UnnecessaryWithSuggestionDescriptor.HelpLinkUri,
+                UnnecessaryWithSuggestionDescriptor.CustomTags.ToArray());


does DiagnosticDescriptor not have a With'er? #Resolved

Nope.

In reply to: 212729411 [](ancestors = 212729411)

Note: instead of manually constructing the descriptor, you can use the helpers defined in the base class. That would help remove a lot of redundancy here. #unresolved #Resolved

I am going to combine this into a single message/option/ID for now.

In reply to: 212774354 [](ancestors = 212774354)

CyrusNajmabadi · 2018-08-24T19:17:47Z

+            _noReadsUnnecessaryWithSuggestionDescriptor = new DiagnosticDescriptor(
+                UnnecessaryWithSuggestionDescriptor.Id,
+                UnnecessaryWithSuggestionDescriptor.Title,
+                new LocalizableResourceString(nameof(FeaturesResources.Avoid_Members_Without_Reads_Message), FeaturesResources.ResourceManager, typeof(FeaturesResources)),


this feels super weird... same diagnostic ID... but different message.... #Resolved

This is a very common pattern in FxCop analyzers where we have large number of minor variations for the same underlying issue, here "an unused member". The pattern we chose there was to avoid assigning a new ID for each such case, and instead just have different descriptors with differing messages. This also improves the FixAll experience by avoiding extra iterations per ID. I am not sure what is the pattern for such scenarios in the IDE analyzers, if we have encountered one till now. I can merge both the cases into a single message, but I felt it is helpful to provide the distinction to the end user, especially as we already know it in the analyzer.

I will wait for further feedback from other reviewers on this.

In reply to: 212729488 [](ancestors = 212729488)

Well, is there a problem with just havin a suitable single message? The specific code-fix can have a particular message to show the user if necessary.

After all, you only have one option controlling this. And you have fix-all fix all of these up. So distinguishing them in any way at all seems superfluous to me. #Resolved

I am not sure what is the pattern for such scenarios in the IDE analyzers

IDE analyzer pattern is that these are either actually different things that should be displayed to the users, or they're not. If htey are, then they should be controlled through separate mechanisms (like options) and should not participate in fix-all together (since we have presented them as being different). If they aren't considered different things (i.e. you can only turn them all on/off at once, and you fix them all together), then they have the same message. #Resolved

I have taken this route - we can split into multiple messages/options/IDs if needed in future.

In reply to: 212774669 [](ancestors = 212774669)

Sounds good. Note, if you want, you can always have different messages for the fix title. That part is very flexible and can be different if you think it would provide clarity and whatnot. For example, you could have 'Remove unused member' as well as 'Remove unwritten member'. #Resolved

CyrusNajmabadi · 2018-08-24T19:18:09Z

+
+        public override bool OpenFileOnly(Workspace workspace) => false;
+
+        public override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;


comment why this is appropriate. #Resolved

CyrusNajmabadi · 2018-08-24T19:18:32Z

+
+        protected override void InitializeWorker(AnalysisContext context)
+        {
+            context.EnableConcurrentExecution();


this should be unnecessary. the base type already sets this. #Resolved

CyrusNajmabadi · 2018-08-24T19:20:21Z

+            // We want to analyze references in generated code, but not report unused members in generated code.
+            context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze);
+
+            context.RegisterCompilationStartAction(compilationStartContext =>


please follow IDE conventions. We do not do the 'ultra nested function' approach to analyzers. We sometimes hav ea small amount of code here (for example to get compilation information, or to create some shared state). but then we call explicit functions that are passed this information, rather than having nested functions that capture. it makes data flow explicit and helps with function readability. I know you like this style, but it stands out highly against all the other features and I would prefer we not unnecesarily do that. #Resolved

CyrusNajmabadi · 2018-08-24T19:20:51Z

+
+            context.RegisterCompilationStartAction(compilationStartContext =>
+            {
+                var entryPoint = compilationStartContext.Compilation.GetEntryPoint(compilationStartContext.CancellationToken);


this is fairly expensive i believe. Is there any concern aobut the costs here and how this could affect time for analysis results to appear? #Resolved

See below in the review for suggestion on how to avoid this expensive cost. #Resolved

CyrusNajmabadi · 2018-08-24T19:21:58Z

+
+                return;
+
+                // Local functions.


please make into sibling functions. That's our pattern for at least 99% of our Analyze methods in our anlayzers. Note that he analyzer itself already acts as hte proper container for these guys. We don't need another level of encapsulation #Resolved

CyrusNajmabadi · 2018-08-24T19:22:35Z

+                    if (IsCandidateSymbol(symbolContext.Symbol))
+                    {
+                        // Initialize unused state to 'true'.
+                        symbolValueUsageStateMap.GetOrAdd(symbolContext.Symbol, valueFactory: s => ValueUsageInfo.None);


we should only hit this once per symbol. Can we have some sort of assert that this never 'get's an existing value? #Resolved

We might receive a symbol reference (operation) callback before a symbol declaration callback, so even though we cannot receive duplicate callbacks for a symbol, an entry might already be present of the declared symbol here.

In reply to: 212730552 [](ancestors = 212730552)

That definitely needs a comment :) #Resolved

CyrusNajmabadi · 2018-08-24T19:23:34Z

+                    symbolValueUsageStateMap.AddOrUpdate(
+                        memberSymbol,
+                        addValue: usageInfo,
+                        updateValueFactory: (sym, currentUsageInfo) => currentUsageInfo | usageInfo);


it seems unfortunate that this causes an allocation on every symbol usage. Is there any overload of symbolValueUsageStateMap that would prevent this.

If not, i would consider actually not using a concurrent dictionary and just use a plain lock as allocation costs here could be significant. #Resolved

CyrusNajmabadi · 2018-08-24T19:25:08Z

+                        // So, we treat this usage as a write-only usage.
+                        if (memberReference.Language == LanguageNames.VisualBasic &&
+                            memberReference.Parent is ICompoundAssignmentOperation compoundAssignment &&
+                            compoundAssignment.Target == memberReference)


This is an innapropriate way to do things. We do not want our code layer to have language specific knowledge between VB and C#. If it needs to we can encode that in a few ways, including:

subclassing. Make this an abstract class and allow VB to customize control flow.

using an appropriate service if this is a general question that would be vlauable to expose for several features. have the VB impl of the service do whatever it needs to do that is distinct. #Resolved

CyrusNajmabadi · 2018-08-24T19:25:52Z

+                    if (IsCandidateSymbol(invocation.TargetMethod))
+                    {
+                        OnSymbolUsage(invocation.TargetMethod, ValueUsageInfo.Read);
+                    }


probably worth explaining why we need this hwen we have AnalyzeMemberReferenceOperation #Resolved

Added a note about requiring to do this due to #26206

In reply to: 212731300 [](ancestors = 212731300)

mavasani · 2018-08-28T22:20:04Z

@CyrusNajmabadi Addressed feedback, thanks.

CyrusNajmabadi · 2018-08-28T22:38:22Z

Thanks. Looking good. Skimmed over the last commit and liked it a lot. I'll do a thorough pass tonight/tomorrow!

mavasani · 2018-08-30T00:36:18Z

@CyrusNajmabadi @jcouv @sharwell any further feedback? Thanks.

CyrusNajmabadi · 2018-08-30T01:37:25Z

Looking now!

CyrusNajmabadi · 2018-08-30T01:43:32Z

+            {   
+                // Get symbol to be removed.
+                var diagnosticNode = diagnostic.Location.FindNode(getInnermostNodeForTie: true, cancellationToken);
+                var symbol = semanticModel.GetDeclaredSymbol(diagnosticNode, cancellationToken);


note: that this works is super fragile. You are using the Location of the diagnostic (which is often a token of the member). You're then calling FindNode, which fortunately is always finding the node that works for GetDeclaredSymbol.

What would be safer is to use .AdditionalLocations to store the actual Symbol Location[0].

Diagnostic is reported with the primary location as Symbol.Locations[0].

In reply to: 213882742 [](ancestors = 213882742)

CyrusNajmabadi · 2018-08-30T01:44:27Z

+        // For example, if we had an unused field with no references, then editing any single method body
+        // to reference this field should clear the unused field diagnostic.
+        // Hence, we need to re-analyze the declarations in the whole file for any edits within the document. 
+        public override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;


great comment. thanks!

CyrusNajmabadi · 2018-08-30T01:46:10Z

+                    return true;
+                }
+
+                if (methodSymbol.IsAsync &&


see previous comment about Main not needing to be 'async' to be Task-returning. #Resolved

CyrusNajmabadi · 2018-08-30T01:46:59Z

+
+                            var diagnostic = DiagnosticHelper.Create(
+                                rule,
+                                member.Locations[0],


won't this squiggle the entire member? or is .Locations just hte name? if so, this is fine, but then 'additionalLocations' is not. #Resolved

It squiggles/fades just the symbol name. Additional locations should almost always be null here, except for partial methods, so I am just going to ignore it for now and pass null.

In reply to: 213883172 [](ancestors = 213883172)

CyrusNajmabadi

Some small concerns. Almost there!

mavasani · 2018-08-30T02:26:16Z

+
+class MyClass
+{
+    private static Task [|Main|]() => return Task.CompletedTask;


return [](start = 38, length = 6)

Ah, the unit test had compile error that cause it not to be flagged, and I forgot to remove the IsAsync check. Fixed now. #Resolved

Verified test fails after fixing the compile error, and passes after removing the IsAsync check.

In reply to: 213887741 [](ancestors = 213887741)

CyrusNajmabadi

Awesome feature. Very excited for this.

mavasani · 2018-08-30T15:57:55Z

retest ubuntu_16_debug_prtest please

mavasani · 2018-08-30T20:32:06Z

@dotnet/roslyn-ide Can I get one more review?

ivanbasov · 2018-08-31T02:49:42Z

        HasAnyExecutableCodeActions = CodeBlockActionsCount > 0 ||

Is it used anywhere else? If used, should not we use another flag?

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerActionCounts.cs:71 in 2055d1c. [](commit_id = 2055d1c, deletion_comment = False)

ivanbasov · 2018-08-31T02:50:07Z

+
+        private static ImmutableArray<DiagnosticDescriptor> CreateDescriptors(bool forceEnableRules)
+        {
+            // TODO: Enable these rules by default once we have designed the Tools|Option location and UI for such code quality rules.


TODO [](start = 15, length = 4)

issue # #Resolved

#29519 - I am planning to work on it next week.

In reply to: 214232978 [](ancestors = 214232978)

ivanbasov · 2018-08-31T02:53:02Z

+                    removeUnreadMembersRule, removeUnreadMembersRuleUnnecessaryWithFadingRule);
+        }
+
+        private DiagnosticDescriptor RemoveUnusedMemberRule => SupportedDiagnostics[1];


s[1] [](start = 82, length = 4)

Where are those indices defined? Maybe a comment will be helpful. #Resolved

Added a comment

In reply to: 214233251 [](ancestors = 214233251)

ivanbasov · 2018-08-31T02:57:32Z

+
+                            var diagnostic = DiagnosticHelper.Create(
+                                rule,
+                                member.Locations[0],


Locations [](start = 39, length = 9)

Please add a comment regarding partial methods. #Resolved

ivanbasov

mavasani · 2018-08-31T13:34:33Z

        HasAnyExecutableCodeActions = CodeBlockActionsCount > 0 ||

This flag is used to determine if we need to run any syntax/operation actions for an analyzer, and for symbol start analyzers we cannot determine this at compilation level as analyzer might register these actions per symbol. The change here conservatively assumes that symbol start analyzers can have syntax/operation actions.

In reply to: 417534528 [](ancestors = 417534528)

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerActionCounts.cs:71 in 2055d1c. [](commit_id = 2055d1c, deletion_comment = False)

mavasani · 2018-08-31T15:16:44Z

@dotnet-bot retest windows_debug_unit32_prtest please

sharwell · 2018-08-30T20:42:41Z

        <target state="translated">Nie można zastosować zmian — nieoczekiwany błąd: „{0}”</target>
        <note />
      </trans-unit>
-      <trans-unit id="Field_preferences_colon">


❔ Why did these files change? It looks like the only changes are:

Reordering things

Removing a translation that was already available

I added new resources, removed the old ones and then reverted these changes and the tooling re-ordered and removed translation.

sharwell · 2018-08-31T16:07:21Z

+        {
+        }
+
+        // For testing purposes only.


❕ Make sure we have an issue filed to have the test suite always enable rules for testing and remove this constructor.

Neme12 · 2018-09-03T05:40:09Z

+            | x.Prop++        |  ✔️  |  ✔️   |             |             |                 |
+            | Foo(x.Prop)     |  ✔️  |       |             |             |                 |
+            | Foo(x.Prop),    |      |       |     ✔️      |             |                 |
+               where void Foo(in T v)


what if the Prop returns by value and not by ref, in which case there is a copy made? and other cases where there is a copy instead of a reference when in is skipped in the argument? is this handled properly as a Read as opposed to ReadableRef?

mavasani added the Area-IDE label Aug 24, 2018

mavasani requested review from a team, CyrusNajmabadi, jinujoseph and sharwell August 24, 2018 18:38

mavasani requested a review from a team as a code owner August 24, 2018 18:38

mavasani changed the title ~~Add analyzer/fixer to flag and remove unused private members (fields/…~~ [DeadCodeAnalysis] Add analyzer/fixer to flag and remove unused private members (fields/… Aug 24, 2018

mavasani commented Aug 24, 2018

View reviewed changes

CyrusNajmabadi reviewed Aug 24, 2018

View reviewed changes

CyrusNajmabadi reviewed Aug 30, 2018

View reviewed changes

CyrusNajmabadi suggested changes Aug 30, 2018

View reviewed changes

mavasani commented Aug 30, 2018

View reviewed changes

Address some more feedback

591bce6

CyrusNajmabadi approved these changes Aug 30, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into DeadCodeAnalysis

2055d1c

ivanbasov reviewed Aug 31, 2018

View reviewed changes

ivanbasov approved these changes Aug 31, 2018

View reviewed changes

mavasani added 2 commits August 31, 2018 06:36

Review feedback

0bc4f7f

Merge remote-tracking branch 'upstream/master' into DeadCodeAnalysis

045c049

sharwell approved these changes Aug 31, 2018

View reviewed changes

mavasani mentioned this pull request Aug 31, 2018

IDE analyzer/fixer test framework should always force enable analyzer rules by default #29615

Closed

mavasani merged commit 27f2414 into dotnet:master Aug 31, 2018

mavasani deleted the DeadCodeAnalysis branch August 31, 2018 16:38

Neme12 reviewed Sep 3, 2018

View reviewed changes

jinujoseph added this to the 16.0 milestone Dec 13, 2018


		public override bool OpenFileOnly(Workspace workspace) => false;

		public override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;

Conversation

mavasani commented Aug 24, 2018

Uh oh!

mavasani Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 24, 2018 • edited by mavasani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mavasani Aug 24, 2018 •

edited

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

mavasani Aug 24, 2018 •

edited

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 25, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading

CyrusNajmabadi Aug 24, 2018 •

edited by mavasani

Loading