Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove _showPosition from DiagnosticDescription #19353

Merged
merged 1 commit into from
May 10, 2017
Merged

remove _showPosition from DiagnosticDescription #19353

merged 1 commit into from
May 10, 2017

Conversation

TyOverby
Copy link
Contributor

@TyOverby TyOverby commented May 8, 2017 •
edited by jaredpar
Loading

Test only change

Closes #17155

_showPosition was really weird.

  • Nowhere was _showPosition ever set to false from the constructor.
  • The Equals method set showPosition to false on both DiagnosticDescriptions if both have locations and both are the same, but only if the method didn't bail out earlier.

CC @jcouv @AlekseyTs

@TyOverby
Copy link
Contributor Author

TyOverby commented May 8, 2017

cc more people that might be interested @cston @khyperia

jaredpar
jaredpar reviewed May 8, 2017
View reviewed changes
src/Test/Utilities/Portable/Diagnostics/DiagnosticDescription.cs
@@ -219,13 +217,8 @@ public override bool Equals(object obj)
{
if (_startPosition.Value != d._startPosition.Value)
{
_showPosition = true;
d._showPosition = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm ... why should Equals every be changing state??? You've deleted lines that terrify me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RIGHT

Copy link
Contributor Author

@TyOverby TyOverby May 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OmarTawfik can attest: when I found that this was the source of the bug, I cried tears of blood.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a hidden gem as old as our history goes ~~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us forever burn this mistake into our memories. Today will be remembered as ""In No State chANging Equals" day, or "INSANE" for short.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like good RoslynAnalyzer candidate to check if fields are set in .Equals, .GetHashCode, .ToString... methods

Copy link
Member

@mgravell mgravell May 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidKarlas i could forgive it in GetHashCode and ToString as long as it is just caching a lazily computed value to prevent recalc

jaredpar
jaredpar approved these changes May 8, 2017
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't decide if I'm more surprised this exists or that everything passes when we remove it.

jaredpar
jaredpar requested changes May 8, 2017
View reviewed changes
Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ETA failure looks real. Apparently the internal code base depends on this

https://ci.dot.net/job/Private/job/dotnet_roslyn-internal/job/master/job/windows_eta_open_prtest/6088/console

@OmarTawfik OmarTawfik mentioned this pull request May 8, 2017
Merged
@jaredpar
Copy link
Member

jaredpar commented May 8, 2017

The ETA failure is real. But I think we can simply delete the usage of showPosition in the code once this change is in.

https://github.com/dotnet/roslyn-internal/blob/master/Closed/Hosting/RoslynETAHost/Diagnostics/Features/IOperation/IOperationUnitTestGenerator.cs#L347

Need to sync up with @mavasani to make sure. If he agrees we need to coordinate a change between the two repos.

@mavasani
Copy link
Contributor

mavasani commented May 8, 2017

IOperation unit test generator uses this overload when creating new tests, so it shouldn't affect existing tests. You can get rid of the explicit argument at https://github.com/dotnet/roslyn-internal/blob/master/Closed/Hosting/RoslynETAHost/Diagnostics/Features/IOperation/IOperationUnitTestGenerator.cs#L347 before merging your PR to avoid any breaks.

@jaredpar
Copy link
Member

jaredpar commented May 8, 2017

PR to resolve the internal break https://github.com/dotnet/roslyn-internal/pull/1831

@jaredpar
Copy link
Member

jaredpar commented May 9, 2017

retest eta please

@jaredpar
Copy link
Member

jaredpar commented May 9, 2017

retest windows_debug_unit32_prtest please

@jaredpar
Copy link
Member

jaredpar commented May 9, 2017

Justification for retest:

  • ETA: fixed the underlying build break in PR and need to retest against latest
  • Unit 32 debug: hit the known timeout bug

jcouv
jcouv approved these changes May 9, 2017
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I understand this is test code, but I'd consider adding a test (feed a couple actual and expected diagnostics, and run a comparison on them).

@TyOverby
Copy link
Contributor Author

TyOverby commented May 9, 2017

@jcouv: Yeah, a few tests wouldn't be amiss. Where do we typically put test-tests?

@TyOverby TyOverby merged commit 5e053aa into dotnet:master May 10, 2017
@TyOverby
Copy link
Contributor Author

This is killing me. I'm going to merge now and add tests later.

@TyOverby TyOverby mentioned this pull request May 10, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarTawfik OmarTawfik OmarTawfik left review comments

@davkean davkean davkean left review comments

@DavidKarlas DavidKarlas DavidKarlas left review comments

@mgravell mgravell mgravell left review comments

@mcosta91 mcosta91 mcosta91 approved these changes

@jcouv jcouv jcouv approved these changes

@jaredpar jaredpar jaredpar approved these changes

Assignees
No one assigned
Labels
Area-Compilers cla-already-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@TyOverby @jaredpar @mavasani @mgravell @DavidKarlas @davkean @jcouv @OmarTawfik @mcosta91 @dnfclas