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.

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

Bug fix for issue #8111 - Don't mark cast as redundant if removing it will change the shape of a new anonymous object #8195

Merged
merged 5 commits into from Apr 20, 2016

Conversation

steveniles
Copy link
Contributor

@steveniles steveniles commented Jan 27, 2016

Fixes #8111

This fixes a corner case in which the IDE0004 diagnostic ("C# Cast is redundant") claims a cast is safe to remove, when removing it could in fact change program behavior, by changing the type of an anonymous object's member

… will change the shape of a new anonymous object

This fixes a corner case in which the IDE0004 diagnostic ("C# Cast is
redundant") claims a cast is safe to remove, when removing it could in
fact change program behavior, by changing the type of an anonymous
object's member
@davkean davkean added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 28, 2016
@davkean
Copy link
Member

davkean commented Jan 28, 2016

@dotnet-bot test eta please

tag @dotnet/roslyn-ide @DustinCampbell

@davkean
Copy link
Member

davkean commented Jan 28, 2016

@dotnet-bot retest prtest/mac/dbg/unit32 please
// Previous failure: http://dotnet-ci.cloudapp.net/job/roslyn_prtest_mac_dbg_unit32/3066/
// Retest reason: Mono deadlock

public enum Directions { North, East, South, West }
}
");
}
Copy link
Member

Choose a reason for hiding this comment

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

It's be good to add a test to ensure that it still reports redundant identity casts in this scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Also, tests for casts when the property name is omitted. E.g.

object thing = new { (int)Directions.South };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both good ideas, I'll try to get those in soon

@DustinCampbell
Copy link
Member

Ignore my comment about object thing = new { (int)Directions.South };. My good friend @CyrusNajmabadi just pointed out to me that's not legal syntactically. 😄

@davkean
Copy link
Member

davkean commented Feb 3, 2016

tag @dotnet/roslyn-ide for another look.

@DustinCampbell
Copy link
Member

LGTM 👍

@@ -430,10 +430,23 @@ protected override bool ReplacementChangesSemanticsForNodeLanguageSpecific(Synta
{
return !TypesAreCompatible((ImplicitArrayCreationExpressionSyntax)currentOriginalNode, (ImplicitArrayCreationExpressionSyntax)currentReplacedNode);
}
else if (currentOriginalNode is AnonymousObjectMemberDeclaratorSyntax)
Copy link
Contributor

Choose a reason for hiding this comment

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

why 'is' instead of the Kind checks like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw others with "is", especially at the top of the method, copied that style.

@CyrusNajmabadi
Copy link
Contributor

👍 just had a nit.

@davkean
Copy link
Member

davkean commented Feb 11, 2016

Sorry for the late reply on this, we'll try and get this merged in today.

@dotnet-bot test eta please
@dotnet-bot test vsi please

@natidea
Copy link
Contributor

natidea commented Feb 23, 2016

@dotnet-bot test eta please
@dotnet-bot test vsi please

@davkean
Copy link
Member

davkean commented Mar 1, 2016

@dotnet-bot test eta please
@dotnet-bot test vsi please

@DustinCampbell
Copy link
Member

test vsi please

@DustinCampbell
Copy link
Member

retest this please

@DustinCampbell
Copy link
Member

@steveniles -- sorry it took so long to get this one in. Running tests one more time. 😄

@steveniles
Copy link
Contributor Author

@DustinCampbell No problem, figured it would get done once the issues with the test server(s) were resolved :)

@DustinCampbell DustinCampbell merged commit 798e642 into dotnet:master Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE cla-already-signed Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
7 participants