Generate readonlyattribute if it does not exist by OmarTawfik · Pull Request #18715 · dotnet/roslyn

OmarTawfik · 2017-04-14T20:06:47Z

Things done in this PR:

Refactor GetAttributes() and GetReturnTypeAttributes() of various helpers through out the code base to accept a parameter of type PEModuleBuilder instead of CompilationState.
Symbols now store the fact that they need an IsReadOnly attribute on the compilation object.
The PEAssemblyBuilder uses that fact and either generates that type during emitting, or gives an error if the user also defines that type in the same compilation.
Added a well known type Microsoft.CodeAnalysis.EmbeddedAttribute that both C# and VB compilers recognize and ignore types annotated by it during look up. All types embedded will have both this attribute and CompilerGeneratedAttribute.
Bug Defective tests: testing source attributes vs emitted attributes #18799 was filed because tests were testing synthesized attributes through internal APIs instead of testing compiled modules. Tests have a workaround (and a TODO) for that until the bug is fixed (optimally in master).

cc @dotnet/roslyn-compiler @VSadov @AlekseyTs

…eeded

cston · 2017-04-26T15:09:18Z

src/Compilers/Core/Portable/PEWriter/Members.cs

        /// Custom attributes associated with the method's return value.
        /// </summary>
-        IEnumerable<ICustomAttribute> ReturnValueAttributes { get; }
+        IEnumerable<ICustomAttribute> GetReturnValueAttributes(CommonPEModuleBuilder moduleBuilder);


Consider passing an EmitContext argument instead, for consistency with other Cci interface methods. #Resolved

cston · 2017-04-26T18:10:07Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

+        internal override ImmutableArray<NamedTypeSymbol> GetAdditionalTopLevelTypes(DiagnosticBag diagnostics)
        {
-            return _additionalTypes;
+            Interlocked.CompareExchange(ref _embeddedAttributesConstructors, CreateEmbeddedAttributesIfNeeded(diagnostics), null);


Interlocked.CompareExchange [](start = 12, length = 27)

GetAdditionalTopLevelTypes should be called once (or at most once per emit) so Interlocked.CompareExchange shouldn't be necessary. #Resolved

Yes. the identity of the dictionary is unimportant. Even if we assign optimistically on multiple threads a dictionary with same contents. interlocked would not be needed. Simple assignment is sufficient.

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

cston · 2017-04-26T18:17:29Z

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

+
+        internal virtual SynthesizedAttributeData SynthesizeEmbeddedAttribute()
+        {
+            // For modules, this attribute should be present. Only assemblies generate and embedd this type.


Typo here and method below: embed #Resolved

cston · 2017-04-26T18:21:15Z

src/Compilers/CSharp/Portable/Symbols/SourceEmbeddedAttributeSymbol.cs

+        private NamedTypeSymbol _baseType;
+        private MissingNamespaceSymbol _namespace;
+        private ImmutableArray<Symbol> _members;
+        private ModuleSymbol _module;


readonly for all fields. #Resolved

cston · 2017-04-26T18:23:24Z

src/Compilers/CSharp/Portable/Symbols/SourceEmbeddedAttributeSymbol.cs

+            _module = compilation.SourceModule;
+
+            _namespace = new MissingNamespaceSymbol(compilation.GlobalNamespace, nameParts[0]);
+            for (int i = 1; i + 1 < nameParts.Length; i++)


_namespace = compilation.GlobalNamespace; for (int i = 0; ... ``` #Resolved

AlekseyTs · 2017-04-26T18:24:54Z

src/Compilers/Core/Portable/WellKnownTypes.cs


-        System_Runtime_CompilerServices_ReadOnlyAttribute,
+        System_Runtime_CompilerServices_IsReadOnlyAttribute,
+        Microsoft_CodeAnalysis_EmbeddedAttribute,


Microsoft_CodeAnalysis_EmbeddedAttribute [](start = 8, length = 40)

I am just started looking at this change, but the fact that we added this type to the well-known types list feels very surprising to me. Well-known types are not used to identify attribute applications, and, when we are embedding types we are using the types that we embed, which aren't well-known (they are hidden from everyone, but the compiler). #Closed

cston · 2017-04-26T18:25:03Z

src/Compilers/CSharp/Portable/Symbols/SourceEmbeddedAttributeSymbol.cs

+        public SourceEmbeddedAttributeSymbol(WellKnownType wellKnownType, CSharpCompilation compilation)
+        {
+            var nameParts = WellKnownTypes.GetMetadataName(wellKnownType).Split('.');
+            Debug.Assert(nameParts.Length > 1, "Attribute cannot be in global namespace?");


Is the assert necessary? #ByDesign

I'd say yes, since the code below operates on that fact.

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

AlekseyTs · 2017-04-26T18:30:32Z

src/Compilers/Core/Portable/PEWriter/Members.cs

        /// Custom attributes associated with the method's return value.
        /// </summary>
-        IEnumerable<ICustomAttribute> ReturnValueAttributes { get; }
+        IEnumerable<ICustomAttribute> GetReturnValueAttributes(CommonPEModuleBuilder moduleBuilder);


IEnumerable GetReturnValueAttributes(CommonPEModuleBuilder moduleBuilder); [](start = 8, length = 92)

Consider passing an EmitContext argument instead, for consistency with other Cci interface methods.

I agree with this suggestion, contracts in interfaces should use EmitContext. #Closed

AlekseyTs · 2017-04-26T18:48:39Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+    <value>Do not use 'System.Runtime.CompilerServices.IsReadOnlyAttribute'. This is reserved for compiler usage.</value>
+  </data>
+  <data name="ERR_TypeReserved" xml:space="preserve">
+    <value>The type '{0}' is reserved to be used by the compiler.</value>


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

"type name" vs. "type" ? #Closed

AlekseyTs · 2017-04-26T18:51:17Z

src/Compilers/CSharp/Portable/Binder/Binder_Lookup.cs


-            if (WrongArity(symbol, arity, diagnose, options, out diagInfo))
+            // Check for external symbols marked with 'Microsoft.CodeAnalysis.Embedded' attribute
+            if (!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) && unwrappedSymbol.IsHiddenByCodeAnalysisEmbeddedAttribute())


!this.Compilation.SourceModule.Equals(unwrappedSymbol.ContainingModule) [](start = 16, length = 71)

Do we have tests covering significance of this condition? #Closed

Curious - why "external" part makes a difference? I thought the embedded symbols are not bindable regardless of where defined.

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

We have tests that cover one condition. Adding more tests now.
edit: @VSadov I'm correcting this now. Lookup would fail if on embedded attributes defined somewhere else. If you define your own embedded attribute we would ignore it. UNLESS the compiler needs to generate one, then we will produce an error.

In reply to: 113549567 [](ancestors = 113549567,113533730)

Could you point me to a test that demonstrates significance of the first condition?

In reply to: 114024707 [](ancestors = 114024707,113549567,113533730)

Please check AttributeTests_Embedded.cs:
ReferencingEmbeddedAttributesFromADifferentAssemblyFails_Internal
ReferencingEmbeddedAttributesFromADifferentAssemblyFails_Public
and
EmbeddedAttributeInSourceShouldTriggerAnErrorIfCompilerNeedsToGenerateOne

In reply to: 115050349 [](ancestors = 115050349,114024707,113549567,113533730)

AlekseyTs · 2017-04-26T18:57:53Z

src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs

        /// </summary>
        private Symbol[] _lazyWellKnownTypeMembers;

+        internal bool NeedsGeneratedIsReadOnlyAttribute { get; private set; }


internal bool NeedsGeneratedIsReadOnlyAttribute { get; private set; } [](start = 8, length = 69)

It would be good to have a comment when one should expect this property to have actual value. We should probably have a way to "freeze" the value and a way to assert an attempt to set the value it after it was frozen and an attempt to read the value before it is frozen. #Closed

AlekseyTs · 2017-04-26T19:05:52Z

src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs

+                member: WellKnownMember.System_Runtime_CompilerServices_IsReadOnlyAttribute__ctor,
+                diagnostics: DeclarationDiagnostics,
+                location: readOnlySymbol.Locations.FirstOrDefault() ?? Location.None,
+                isOptional: Options.OutputKind != OutputKind.NetModule);


isOptional: Options.OutputKind != OutputKind.NetModule [](start = 16, length = 54)

It feels like this doesn't give us the behavior we want. I think we want to embed the attribute only if the type cannot be found, the lack of the required constructor should result in an error rather than in an embedded attribute. #Closed

If the attribute defined in current module and does not have right constructor, embedding another attribute would cause a conflict. Is that the reason for giving an error?

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

Good point. I think the lookup should be modified to give a different error if the type was found but it was lacking the correct signature

In reply to: 113554955 [](ancestors = 113554955,113537098)

Good point. I think the lookup should be modified to give a different error if the type was found but it was lacking the correct signature

I think the existing error for a missing well-known member will work just fine.

In reply to: 114047541 [](ancestors = 114047541,113554955,113537098)

AlekseyTs · 2017-04-26T19:11:36Z

src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs

+            var attributeSymbol = Binder.GetWellKnownTypeMember(
+                compilation: this,
+                member: WellKnownMember.System_Runtime_CompilerServices_IsReadOnlyAttribute__ctor,
+                diagnostics: DeclarationDiagnostics,


DeclarationDiagnostics [](start = 29, length = 22)

Rather than adding diagnostics to DeclarationDiagnostics directly, we probably should use Symbol.AddDeclarationDiagnostics API instead as some symbols customize where declaration diagnostics should go. #Closed

Moving to a model where diagnosticsbag is defined by the syntax needing it whenever possible.

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

AlekseyTs · 2017-04-26T19:18:57Z

src/Compilers/CSharp/Portable/Symbols/Compilation_WellKnownMembers.cs

        }

-        internal SynthesizedAttributeData SynthesizeReadOnlyAttribute()
+        internal void EnsureIsReadOnlyAttributeExists(Symbol readOnlySymbol)


EnsureIsReadOnlyAttributeExists [](start = 22, length = 31)

Please ensure we have tests for local functions. #Closed

I've a test for the positive case. Will add a test for the negative case as well.

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

cston · 2017-04-26T19:31:36Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+        /// <summary>
+        /// Lazily initiated HasEmbeddedAttribute
+        /// </summary>
+        private ThreeState _lazyHasEmbeddedAttribute = ThreeState.Unknown;


Should this be part of UncommonProperties rather than a field directly on PENamedTypeSymbol? #Closed

yes, this is very uncommon case.

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

Comment recalled.

In reply to: 113556704 [](ancestors = 113556704,113542264)

UncommonProperties are only instantiated for PENamedTypeSymbols with uncommon attributes. The instances of UncommonProperties are not shared. #Resolved

Noted. Will fix it now.

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

cston · 2017-05-11T20:26:43Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

Is there still a case where symbol can be null? If not, remove the null check and perhaps return symbol rather than using a ref parameter.

This is possibly going to be rewritten in Vlad's next PR, as there are more attributes to be generated. His feature will possibly need to generate some attributes that are generated in this PR, so the check will be necessary.
I see the future of embedded attributes going in one of two directions:

keeping the current design, having each feature calling CreateEmbeddedAttributeIfNeeded on the attributes it needs (possibly overlapping), and thus the current design protects against that.

If the features that need embedded attributes grow in numbers, there might be a need for an independent type manager to hold/manage them.

For now we are adding only IsByRefLike, so there is no need for #2 just yet.
The current code seems ok to me for now.

cston · 2017-05-11T20:52:52Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedEmbeddedAttributeSymbol.cs

Minor point: factory.Block() has overload with params array so these could be inlined below. #Resolved

cston · 2017-05-11T21:01:27Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Embedded.cs

Consider reporting the fully qualified type name so it's clear we're not reserving the type name regardless of namespace. #Resolved

Good point.

cston · 2017-05-11T21:11:16Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Embedded.cs

Duplicate error?

No. because the compiler is trying to synthesize two attributes here (Embedded and IsReadOnly).

cston · 2017-05-11T21:11:43Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Embedded.cs

Duplicate error?

No. because the compiler is trying to synthesize two attributes here (Embedded and IsReadOnly).

cston · 2017-05-11T21:41:04Z

LGTM

OmarTawfik · 2017-05-11T23:01:39Z

@cston @AlekseyTs addressed the latest set of comments.
I think this PR is getting too big for codeflow (it started misbehaving and I'm not able to see older comments, trying to contact them). Please let me know if I missed something.
Also, I'd like to merge this PR (if you sign off on the functional changes here) sooner than later to unblock @VSadov. If there are any missing tests left I can address them in the next PR.

AlekseyTs

Please wait for sign-off from me.

AlekseyTs · 2017-05-12T17:53:45Z

src/Compilers/CSharp/Portable/Emitter/Model/PEAssemblyBuilder.cs

+            builder.AddRange(_additionalTypes);
+
+            CreateEmbeddedAttributesIfNeeded();
+            diagnostics.AddRange(_lazyEmbeddedAttributesDiagnostics);


diagnostics.AddRange(_lazyEmbeddedAttributesDiagnostics); [](start = 12, length = 57)

This is likely to cause duplication of errors. We should avoid that, we specifically avoid reporting duplicate errors in PEModuleBuilder, see _reportedErrorTypesMap for example. Please revert to the previous approach.

AlekseyTs · 2017-05-12T18:16:53Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_RefReadOnly.cs

+{
+    private int value;
+
+    public override ref readonly int Method(ref readonly int x) => ref value;


public override ref readonly int Method(ref readonly int x) => ref value; [](start = 3, length = 74)

The override scenarios should be tested inside IsReadOnlyAttribute type. This test could be valuable too, but it doesn't test most interesting scenario.

Similar for explicit interface implementations. In these tests, test both scenarios when base type/interface is defined in the same assembly and when it is defined in a different assembly.

AlekseyTs · 2017-05-12T18:19:58Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_RefReadOnly.cs

+        }
+
+        [Fact]
+        public void RefReadOnlyDefinitionsInsideUserDefinedIsReadOnlyAttribute_Class_CorrectParent()


RefReadOnlyDefinitionsInsideUserDefinedIsReadOnlyAttribute_Class_CorrectParent [](start = 20, length = 78)

Added tests for indexers, operators, properties, and overrides. There is already a test that verifies we produce errors if IsReadOnly was an interface, because it is lacking the constructor.

I don't see any tests added for explicit interface implementations. I believe I asked for them in the previous review pass.

AlekseyTs · 2017-05-12T19:01:40Z

Done with review pass. I believe the remaining issues that I track are summarized in the last three comments above. At the very least the requested tests should be added before the PR can be merged, I think.

OmarTawfik · 2017-05-13T02:42:15Z

@AlekseyTs added the tests requested in your last comment in 40748ee

About your other comment:

This is likely to cause duplication of errors. We should avoid that, we specifically avoid reporting duplicate errors in PEModuleBuilder, see _reportedErrorTypesMap for example. Please revert to the previous approach.

This contradicts with the point @cston pointed out. Basically, if I reverted, first call to GetAdditionalTopLevelTypes(diagnostics) will return diagnostics that the subsequent calls will not.
The current proposed implementation de-dupes errors between different attributes, so that if two features asked to embed the same attribute, we won't duplicate errors then. But the errors are saved so that the API is consistent between different external calls to get additional types.
Do you want to revert that behavior? am I missing something?

AlekseyTs · 2017-05-13T09:32:47Z

Do you want to revert that behavior? am I missing something?

Yes, I believe we should revert the behavior. There is no requirement for this API to produce the same errors on every call. In fact, I believe it is preferred that this API wouldn't produce errors due to the same reasons on the second and subsequent calls.

AlekseyTs · 2017-05-13T10:11:35Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs

                _explicitInterfaceType.CheckAllConstraints(conversions, new SourceLocation(explicitInterfaceSpecifier.Name), diagnostics);
            }
+
+            if (_refKind == RefKind.RefReadOnly)


if (_refKind == RefKind.RefReadOnly) [](start = 12, length = 36)

It feels like we should be moving similar checks for all source symbols into an override of AfterAddingTypeMembersChecks API. This should ensure that the checks won't cause circularity issues in the future due to some changes in the compiler's behavior.

I don't think the general rule applies, because for some symbols like SourceConstructorMethod we need the parameters to be computed first before we perform that check, and calling Parameters in AfterAddingTypeMembersChecks would call MethodChecks in turn.
For SourcePropertySymbol, it wasn't necessary since it computes parameters without calling MethodChecks. Some symbols I believe don't even use this API like LocalFunctionSymbol.

AlekseyTs · 2017-05-13T10:15:20Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs

+
+            if (_refKind == RefKind.RefReadOnly)
+            {
+                DeclaringCompilation.EnsureIsReadOnlyAttributeExists(diagnostics, _location, modifyCompilationForRefReadOnly: true);


_location [](start = 82, length = 9)

I didn't notice any location changes for errors produced by this call. Is location not changed in iteration 18, or do we have a test gap?

We have tests covering these diagnostics, like IsReadOnlyAttributeExistsWithWrongConstructorSignature_Assembly. However, Covering every permutation for every symbol type and error type and location (parameter vs return type) I believe should be left to the final test plan (lot's of moving pieces right now in this feature).
However, I've added 5 more tests for symbols for that specific error location for this PR.

OmarTawfik · 2017-05-14T01:44:57Z

@AlekseyTs I reverted the change and added a few more tests. Also answered the last comment about AfterAddingTypeMembersChecks.

OmarTawfik · 2017-05-16T00:52:02Z

@AlekseyTs moved calls of EnsureIsReadOnlyAttributeExists for declaration-level symbols to AfterAddingTypeMembersChecks as we discussed. Also added a couple more tests for reporting diagnostics for non-declaration symbols (lambdas and local functions) to cover the discussed gap.

dismissed

AlekseyTs

LGTM, modulo the failing tests and merge conflicts.

Call this.DeclaringCompilation.EnsureReadOnlyAttributeExists() when n…

d01bccc

…eeded

OmarTawfik added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 14, 2017

dnfclas added the cla-already-signed label Apr 14, 2017

Omar Tawfik added 9 commits April 14, 2017 13:33

Promote EmbeddedAttribute to be used by both compilers

ea8e8da

Generate Embedded attributes on PE Module Builder

389871a

Thread PEModuleBuilder through Symbol.AddSynthesizedAttributes()

45abc25

Fix failing tests

f14b9b0

Base case passing

72459ef

Refactor ReadOnlyAttribute to IsReadOnlyAttribute

13e65c1

Fix build break

ddf80c5

More Tests

801d046

Clean up

ab6d26c

OmarTawfik added Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Apr 26, 2017

OmarTawfik requested a review from VSadov April 26, 2017 07:23

cston reviewed Apr 26, 2017

View reviewed changes

AlekseyTs reviewed Apr 26, 2017

View reviewed changes

cston reviewed Apr 26, 2017

View reviewed changes

AlekseyTs reviewed Apr 26, 2017

View reviewed changes

cston reviewed Apr 26, 2017

View reviewed changes

cston reviewed May 11, 2017

View reviewed changes

More PR Comments

7be6b63

AlekseyTs previously requested changes May 12, 2017

View reviewed changes

AlekseyTs reviewed May 12, 2017

View reviewed changes

More tests for explicit interface implementations

40748ee

AlekseyTs reviewed May 13, 2017

View reviewed changes

Omar Tawfik added 2 commits May 13, 2017 16:02

Latest PR Comments

f4ce74d

Merge conflicts resolved

83f6355

Moved EnsureIsReadOnlyAttributeExists to AfterAddingTypeMembersChecks

f6dc461

AlekseyTs approved these changes May 16, 2017

View reviewed changes

Omar Tawfik added 2 commits May 16, 2017 13:25

Resolve conflicts

0bc8579

Fix failing tests

7042fc6

OmarTawfik merged commit d63f038 into dotnet:features/readonly-ref May 18, 2017

OmarTawfik deleted the generate-readonlyattribute-if-it-does-not-exist branch May 18, 2017 18:56

jnm2 mentioned this pull request Oct 17, 2024

Attributes recognized by the compiler in binaries dotnet/docs#43110

Closed

Conversation

OmarTawfik commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 26, 2017 • 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!

cston Apr 26, 2017 • edited by OmarTawfik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 26, 2017 • 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!

AlekseyTs Apr 26, 2017 • 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!

AlekseyTs Apr 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

OmarTawfik Apr 28, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 26, 2017 • 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!

AlekseyTs Apr 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

OmarTawfik commented Apr 14, 2017 •

edited

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

OmarTawfik Apr 28, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

AlekseyTs Apr 26, 2017 •

edited

Loading

cston Apr 26, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Apr 30, 2017 •

edited

Loading

cston Apr 30, 2017 •

edited by OmarTawfik

Loading

cston May 11, 2017 •

edited by OmarTawfik

Loading

cston May 11, 2017 •

edited by OmarTawfik

Loading