Fix issues with 'Remove unnecessary cast' by CyrusNajmabadi · Pull Request #42883 · dotnet/roslyn

CyrusNajmabadi · 2020-03-29T18:59:27Z

Fixes #41433
Fixes #38599
Fixes #32491
Fixes #25021
Fixes #2987
Fixes #38347
Fixes #36631
Fixes #34326
Fixes #29726
Fixes #6309
Fixes #34873
Fixes #37953
Fixes #42657
Fixes #42108

This PR should be reviewed one commit at a time.

CyrusNajmabadi · 2020-03-29T22:56:16Z

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/CastExpressionSyntaxExtensions.cs

-// The .NET Foundation licenses this file to you under the MIT license.
-// See the LICENSE file in the project root for more information.
-
-using System.Linq;


stale file. likely a merge conflict when we moved things to the shared layer.

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Simplification/Simplifiers/CastSimplifier.cs

src/Analyzers/CSharp/Tests/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.cs

sharwell · 2020-03-29T22:32:14Z

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Simplification/Simplifiers/CastSimplifier.cs

+                    return false;
+
                // Don't remove any explicit numeric casts.
                // https://github.com/dotnet/roslyn/issues/2987 tracks improving on this conservative approach.


❔ Comment still necessary?

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Simplification/Simplifiers/CastSimplifier.cs

sharwell · 2020-03-29T22:36:41Z

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Simplification/Simplifiers/CastSimplifier.cs

+            if (castedExpressionNode.WalkDownParentheses().IsKind(SyntaxKind.DefaultLiteralExpression) &&
+                !castType.Equals(outerType) &&
+                outerType.IsNullable())


😕 It's hard to relate this condition to the one it tests for. I'll need to come back to it.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ISymbolExtensions.cs

sharwell · 2020-03-30T01:56:43Z

Fixes #2987

Please update that issue to explain how this improvement was approached.

...es/SharedUtilitiesAndExtensions/Compiler/CSharp/Simplification/Simplifiers/CastSimplifier.cs

sharwell · 2020-03-30T02:18:14Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs

+                // Switch expression's expression changed.  Ensure it's the same type as before. If not, inference of
+                // the meaning of the patterns within can change.


❔ Do we not need to check the patterns, like we do for switch statements?

it's quite possble we need to for total correctness. i'm not certain at this point. going to leave it alone for now :)

...rkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AbstractSpeculationAnalyzer.cs

…ence types.

…mplification/Simplifiers/CastSimplifier.cs Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>

sharwell · 2020-03-30T03:42:33Z

src/EditorFeatures/CSharpTest/ImplementInterface/ImplementInterfaceTests.cs

    public int Compare(object x, object y)
    {
-        return this.x.Compare(x, y);
+        return ((IComparer)this.x).Compare(x, y);


📝 I'm OK with the approach taken in this pull request to put us back on a more conservative path, but we should file a follow-up issue to consider ways to improve this scenario in the future.

ghost

Auto-approval

CyrusNajmabadi added 8 commits March 29, 2020 11:58

Do not remove necessary cast from IntPtr to int*

c0290f6

Add casts

6625235

Fix remove-unnecessary-cast with arguments to local functions.

81c098c

Fix remove-unnecessary-cast with interpolations.

a4c31a6

Do not remove necessary casts to interfaces on non-sealed types.

6715484

Add test

c54b6ef

Do not remove necessary casts to default literal.

7e18d71

Do not remove necessary fp casts.

434a187

CyrusNajmabadi requested a review from mavasani March 29, 2020 22:55

CyrusNajmabadi commented Mar 29, 2020

View reviewed changes

CyrusNajmabadi added 3 commits March 29, 2020 15:57

Simplify

158edb5

Always check if the change is safe before presuming it is.

a2e7324

Do not remove necessary casts in switch expressions

e2f1f1a

CyrusNajmabadi marked this pull request as ready for review March 29, 2020 23:48

CyrusNajmabadi requested a review from a team as a code owner March 29, 2020 23:48

CyrusNajmabadi added 2 commits March 29, 2020 18:39

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

dc0dd2e

Restore logic

bc7a42b

sharwell reviewed Mar 30, 2020

View reviewed changes

CyrusNajmabadi mentioned this pull request Mar 30, 2020

Need to ensure cast-simplification works properly with intptr/nint #42888

Closed

Add test

882caaf

sharwell reviewed Mar 30, 2020

View reviewed changes

CyrusNajmabadi and others added 4 commits March 29, 2020 20:05

Remove special rules for removing casts to interfaces from user refer…

c90cadd

…ence types.

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Si…

8132170

…mplification/Simplifiers/CastSimplifier.cs Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Si…

95eb749

…mplification/Simplifiers/CastSimplifier.cs Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>

Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Si…

31b6249

…mplification/Simplifiers/CastSimplifier.cs Co-Authored-By: Sam Harwell <sam@tunnelvisionlabs.com>

CyrusNajmabadi mentioned this pull request Mar 30, 2020

Improve cast simplification for pointer conversions and explicit numeric casts in checked/unchecked context #2987

Closed

CyrusNajmabadi changed the title ~~Fix issues with 'Remove unneccessary cast'~~ Fix issues with 'Remove unnecessary cast' Mar 30, 2020

sharwell approved these changes Mar 30, 2020

View reviewed changes

CyrusNajmabadi added the auto-merge label Mar 30, 2020

ghost approved these changes Mar 30, 2020

View reviewed changes

Fixup remainder of tests

03cdf14

jinujoseph added this to the 16.6.P3 milestone Mar 30, 2020

CyrusNajmabadi added 3 commits March 29, 2020 23:00

Fixup test

42b9692

Remove casts from strings to interfaces.

5c2e487

Add docs

f73f19f

ghost merged commit fcefb5e into dotnet:master Mar 30, 2020

nxtn mentioned this pull request Mar 30, 2020

Don't remove pointer cast in return statements #39838

Closed

CyrusNajmabadi deleted the unnecessaryCAst branch March 30, 2020 17:42

This pull request was closed.

		// Switch expression's expression changed. Ensure it's the same type as before. If not, inference of
		// the meaning of the patterns within can change.

Conversation

CyrusNajmabadi commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sharwell commented Mar 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Mar 30, 2020

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CyrusNajmabadi commented Mar 29, 2020 •

edited

Loading