Do not offer to remove cast if it will cause the compiler to warn about it. #43492

CyrusNajmabadi · 2020-04-19T22:18:52Z

…ut it.

CyrusNajmabadi · 2020-04-19T22:19:15Z

src/Compilers/CSharp/Portable/Symbols/SpecialTypeExtensions.cs

-                default:
-                    return false;
-            }
-        }


same impl as VB. moved to common location so C#/VB compilers can reference this, as can IDE.

CyrusNajmabadi · 2020-04-19T22:52:47Z

src/Compilers/Core/Portable/Operations/CommonConversion.cs

@@ -50,6 +52,10 @@ internal CommonConversion(bool exists, bool isIdentity, bool isNumeric, bool isR
        /// </summary>
        public bool IsIdentity => (_conversionKind & ConversionKind.IsIdentity) == ConversionKind.IsIdentity;
        /// <summary>
+        /// Returns true if the conversion is an nullable conversion.
+        /// </summary>
+        public bool IsNullable => (_conversionKind & ConversionKind.IsNullable) == ConversionKind.IsNullable;


this seems safe to add to the common layer. Both VB and C# have htis concept and it seems identical from my reading. Namely:

c#:

Implicit nullable conversions

Predefined implicit conversions that operate on non-nullable value types can also be used with nullable forms of those types. For each of the predefined implicit identity and numeric conversions that convert from a non-nullable value type S to a non-nullable value type T, the following implicit nullable conversions exist:

An implicit conversion from S? to T?.

An implicit conversion from S to T?.

Evaluation of an implicit nullable conversion based on an underlying conversion from S to T proceeds as follows:

If the nullable conversion is from S? to T?:

If the source value is null (HasValue property is false), the result is the null value of type T?.

Otherwise, the conversion is evaluated as an unwrapping from S? to S, followed by the underlying conversion from S to T, followed by a wrapping (Nullable types) from T to T?.

If the nullable conversion is from S to T?, the conversion is evaluated as the underlying conversion from S to T followed by a wrapping from T to T?.

(and the corresponding explicit version of this)

VB:

A value type T can convert to and from the nullable version of the type, T?. The conversion from T? to T throws a System.InvalidOperationException exception if the value being converted is Nothing. Also, T? has a conversion to a type S if T has an intrinsic conversion to S. And if S is a value type, then the following intrinsic conversions exist between T? and S?:

A conversion of the same classification (narrowing or widening) from T? to S?.

A conversion of the same classification (narrowing or widening) from T to S?.

A narrowing conversion from S? to T.

CyrusNajmabadi · 2020-04-19T22:53:08Z

src/Compilers/VisualBasic/Portable/Symbols/SpecialTypeExtensions.vb

-                Case Else
-                    Return False
-            End Select
-        End Function


removed in favor of the identical common version.

CyrusNajmabadi · 2020-04-21T17:45:01Z

@dotnet/roslyn-compiler @jcouv @333fred could i get eyes here? Thanks!

jcouv · 2020-04-21T20:03:37Z

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -22,6 +22,7 @@ Microsoft.CodeAnalysis.ITypeSymbol.IsNativeIntegerType.get -> bool
 Microsoft.CodeAnalysis.InitializationContext
 Microsoft.CodeAnalysis.InitializationContext.CancellationToken.get -> System.Threading.CancellationToken
 Microsoft.CodeAnalysis.InitializationContext.RegisterForSyntaxNotifications(Microsoft.CodeAnalysis.SyntaxReceiverCreator receiverCreator) -> void
+Microsoft.CodeAnalysis.Operations.CommonConversion.IsNullable.get -> bool


Tagging @jaredpar @gafter @333fred as FYI for public API change

jcouv · 2020-04-21T20:05:29Z

src/Compilers/Core/Portable/SpecialTypeExtensions.cs

+        /// <summary>
+        /// Checks if a type is considered a "built-in integral" by CLR.
+        /// </summary>
+        public static bool IsIntegralType(this SpecialType specialType)


IsIntegralType [](start = 27, length = 14)

Please add a compiler test that will fail once this change is merged with the native ints feature. That way we'll be reminded/forced to add logic here.

Hum, I got confused. Native ints have been merged already.
Tagging @cston to confirm whether this method should handle them.

In reply to: 412454952 [](ancestors = 412454952)

I reviewed this method before merging the native integers feature, and didn't find a caller that might have a native integer type.

In reply to: 412468506 [](ancestors = 412468506,412454952)

jcouv · 2020-04-21T20:28:46Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems

@@ -486,4 +487,7 @@
  <ItemGroup Condition="'$(DefaultLanguageSourceExtension)' != '' AND '$(BuildingInsideVisualStudio)' != 'true'">
    <ExpectedCompile Include="$(MSBuildThisFileDirectory)**\*$(DefaultLanguageSourceExtension)" />
  </ItemGroup>
+  <ItemGroup>


What does this do?

basically, it gives us a folder in the UI where this linked file goes. it seems needed because we have our own file called SpecialTypeExtensions, and we want to link in yours.

jcouv

Compiler change LGTM Thanks (iteration 3)

333fred

Compiler changes LGTM (commit 3)

CyrusNajmabadi · 2020-04-22T04:28:33Z

Thanks all!

mavasani · 2020-04-22T16:58:59Z

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

@@ -315,6 +320,227 @@ public static bool IsUnnecessaryAsCast(BinaryExpressionSyntax cast, SemanticMode
            return false;
        }

+        private static bool CastRemovalWouldCauseSignExtensionWarning(ExpressionSyntax expression, SemanticModel semanticModel, CancellationToken cancellationToken)
+        {
+            // Logic copied from DiagnosticsPass_Warnings.CheckForBitwiseOrSignExtend.  Including comments.


Would it be possible to move this logic to a shared helper class in a separate file in compiler layer, so it can be linked into this project instead of cloned? I am concerned this would diverge pretty soon and introduce bugs.

I understand this has lot of customer logic specific to the analyzer, but it would be good if at least the core logic that can possibly be shared is shared. Please let me know if you have already done so for everything possible, in which case it is fine.

Unfortunately, the compiler logic is all built on compiler internals (i.e. bound nodes).

i did have to tweak some things. for example, our function says: would this be an issue if the cast was removed. the compiler version obviously is written analyzing the actual expression witht the cast.

I am concerned this would diverge pretty soon and introduce bugs.

So i looked, and this hasn't changed in the compiler layer since we went open source. it's effectively a very very old holdover from the native compiler. So i'm not really concerned about that.

Please let me know if you have already done so for everything possible, in which case it is fine.

I tried to share as much as i could. all the helper extensions are shared. but the core logic that does this specific check is still a copy :-/

Do not offer to remove cast if it will cause the compiler to warn abo…

5cbf183

…ut it.

CyrusNajmabadi requested review from a team as code owners April 19, 2020 22:18

CyrusNajmabadi commented Apr 19, 2020

View reviewed changes

Add nullable tests.

86fc711

CyrusNajmabadi commented Apr 19, 2020

View reviewed changes

Fix

b134a45

jcouv reviewed Apr 21, 2020

View reviewed changes

jcouv self-assigned this Apr 21, 2020

jcouv added Area-Compilers Area-IDE labels Apr 21, 2020

jcouv approved these changes Apr 21, 2020

View reviewed changes

333fred approved these changes Apr 22, 2020

View reviewed changes

CyrusNajmabadi merged commit 8f4fdad into dotnet:master Apr 22, 2020

ghost added this to the Next milestone Apr 22, 2020

CyrusNajmabadi deleted the orWideningCastCheck branch April 22, 2020 04:28

mavasani reviewed Apr 22, 2020

View reviewed changes

sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not offer to remove cast if it will cause the compiler to warn about it. #43492

Do not offer to remove cast if it will cause the compiler to warn about it. #43492

CyrusNajmabadi commented Apr 19, 2020

CyrusNajmabadi Apr 19, 2020

CyrusNajmabadi Apr 19, 2020

CyrusNajmabadi Apr 19, 2020

CyrusNajmabadi commented Apr 21, 2020

jcouv Apr 21, 2020

jcouv Apr 21, 2020

jcouv Apr 21, 2020

cston Apr 21, 2020

jcouv Apr 21, 2020

CyrusNajmabadi Apr 21, 2020

jcouv left a comment

333fred left a comment

CyrusNajmabadi commented Apr 22, 2020

mavasani Apr 22, 2020

mavasani Apr 22, 2020

CyrusNajmabadi Apr 22, 2020

Do not offer to remove cast if it will cause the compiler to warn about it. #43492

Do not offer to remove cast if it will cause the compiler to warn about it. #43492

Conversation

CyrusNajmabadi commented Apr 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment