Reimplement the logic for remove-unnecessary-cast. by CyrusNajmabadi · Pull Request #56932 · dotnet/roslyn

CyrusNajmabadi · 2021-10-03T00:29:38Z

Fixes #52524
Fixes #53698
Fixes #55621
Fixes #56207

Should be reviewed one commit at a time.

This PR replaces the entire existing implementation of 'RemoveUnnecessaryCast' with a new one built on top of simpler logic based on first principles around the semantic operations the compiler forms.

As part of this much of the test cases were beefed up. This is the majority of the PR size here. In terms of hte simplifier itself, we went down from a whopping 3500 lines to just 800 lines. Around 25% the size/complexity of what it used to be.

The most general rule that this follows is the question: does the expression without a cast get converted to the same type that the original expression converted to? If so, this is a candidate for removal. In almost all cases, this is safe too. However, there are some unfortunately complex corner cases in the language that need to be handled (including cases where the change is legal, but the compiler issues warnings that code may be confusing).

The changes also follow the rule that the emitted code should effectively be the same as without the cast. However, following that rule regresses a a few cases we used to support. There are no known false positives. However, there are a few corner cases where things we used to support are no longer supported. These cases violated the above rule that the emitted code would be the same without the cast (though at runtime this would not be observable). #56938 tracks the cases we don't support anymore to bring back in the future.

On another positive note, this change fixes several outstanding reported bugs that would have been difficult to fix before. It also fixes several bits of broken behavior observed in many features where casts were either removed inappropriately, or not removed where they should have been.

…ing tests.

… tests.

…g tests.

…passing, 81 failing tests.

…reference type. 244 passing, 73 failing tests.

…9 failing tests.

…xing. 253 passing, 70 failing tests.

…ling tests.

…sts.

… 274 passing, 52 failing tests.

…g, 78 failing tests.

…g, 72 failing tests.

…78 passing, 68 failing tests.

…s not sealed. 381 passing, 65 failing tests.

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

sharwell · 2021-10-08T16:05:36Z

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

+            if (rewrittenOperation.Parent is not IDelegateCreationOperation rewrittenDelegateCreationOperation)
+                return false;

-            RoslynDebug.Assert(operation.Type is not null);
-            if (!operation.Type.SpecialType.IsIntegralType())
-                return null;
+            if (rewrittenDelegateCreationOperation.Type?.TypeKind != TypeKind.Delegate)
+                return false;


❔ Can these be combined? Not sure if pattern IDelegateCreationOperation { Type.TypeKind: TypeKind.Delegate } accounts for the possibility that Type is null.

accounts for the possibility that Type is null.

Yup. It does. That's inherent in dotted property patterns.

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

sharwell · 2021-10-08T16:44:03Z

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

            if (!conversion.IsIdentity)
                return false;

+            var castType = semanticModel.GetTypeInfo(castNode, cancellationToken).Type;
+            var castedExpressionType = semanticModel.GetTypeInfo(castedExpressionNode, cancellationToken).Type;
+
            // Floating point casts can have subtle runtime behavior, even between the same fp types. For example, a
            // cast from float-to-float can still change behavior because it may take a higher precision computation and
            // truncate it to 32bits.
            //
            // Because of this we keep floating point conversions unless we can prove that it's safe.  The only safe
            // times are when we're loading or storing into a location we know has the same size as the cast size
            // (i.e. reading/writing into a field).


Should boxing be included?

float value = 0.0f; object boxed = (float)value;

hrmm... this is a good case. @tannergooding ignoring the value 0.0f, is the above guaranteed to always produce the exact same results as:

float value = ...; object boxed = value;

I'm guess so as boxing the value means we'll have to narrow to 32bits, so the explicit cast to force a potentially widened float to 32bits happens either way.

(and i presume the same happens for double as well)?

Since value is already a float, I believe there is no need to insert an additional cast here.

-- The runtime spec dictates that the type on the evaluation stack is F, not float32 and not float64. This means that:

loading a float32 or float64 onto the stack "converts" it to type F, in the strictest sense.

operations, such as x + y (where x and y are both floating-point types) results in a value of type F.

if you don't explicitly downcast and consume the value, you "can" see differing behavior on some platforms, which is why C# says the cast in (float)(x + y) + z is required.

In practice, on modern .NET, this largely only matters for certain scenarios on 32-bit; 64-bit exclusively uses the SSE/SSE2 instructions and directly operate on float32 or float64

However, some runtime is free to implement these as say 80-bit operations; such as was done on the x87 FPU stack

However, box doesn't interpret the value based on the evaluation stack; instead it takes an explicit typeTok that specifies how the value should be interpreted and C# correctly tracks x and x + y and x + y + z as float, so the type token emitted here is System.Single

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

…mabadi/roslyn into removeUnnecessaryCastV2

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

…mabadi/roslyn into removeUnnecessaryCastV2

Fork the existing code so we can run both versions SxS.

cb7d4e8

ghost added the Area-IDE label Oct 3, 2021

Remove existing implementation. 92 passing, 217 failing tests.

b10a2a3

CyrusNajmabadi force-pushed the removeUnnecessaryCastV2 branch from ce515c4 to b10a2a3 Compare October 3, 2021 00:47

CyrusNajmabadi added 2 commits October 2, 2021 17:49

Don't remove casts with syntax errors. 93 passing, 216 failing tests.

5047e05

Have to have an IConversionOperation to proceed. 88 passing, 221 fail…

67ca12e

…ing tests.

CyrusNajmabadi force-pushed the removeUnnecessaryCastV2 branch from 88f3510 to 67ca12e Compare October 3, 2021 00:55

CyrusNajmabadi added 8 commits October 2, 2021 17:57

Conversion has to exist to proceed. 90 passing, 219 failing tests.

4b0316f

Explicit casts cannot be removed. 151 passing, 158 failing tests.

3f70f22

Original conversion must be to a legal type. 151 passing, 158 failing…

86b1cff

… tests.

Rewritten conversion must be to a legal type. 160 passing, 149 failin…

762e5b5

…g tests.

Rewritten conversion must be the same type as the original code. 228 …

1cdbaa2

…passing, 81 failing tests.

Fix incorrect tests. Add new tests. 244 passing, 73 failing tests.

380f52d

Don't remove casts that woudl cause null to need to convert to a non-…

14c8637

…reference type. 244 passing, 73 failing tests.

Null can be converted to a nullable type. 245 passing, 72 failing tests.

b92efe8

CyrusNajmabadi mentioned this pull request Oct 3, 2021

Compiler doesn't have conversion node for expressions in interpolation holes #56934

Open

CyrusNajmabadi added 4 commits October 2, 2021 19:30

Casts to 'object' in an interpolation are unnecessary. 248 passing, 6…

8e68b6e

…9 failing tests.

Do not remove casts that change the type of an expression prior to bo…

f653c2e

…xing. 253 passing, 70 failing tests.

Do not remove necessary formattable string casts. 258 passing, 67 fai…

e68fef8

…ling tests.

Do not remove 'default' cast in a pattern. 262 passing, 58 failing te…

2711c53

…sts.

CyrusNajmabadi requested a review from jcouv October 3, 2021 03:23

CyrusNajmabadi added 2 commits October 2, 2021 20:25

Do not remove 'default' cast in a pattern. 271 passing, 54 failing te…

176fc56

…sts.

Do not remove cast if it would remove a user defined conversion call.…

74e44bf

… 274 passing, 52 failing tests.

CyrusNajmabadi force-pushed the removeUnnecessaryCastV2 branch from 9aaff7c to 74e44bf Compare October 3, 2021 03:44

CyrusNajmabadi mentioned this pull request Oct 3, 2021

Move SymbolKey to a shared-source location so it can be used by our analzyer code. #56935

Merged

CyrusNajmabadi added 5 commits October 2, 2021 21:47

Merge branch 'shareSymbolKey' into removeUnnecessaryCastV2

8ec380c

Handle cast removal for expressions with members accesses. 368 passin…

ade841e

…g, 78 failing tests.

Handle cast removal for expressions with members accesses. 374 passin…

ad52d49

…g, 72 failing tests.

Cannot remove a boxing cast as semantic can change due to the copy. 3…

2455559

…78 passing, 68 failing tests.

Keeping the same call to an interface method is ok even if hte type i…

37049c3

…s not sealed. 381 passing, 65 failing tests.

CyrusNajmabadi added 2 commits October 7, 2021 11:09

Handle flipped case.

119936c

Add tests

d1e12e7

CyrusNajmabadi requested review from RikkiGibson and sharwell October 7, 2021 18:23

CyrusNajmabadi added 2 commits October 7, 2021 16:20

Use simpler sign extension check.

bcf4d5e

fxi speeling.

2eaecc8

sharwell reviewed Oct 8, 2021

View reviewed changes

CyrusNajmabadi and others added 2 commits October 8, 2021 11:29

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

eae8e45

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

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

324200c

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

sharwell approved these changes Oct 8, 2021

View reviewed changes

CyrusNajmabadi and others added 9 commits October 8, 2021 11:32

Move diag check up

690b6eb

Inline method

37feb18

Merge branch 'removeUnnecessaryCastV2' of https://github.com/CyrusNaj…

2682302

…mabadi/roslyn into removeUnnecessaryCastV2

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

2759305

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

Add testS

717fe48

Add tests

7c310f8

Merge branch 'removeUnnecessaryCastV2' of https://github.com/CyrusNaj…

0142c88

…mabadi/roslyn into removeUnnecessaryCastV2

Add tests for stackalloc behavior.

3b2bee6

remove unnecessary fp cast prior to boxing.

99bde27

CyrusNajmabadi enabled auto-merge October 8, 2021 19:55

CyrusNajmabadi merged commit 29508e5 into dotnet:main Oct 9, 2021

ghost added this to the Next milestone Oct 9, 2021

CyrusNajmabadi deleted the removeUnnecessaryCastV2 branch October 9, 2021 01:46

RikkiGibson modified the milestones: Next, 17.1.P1 Oct 25, 2021

This was referenced Dec 7, 2021

Add test demonstrating issue does not repro. #58178

Merged

"IDE0004 C# Cast is redundant" is reported incorrectly for default #49954

Closed

Youssef1313 mentioned this pull request Jan 7, 2022

IDE0044 or CS0266 - sbyte assignment via ternary operator and integer literal leaves you with no choice #58709

Closed

Youssef1313 mentioned this pull request Mar 14, 2022

[Use switch expression (IDE0066)]: Incorrect Code Suggestion #60159

Closed

Youssef1313 mentioned this pull request Aug 18, 2022

New rule: prefer Span.Clear over Span.Fill(default) dotnet/roslyn-analyzers#5181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement the logic for remove-unnecessary-cast.#56932

Reimplement the logic for remove-unnecessary-cast.#56932
CyrusNajmabadi merged 131 commits intodotnet:mainfrom
CyrusNajmabadi:removeUnnecessaryCastV2

CyrusNajmabadi commented Oct 3, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Oct 8, 2021

Uh oh!

CyrusNajmabadi Oct 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Oct 8, 2021

Uh oh!

CyrusNajmabadi Oct 8, 2021 •

edited

Loading

Uh oh!

tannergooding Oct 8, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

CyrusNajmabadi commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 8, 2021 • 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!

tannergooding Oct 8, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CyrusNajmabadi commented Oct 3, 2021 •

edited

Loading

CyrusNajmabadi Oct 8, 2021 •

edited

Loading

tannergooding Oct 8, 2021 •

edited

Loading