"Generate Equals" should ignore indexers and setter-only properties by Neme12 · Pull Request #25700 · dotnet/roslyn

Neme12 · 2018-03-24T15:39:04Z

Neme12 · 2018-03-25T19:12:03Z

@CyrusNajmabadi Would you take a quick look at this? Thanks

CyrusNajmabadi · 2018-03-25T19:28:56Z

...Test/GenerateEqualsAndGetHashCodeFromMembers/GenerateEqualsAndGetHashCodeFromMembersTests.vb

+
+        <WorkItem(25690, "https://github.com/dotnet/roslyn/issues/25690")>
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsGenerateEqualsAndGetHashCode)>
+        Public Async Function TestWithDialogNoIndexer() As Task


note: this is not an indexer for VB, it's a parameterized property. To make an indexer, you need to use "default property". I would consider adding a test for both cases.

ok, will add test

I think i've seen this referred to as an "indexer property" in the docs

Internally, we've always called them "parameterized properties". Indexers use the "Default" keyword.

Neme12 · 2018-03-25T23:03:11Z

@CyrusNajmabadi I made other changes to fix one more issue, you might want to review again.

CyrusNajmabadi · 2018-03-26T00:11:11Z

src/Features/Core/Portable/GenerateFromMembers/AbstractGenerateFromMembersService.cs

-               IsWritableFieldOrProperty(symbol);
+            => !symbol.IsStatic &&
+               (symbol is IFieldSymbol field && IsViableField(field) && !field.IsConst ||
+                symbol is IPropertySymbol property && IsViableProperty(property) && property.IsWritableInConstructor());


this mix of &&'s and ||'s is confusing. Please extract into helper and parenthesize to keep precedence clear. In general, we prefer expressions that are uniform with &&'s or ||'s.

I thought about this for a while and found it hard to extract common code between IsWritableInstanceFieldOrProperty and IsReadableInstanceFieldOrProperty. They use the same logic for whether the field/property is "viable" but they also need to do more checks that are still specific to each type. Basically one method would check the type and some condition, and the other one would check the type again for another condition. So I don't know what to do there.

IsWritableInstanceFieldOrProperty

IsReadableInstanceFieldOrProperty

Based on the name, it sounds like they can reuse whatever computes if this is an InstanceFieldOrProperty

Then you can have code that checks if it's Readable or Writable.

I thought about this for a while and found it hard to extract common code

Right now the mixing of precedence operators is confusing here. Please at least parenthesize for clarity. Also, to prevent mixing and matching of operators, just make this a full body method. THis is complex enough that collapsing into an expression is harder to understand.

Based on the name, it sounds like they can reuse whatever computes if this is an InstanceFieldOrProperty

Yes. But then you'd still have another method for IsWritableInstanceFieldOrProperty which would have the same structure - it needs to check writability based on whether the symbol is a field or a property. So the code reuse would actually reuse some things but duplicate other things, and introduce unnecessary type checks after we already did the same check before.

CyrusNajmabadi

Please refactor the complex expressions to give precedence to clarity.

Neme12 · 2018-03-26T10:18:25Z

to give precedence to clarity.

interesting way to phrase things 😃

CyrusNajmabadi · 2018-03-27T18:36:10Z

src/Features/Core/Portable/GenerateFromMembers/AbstractGenerateFromMembersService.cs

+        {
+            switch (symbol)
+            {
+                case IFieldSymbol field when IsViableField(field): return true;


can you not just do: case IFieldSymbol field: return IsViableField(field);? It seems much simpler than having 'when' and a boolean.

CyrusNajmabadi · 2018-03-27T18:36:28Z

src/Features/Core/Portable/GenerateFromMembers/AbstractGenerateFromMembersService.cs

+            switch (symbol)
+            {
+                case IFieldSymbol field when IsViableField(field): return true;
+                case IPropertySymbol property when IsViableProperty(property): return !property.IsWriteOnly;


same issue here. no need for 'when'. just merge its condition into the actual return statement.

I found this a little nicer than having && inside each case but ok

CyrusNajmabadi · 2018-03-27T18:38:02Z

src/Features/Core/Portable/GenerateFromMembers/AbstractGenerateFromMembersService.cs

-            => !symbol.IsStatic &&
-               (symbol is IFieldSymbol field && IsViableField(field) && !field.IsConst ||
-                symbol is IPropertySymbol property && IsViableProperty(property) && property.IsWritableInConstructor());
+            => !symbol.IsStatic && IsWritableFieldOrProperty(symbol);


these two methods are very readable. thank you :)

CyrusNajmabadi

Thanks for the change. LGTM.

Neme12 · 2018-03-29T23:35:54Z

retest windows_debug_vs-integration_prtest please

Neme12 · 2018-03-29T23:36:04Z

retest windows_release_vs-integration_prtest please

…exers

Neme12 · 2018-04-11T22:13:44Z

retest windows_release_unit32_prtest please

Neme12 · 2018-04-12T00:14:51Z

retest windows_release_unit32_prtest please

DustinCampbell

LGTM

DustinCampbell · 2018-04-12T01:27:20Z

@dotnet-bot retest windows_release_unit64_prtest please

DustinCampbell · 2018-04-12T01:27:43Z

@dotnet-bot retest this please

Neme12 · 2018-04-12T06:23:51Z

retest windows_debug_unit32_prtest please

DustinCampbell · 2018-04-12T14:55:36Z

@dotnet-bot retest this please

Neme12 · 2018-04-12T15:18:58Z

I think some of those tests are stuck. I have this on another PR as well and retesting doesn't help.

DustinCampbell · 2018-04-12T15:21:28Z

Yeah, @jaredpar just sent email internally about it. The Jenkins machines are stuck, but it's being looked at.

Neme12 · 2018-04-13T06:59:55Z

retest windows_debug_spanish_unit32_prtest please

Neme12 · 2018-04-13T07:00:06Z

retest windows_release_vs-integration_prtest please

Neme12 · 2018-04-13T14:52:23Z

retest windows_debug_spanish_unit32_prtest please

Neme12 · 2018-04-13T17:35:17Z

retest windows_debug_spanish_unit32_prtest please

DustinCampbell · 2018-04-13T20:50:51Z

@jaredpar: Is the windows_debug_spanish_unit32_prtest flaky at the moment? We've run it a few times, but we keep getting back, "xunit produced no error output but had exit code 1"

jaredpar · 2018-04-13T20:55:04Z

@DustinCampbell looks like the EE tests are just faky in general. Seen this in a few logs


Microsoft.CodeAnalysis.VisualBasic.ExpressionEvaluator.UnitTests.DebuggerTypeProxyAttributeTests.GenericTypeWithGenericTypeArgument
System.Reflection.TargetInvocationException: Se produjo una excepciÃ³n en el destino de la invocaciÃ³n.
System.AccessViolationException: Intento de leer o escribir en la memoria protegida. A menudo, esto indica que hay otra memoria daÃ±ada.

DustinCampbell · 2018-04-13T21:01:09Z

OK to merge?

jaredpar · 2018-04-13T21:03:20Z

It's fine by me.

DustinCampbell · 2018-04-14T10:40:43Z

Great! @Neme12: Thanks very much for your contributions!

Neme12 · 2018-04-14T11:01:59Z

Thanks, I still have 12 other PRs if you're interested 😄

Neme12 added 2 commits March 24, 2018 15:57

Ignoring properties that have parameters

8618dfe

Adding missing VB test for parity with C#

f92ca99

Neme12 requested a review from a team as a code owner March 24, 2018 15:39

Neme12 mentioned this pull request Mar 24, 2018

"Generate Equals" refactorings produce non-compiling code when using indexers #25690

Closed

CyrusNajmabadi reviewed Mar 25, 2018

View reviewed changes

CyrusNajmabadi approved these changes Mar 25, 2018

View reviewed changes

Neme12 added 3 commits March 25, 2018 23:08

Adding test with true indexer for VB

1e4c979

Adding same tests to GenerateConstructorFromMembers

4435824

Making GenerateEquals ignore setter only properties

91d0c08

Neme12 changed the title ~~"Generate Equals" should not treat indexers as properties~~ "Generate Equals" should ignore indexers and setter-only properties Mar 25, 2018

CyrusNajmabadi reviewed Mar 26, 2018

View reviewed changes

CyrusNajmabadi suggested changes Mar 26, 2018

View reviewed changes

Giving precedence to clarity

1bab56b

CyrusNajmabadi reviewed Mar 27, 2018

View reviewed changes

Using '&&' over 'when'

5f32a2d

CyrusNajmabadi approved these changes Mar 28, 2018

View reviewed changes

jcouv added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 30, 2018

Merge remote-tracking branch 'upstream/master' into generateEqualsInd…

0b3f064

…exers

DustinCampbell approved these changes Apr 12, 2018

View reviewed changes

DustinCampbell merged commit cce6b0e into dotnet:master Apr 14, 2018

Neme12 deleted the generateEqualsIndexers branch April 14, 2018 11:02

Conversation

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

Uh oh!

Uh oh!

Neme12 commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 26, 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!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 26, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 27, 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!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Neme12 commented Mar 29, 2018

Uh oh!

Neme12 commented Mar 29, 2018

Uh oh!

Neme12 commented Apr 11, 2018

Uh oh!

Neme12 commented Apr 12, 2018

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

DustinCampbell commented Apr 12, 2018

Uh oh!

DustinCampbell commented Apr 12, 2018

Uh oh!

Neme12 commented Apr 12, 2018

Uh oh!

DustinCampbell commented Apr 12, 2018

Uh oh!

Neme12 commented Apr 12, 2018

Uh oh!

DustinCampbell commented Apr 12, 2018

Uh oh!

Neme12 commented Apr 13, 2018

Uh oh!

Neme12 commented Apr 13, 2018

Uh oh!

Neme12 commented Apr 13, 2018

Neme12 commented Mar 24, 2018 •

edited

Loading

Neme12 commented Mar 25, 2018 •

edited

Loading

Neme12 Mar 25, 2018 •

edited

Loading

Neme12 commented Mar 25, 2018 •

edited

Loading

CyrusNajmabadi Mar 26, 2018 •

edited

Loading

Neme12 Mar 26, 2018 •

edited

Loading

Neme12 Mar 27, 2018 •

edited

Loading

jaredpar commented Apr 13, 2018 •

edited

Loading