From fcd4ea019e51e53eea65672f0a30f987ca94e317 Mon Sep 17 00:00:00 2001 From: Jonathon Marolf Date: Tue, 2 Jun 2015 20:13:22 -0700 Subject: [PATCH] Correctly detect option strict on/off when determining if cast is redundant Fixes #3161 and Fixes #3163 User Scenario: User has code with narrowing conversion with an explicit cast. We will offer to remove the cast even though this will cause the code to not compile. Fix Description: We previously never consulted optionstrict to determine if a user-defined narrowing cast was necessary. The fix in CastAnalyzer.vb will check if the conversion is narrowing, if option strict is on, or if we generate a warning on implicit narrowing conversions. We also offer to add a cast if an implicit conversion warning is given. Testing: Added regression tests + existing tests --- .../Test/Workspaces/TestWorkspaceFactory.cs | 1 + .../TestWorkspaceFactory_XmlConsumption.cs | 8 +- .../TestWorkspaceFactory_XmlCreation.cs | 12 ++ ...agnosticProviderBasedUserDiagnosticTest.vb | 8 +- .../InsertMissingCastTests.vb | 7 + .../RemoveUnnecessaryCastTests.vb | 160 +++++++++++++++++- .../InsertMissingCastCodeFixProvider.vb | 3 +- .../Portable/Extensions/CastAnalyzer.vb | 25 ++- 8 files changed, 215 insertions(+), 9 deletions(-) diff --git a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory.cs b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory.cs index b5aae0351f1..bbaf15d056f 100644 --- a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory.cs +++ b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory.cs @@ -50,6 +50,7 @@ public partial class TestWorkspaceFactory private const string AnalyzerDisplayAttributeName = "Name"; private const string AnalyzerFullPathAttributeName = "FullPath"; private const string AliasAttributeName = "Alias"; + private const string DiagnosticOptionElementName = "DiagnosticOption"; /// /// Creates a single buffer in a workspace. diff --git a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlConsumption.cs b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlConsumption.cs index 3d4cc1f6896..da064d0003a 100644 --- a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlConsumption.cs +++ b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlConsumption.cs @@ -398,6 +398,7 @@ private static CompilationOptions CreateCompilationOptions(TestWorkspace workspa var rootNamespace = new VisualBasicCompilationOptions(OutputKind.ConsoleApplication).RootNamespace; var globalImports = new List(); var reportDiagnostic = ReportDiagnostic.Default; + var specificDiagnostics = ImmutableDictionary.Create(); if (compilationOptionsElement != null) { @@ -429,6 +430,10 @@ private static CompilationOptions CreateCompilationOptions(TestWorkspace workspa : new VisualBasicCompilationOptions(OutputKind.WindowsRuntimeMetadata).WithGlobalImports(globalImports) .WithRootNamespace(rootNamespace); } + + specificDiagnostics = compilationOptionsElement.Elements(DiagnosticOptionElementName) + .Select(x => KeyValuePair.Create(x.Value, (ReportDiagnostic)Enum.Parse(typeof(ReportDiagnostic), x.Attribute(ReportDiagnosticAttributeName).Value))) + .ToImmutableDictionary(); } else { @@ -446,7 +451,8 @@ private static CompilationOptions CreateCompilationOptions(TestWorkspace workspa .WithSourceReferenceResolver(SourceFileResolver.Default) .WithXmlReferenceResolver(XmlFileResolver.Default) .WithMetadataReferenceResolver(new AssemblyReferenceResolver(MetadataFileReferenceResolver.Default, MetadataFileReferenceProvider.Default)) - .WithAssemblyIdentityComparer(DesktopAssemblyIdentityComparer.Default); + .WithAssemblyIdentityComparer(DesktopAssemblyIdentityComparer.Default) + .WithSpecificDiagnosticOptions(specificDiagnostics); if (language == LanguageNames.VisualBasic) { diff --git a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlCreation.cs b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlCreation.cs index 361f7875c28..be0cd314a29 100644 --- a/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlCreation.cs +++ b/src/EditorFeatures/Test/Workspaces/TestWorkspaceFactory_XmlCreation.cs @@ -75,6 +75,18 @@ private static XElement CreateCompilationOptionsElement(CompilationOptions optio element.SetAttributeValue(RootNamespaceAttributeName, vbOptions.RootNamespace); } + if (vbOptions.SpecificDiagnosticOptions?.Any() == true) + { + element.Add( + vbOptions.SpecificDiagnosticOptions.AsEnumerable().Select( + i => + { + var x = new XElement(DiagnosticOptionElementName, i.Key); + x.SetAttributeValue(ReportDiagnosticAttributeName, System.Enum.GetName(typeof(ReportDiagnostic), i.Value)); + return x; + })); + } + return element; } diff --git a/src/EditorFeatures/VisualBasicTest/Diagnostics/AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest.vb b/src/EditorFeatures/VisualBasicTest/Diagnostics/AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest.vb index 36f1ed58839..99e9016da25 100644 --- a/src/EditorFeatures/VisualBasicTest/Diagnostics/AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest.vb +++ b/src/EditorFeatures/VisualBasicTest/Diagnostics/AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest.vb @@ -32,11 +32,15 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.Diagnostics Return input.Replace("\n", vbCrLf) End Function - Protected Overloads Sub Test(initialMarkup As XElement, expected As XElement, Optional index As Integer = 0, Optional compareTokens As Boolean = True) + Protected Overloads Sub Test(initialMarkup As XElement, expected As XElement, Optional index As Integer = 0, Optional compareTokens As Boolean = True, Optional compilationOptions As VisualBasicCompilationOptions = Nothing) Dim initialMarkupStr = initialMarkup.ConvertTestSourceTag() Dim expectedStr = expected.ConvertTestSourceTag() - MyBase.Test(initialMarkupStr, expectedStr, parseOptions:=Nothing, compilationOptions:=_compilationOptions, index:=index, compareTokens:=compareTokens) + If compilationOptions Is Nothing Then + compilationOptions = CType(_compilationOptions, VisualBasicCompilationOptions) + End If + + MyBase.Test(initialMarkupStr, expectedStr, parseOptions:=Nothing, compilationOptions:=compilationOptions, index:=index, compareTokens:=compareTokens) End Sub Protected Overloads Sub TestMissingWithWorkspaceXml(initialMarkup As XElement) diff --git a/src/EditorFeatures/VisualBasicTest/Diagnostics/InsertMissingCast/InsertMissingCastTests.vb b/src/EditorFeatures/VisualBasicTest/Diagnostics/InsertMissingCast/InsertMissingCastTests.vb index 9cc3d5f7f7d..137cc15cd84 100644 --- a/src/EditorFeatures/VisualBasicTest/Diagnostics/InsertMissingCast/InsertMissingCastTests.vb +++ b/src/EditorFeatures/VisualBasicTest/Diagnostics/InsertMissingCast/InsertMissingCastTests.vb @@ -87,5 +87,12 @@ NewLines("Option Strict On \n Module Program \n Sub Main(args As String()) \n Di TestMissing( NewLines("Option Strict On \n Class A \n End Class \n Class B \n End Class \n Module Program[||] \n Sub Main(args As String()) \n Dim x As A = New B() \n End Sub \n End Module")) End Sub + + + Public Sub TestOptionStrictOn() + Test( +NewLines("Option Strict On \n Module Module1 \n Sub Main() \n Dim red = ColorF.FromArgb(255, 255, 0, 0) \n Dim c As Color = [|red|] \n End Sub \n End Module \n Public Structure ColorF \n Public A, R, G, B As Single \n Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF \n Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} \n End Function \n Public Shared Widening Operator CType(x As Color) As ColorF \n Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) \n End Operator \n Public Shared Narrowing Operator CType(x As ColorF) As Color \n Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) \n End Operator \n End Structure \n Public Structure Color \n Public A, R, G, B As Byte \n Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color \n Return New Color With {.A = a, .R = r, .G = g, .B = b} \n End Function \n End Structure"), +NewLines("Option Strict On \n Module Module1 \n Sub Main() \n Dim red = ColorF.FromArgb(255, 255, 0, 0) \n Dim c As Color = CType(red, Color) \n End Sub \n End Module \n Public Structure ColorF \n Public A, R, G, B As Single \n Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF \n Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} \n End Function \n Public Shared Widening Operator CType(x As Color) As ColorF \n Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) \n End Operator \n Public Shared Narrowing Operator CType(x As ColorF) As Color \n Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) \n End Operator \n End Structure \n Public Structure Color \n Public A, R, G, B As Byte \n Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color \n Return New Color With {.A = a, .R = r, .G = g, .B = b} \n End Function \n End Structure")) + End Sub End Class End Namespace diff --git a/src/EditorFeatures/VisualBasicTest/Diagnostics/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.vb b/src/EditorFeatures/VisualBasicTest/Diagnostics/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.vb index fe788a93de4..4272f229170 100644 --- a/src/EditorFeatures/VisualBasicTest/Diagnostics/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.vb +++ b/src/EditorFeatures/VisualBasicTest/Diagnostics/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.vb @@ -2363,7 +2363,7 @@ End Class Public Sub RemoveCastToICloneableForDelegate() - ' Note: The cast below can be removed because delegates are implictly sealed. + ' Note: The cast below can be removed because delegates are implicitly sealed. Dim markup = @@ -2396,7 +2396,7 @@ End Class Public Sub RemoveCastToICloneableForArray() - ' Note: The cast below can be removed because arrays are implictly sealed. + ' Note: The cast below can be removed because arrays are implicitly sealed. Dim markup = @@ -2427,7 +2427,7 @@ End Class Public Sub RemoveCastToICloneableForArray2() - ' Note: The cast below can be removed because arrays are implictly sealed. + ' Note: The cast below can be removed because arrays are implicitly sealed. Dim markup = @@ -2456,7 +2456,7 @@ End Module Public Sub RemoveCastToIConvertibleForEnum() - ' Note: The cast below can be removed because enums are implictly sealed. + ' Note: The cast below can be removed because enums are implicitly sealed. Dim markup = @@ -2694,5 +2694,157 @@ End Class TestMissing(markup) End Sub + + + + Public Sub DoNotRemoveCastInNarrowingConverionWihtOptionStrictOn() + Dim markup = + +Option Strict On + +Module Module1 + + Sub Main() + Dim red = ColorF.FromArgb(255, 255, 0, 0) + Dim c As Color = [|CType(red, Color)|] + End Sub + +End Module + +Public Structure ColorF + Public A, R, G, B As Single + Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF + Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} + End Function + Public Shared Widening Operator CType(x As Color) As ColorF + Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) + End Operator + Public Shared Narrowing Operator CType(x As ColorF) As Color + Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) + End Operator +End Structure + +Public Structure Color + Public A, R, G, B As Byte + Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color + Return New Color With {.A = a, .R = r, .G = g, .B = b} + End Function +End Structure + + TestMissing(markup) + End Sub + + + + Public Sub DoNotRemoveCastInNarrowingConverionWihtOptionStrictOffBC42016AsWarning() + Dim markup = + +Option Strict Off + +Module Module1 + + Sub Main() + Dim red = ColorF.FromArgb(255, 255, 0, 0) + Dim c As Color = [|CType(red, Color)|] + End Sub + +End Module + +Public Structure ColorF + Public A, R, G, B As Single + Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF + Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} + End Function + Public Shared Widening Operator CType(x As Color) As ColorF + Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) + End Operator + Public Shared Narrowing Operator CType(x As ColorF) As Color + Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) + End Operator +End Structure + +Public Structure Color + Public A, R, G, B As Byte + Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color + Return New Color With {.A = a, .R = r, .G = g, .B = b} + End Function +End Structure + + TestMissing(markup) + End Sub + + + + Public Sub RemoveCastInNarrowingConverionWihtOptionStrictOffBC42016Suppressed() + Dim markup = + +Option Strict Off + +Module Module1 + + Sub Main() + Dim red = ColorF.FromArgb(255, 255, 0, 0) + Dim c As Color = [|CType(red, Color)|] + End Sub + +End Module + +Public Structure ColorF + Public A, R, G, B As Single + Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF + Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} + End Function + Public Shared Widening Operator CType(x As Color) As ColorF + Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) + End Operator + Public Shared Narrowing Operator CType(x As ColorF) As Color + Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) + End Operator +End Structure + +Public Structure Color + Public A, R, G, B As Byte + Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color + Return New Color With {.A = a, .R = r, .G = g, .B = b} + End Function +End Structure + + Dim expected = + +Option Strict Off + +Module Module1 + + Sub Main() + Dim red = ColorF.FromArgb(255, 255, 0, 0) + Dim c As Color = red + End Sub + +End Module + +Public Structure ColorF + Public A, R, G, B As Single + Public Shared Function FromArgb(a As Double, r As Double, g As Double, b As Double) As ColorF + Return New ColorF With {.A = CSng(a), .R = CSng(r), .G = CSng(g), .B = CSng(b)} + End Function + Public Shared Widening Operator CType(x As Color) As ColorF + Return ColorF.FromArgb(x.A / 255, x.R / 255, x.G / 255, x.B / 255) + End Operator + Public Shared Narrowing Operator CType(x As ColorF) As Color + Return Color.FromArgb(CByte(x.A * 255), CByte(x.R * 255), CByte(x.G * 255), CByte(x.B * 255)) + End Operator +End Structure + +Public Structure Color + Public A, R, G, B As Byte + Public Shared Function FromArgb(a As Byte, r As Byte, g As Byte, b As Byte) As Color + Return New Color With {.A = a, .R = r, .G = g, .B = b} + End Function +End Structure + + Dim compilationOptions = New VisualBasicCompilationOptions(OutputKind.ConsoleApplication).WithOptionInfer(True) + compilationOptions = compilationOptions.WithSpecificDiagnosticOptions(compilationOptions.SpecificDiagnosticOptions.Add("BC42016", ReportDiagnostic.Suppress)) + Test(markup, expected, compareTokens:=False, compilationOptions:=compilationOptions) + End Sub End Class End Namespace diff --git a/src/Features/VisualBasic/CodeFixes/InsertMissingCast/InsertMissingCastCodeFixProvider.vb b/src/Features/VisualBasic/CodeFixes/InsertMissingCast/InsertMissingCastCodeFixProvider.vb index 0804811bccb..df0bad79f94 100644 --- a/src/Features/VisualBasic/CodeFixes/InsertMissingCast/InsertMissingCastCodeFixProvider.vb +++ b/src/Features/VisualBasic/CodeFixes/InsertMissingCast/InsertMissingCastCodeFixProvider.vb @@ -12,10 +12,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeFixes.InsertMissingCast Inherits CodeFixProvider Friend Const BC30512 As String = "BC30512" ' Option Strict On disallows implicit conversions from '{0}' to '{1}'. + Friend Const BC42016 As String = "BC42016" ' Implicit conversions from '{0}' to '{1}'. Public NotOverridable Overrides ReadOnly Property FixableDiagnosticIds As ImmutableArray(Of String) Get - Return ImmutableArray.Create(BC30512) + Return ImmutableArray.Create(BC30512, BC42016) End Get End Property diff --git a/src/Workspaces/VisualBasic/Portable/Extensions/CastAnalyzer.vb b/src/Workspaces/VisualBasic/Portable/Extensions/CastAnalyzer.vb index b6edbc3c8b6..67200f6662c 100644 --- a/src/Workspaces/VisualBasic/Portable/Extensions/CastAnalyzer.vb +++ b/src/Workspaces/VisualBasic/Portable/Extensions/CastAnalyzer.vb @@ -253,7 +253,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions If castToOuterType.IsUserDefined OrElse expressionToCastType.IsUserDefined Then Return (HaveSameUserDefinedConversion(expressionToCastType, expressionToOuterType) OrElse HaveSameUserDefinedConversion(castToOuterType, expressionToOuterType)) AndAlso - UserDefinedConversionIsAllowed(_castNode, _semanticModel) + (UserDefinedConversionIsAllowed(_castNode, _semanticModel) AndAlso + NarrowingConversionIsAllowed(expressionToCastType, _semanticModel)) ElseIf expressionToOuterType.IsUserDefined Then Return False End If @@ -342,6 +343,28 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions Return True End Function + Private Shared Function NarrowingConversionIsAllowed(conversion As Conversion, semanticModel As SemanticModel) As Boolean + ' If this is not a narrowing conversion then no VB compiler options will disallow it + If conversion.IsNarrowing Then + ' If Option Strict is On then all narrowing conversions are illegal + If semanticModel.OptionStrict = OptionStrict.On Then + Return False + Else + ' For all other cases we only want to label a cast as unnecessary if BC42016 is suppressed, + ' otherwise removing the cast will cause a warning or error to be shown to the user. + Dim reportDiagnostic As ReportDiagnostic + If semanticModel.Compilation.Options.SpecificDiagnosticOptions.TryGetValue("BC42016", reportDiagnostic) AndAlso + reportDiagnostic = ReportDiagnostic.Suppress Then + Return True + Else + Return False + End If + End If + End If + + Return True + End Function + Private Shared Function IsRequiredWideningNumericConversion(sourceType As ITypeSymbol, destinationType As ITypeSymbol) As Boolean ' VB Language Specification: Section 8.3 Numeric Conversions -- GitLab