Do not offer to remove cast if it will cause the compiler to warn about it.#43492
Conversation
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
same impl as VB. moved to common location so C#/VB compilers can reference this, as can IDE.
| /// <summary> | ||
| /// Returns true if the conversion is an nullable conversion. | ||
| /// </summary> | ||
| public bool IsNullable => (_conversionKind & ConversionKind.IsNullable) == ConversionKind.IsNullable; |
There was a problem hiding this comment.
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.
| Case Else | ||
| Return False | ||
| End Select | ||
| End Function |
There was a problem hiding this comment.
removed in favor of the identical common version.
| 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 |
| /// <summary> | ||
| /// Checks if a type is considered a "built-in integral" by CLR. | ||
| /// </summary> | ||
| public static bool IsIntegralType(this SpecialType specialType) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
| <ItemGroup Condition="'$(DefaultLanguageSourceExtension)' != '' AND '$(BuildingInsideVisualStudio)' != 'true'"> | ||
| <ExpectedCompile Include="$(MSBuildThisFileDirectory)**\*$(DefaultLanguageSourceExtension)" /> | ||
| </ItemGroup> | ||
| <ItemGroup> |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Compiler change LGTM Thanks (iteration 3)
333fred
left a comment
There was a problem hiding this comment.
Compiler changes LGTM (commit 3)
|
Thanks all! |
|
|
||
| private static bool CastRemovalWouldCauseSignExtensionWarning(ExpressionSyntax expression, SemanticModel semanticModel, CancellationToken cancellationToken) | ||
| { | ||
| // Logic copied from DiagnosticsPass_Warnings.CheckForBitwiseOrSignExtend. Including comments. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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 :-/
Fixes #40414