From 84f9744163c0f0c1d2abb490963473913986cde9 Mon Sep 17 00:00:00 2001 From: VSadov Date: Thu, 10 Nov 2016 16:26:10 -0800 Subject: [PATCH] CR feedback --- .../SymbolDisplayVisitor.Types.cs | 2 +- .../Symbols/Tuples/TupleErrorFieldSymbol.cs | 10 +++++- .../Symbols/Tuples/TupleFieldSymbol.cs | 2 +- .../Test/Emit/CodeGen/CodeGenTupleTest.cs | 31 ++++++++++++++++--- .../CodeAnalysisResources.Designer.cs | 9 ++++++ .../Core/Portable/CodeAnalysisResources.resx | 3 ++ .../Core/Portable/Compilation/Compilation.cs | 8 +++++ .../Portable/Symbols/ISymbolExtensions.cs | 30 ++++++++++++++++++ .../SymbolDisplayVisitor.Types.vb | 2 +- .../Symbols/Tuples/TupleErrorFieldSymbol.vb | 8 ++++- .../Symbols/Tuples/TupleFieldSymbol.vb | 2 +- .../Test/Emit/CodeGen/CodeGenTuples.vb | 28 ++++++++++++++--- ...lEquivalenceComparer.GetHashCodeVisitor.cs | 7 +---- .../SymbolKey/SymbolKey.TupleTypeSymbolKey.cs | 8 ++--- 14 files changed, 126 insertions(+), 24 deletions(-) diff --git a/src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs b/src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs index ff2422a2b27..8e9559fa164 100644 --- a/src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs +++ b/src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs @@ -418,7 +418,7 @@ private bool CanUseTupleTypeName(INamedTypeSymbol tupleSymbol) private static bool HasNonDefaultTupleElements(INamedTypeSymbol tupleSymbol) { - return tupleSymbol.TupleElements.Any(e=>(object)e != e.CorrespondingTupleField); + return tupleSymbol.TupleElements.Any(e => !e.IsDefaultTupleElement()); } private void AddTupleTypeName(INamedTypeSymbol symbol) diff --git a/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs index 34fbd98ab3c..fefbc88b96f 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleErrorFieldSymbol.cs @@ -50,7 +50,7 @@ internal sealed class TupleErrorFieldSymbol : SynthesizedFieldSymbolBase _tupleElementIndex = (object)correspondingDefaultFieldOpt == null ? tupleElementIndex << 1 : (tupleElementIndex << 1) + 1; _isImplicitlyDeclared = isImplicitlyDeclared; - Debug.Assert(correspondingDefaultFieldOpt != null || this.IsDefaultTupleElement); + Debug.Assert((correspondingDefaultFieldOpt == null) == this.IsDefaultTupleElement); Debug.Assert(correspondingDefaultFieldOpt == null || correspondingDefaultFieldOpt.IsDefaultTupleElement); _correspondingDefaultField = correspondingDefaultFieldOpt ?? this; @@ -126,6 +126,14 @@ public override bool IsImplicitlyDeclared } } + public override FieldSymbol CorrespondingTupleField + { + get + { + return _correspondingDefaultField; + } + } + internal override bool SuppressDynamicAttribute { get diff --git a/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs index a07cf13bd6b..815ab4b99e3 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Tuples/TupleFieldSymbol.cs @@ -165,7 +165,7 @@ internal class TupleElementFieldSymbol : TupleFieldSymbol _locations = location == null ? ImmutableArray.Empty : ImmutableArray.Create(location); _isImplicitlyDeclared = isImplicitlyDeclared; - Debug.Assert(correspondingDefaultFieldOpt != null || this.IsDefaultTupleElement); + Debug.Assert((correspondingDefaultFieldOpt == null) == this.IsDefaultTupleElement); Debug.Assert(correspondingDefaultFieldOpt == null || correspondingDefaultFieldOpt.IsDefaultTupleElement); _correspondingDefaultField = correspondingDefaultFieldOpt ?? this; diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs index a7d23700e60..e8cb7bb17a9 100644 --- a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs +++ b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs @@ -4843,7 +4843,7 @@ private static ImmutableArray GetTupleElementNames(INamedTypeSymbol tupl return default(ImmutableArray); } - return elements.SelectAsArray(e => e.IsImplicitlyDeclared? null: e.Name); + return elements.SelectAsArray(e => e.ProvidedTupleElementNameOrNull()); } [Fact] @@ -5116,10 +5116,11 @@ public void CreateTupleTypeSymbol_BadNames() NamedTypeSymbol intType = comp.GetSpecialType(SpecialType.System_Int32); var vt2 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).Construct(intType, intType); + var vt3 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T3).Construct(intType, intType, intType); - // illegal C# identifiers and blank - var tuple2 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("123", " ")); - Assert.Equal(new[] { "123", " " }, GetTupleElementNames(tuple2)); + // illegal C# identifiers, space and null + var tuple2 = comp.CreateTupleTypeSymbol(vt3, ImmutableArray.Create("123", " ", null)); + Assert.Equal(new[] { "123", " ", null }, GetTupleElementNames(tuple2)); // reserved identifiers var tuple3 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("return", "class")); @@ -5137,6 +5138,28 @@ public void CreateTupleTypeSymbol_BadNames() } } + [Fact] + public void CreateTupleTypeSymbol_EmptyNames() + { + var comp = CSharpCompilation.Create("test", references: new[] { MscorlibRef }); + + NamedTypeSymbol intType = comp.GetSpecialType(SpecialType.System_Int32); + var vt2 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).Construct(intType, intType); + + // not tuple-compatible underlying type + try + { + // illegal C# identifiers and blank + var tuple2 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("123", "")); + Assert.True(false); + } + catch (ArgumentException e) + { + Assert.Contains(CodeAnalysisResources.TupleElementNameEmpty, e.Message); + Assert.Contains("1", e.Message); + } + } + [Fact] public void CreateTupleTypeSymbol_VisualBasicElements() { diff --git a/src/Compilers/Core/Portable/CodeAnalysisResources.Designer.cs b/src/Compilers/Core/Portable/CodeAnalysisResources.Designer.cs index 13fa558d78c..aa8dda137e5 100644 --- a/src/Compilers/Core/Portable/CodeAnalysisResources.Designer.cs +++ b/src/Compilers/Core/Portable/CodeAnalysisResources.Designer.cs @@ -1333,6 +1333,15 @@ internal class CodeAnalysisResources { } } + /// + /// Looks up a localized string similar to Tuple element name cannot be an empty string.. + /// + internal static string TupleElementNameEmpty { + get { + return ResourceManager.GetString("TupleElementNameEmpty", resourceCulture); + } + } + /// /// Looks up a localized string similar to Tuples must have at least two elements.. /// diff --git a/src/Compilers/Core/Portable/CodeAnalysisResources.resx b/src/Compilers/Core/Portable/CodeAnalysisResources.resx index 85f65775e6f..5d027707e19 100644 --- a/src/Compilers/Core/Portable/CodeAnalysisResources.resx +++ b/src/Compilers/Core/Portable/CodeAnalysisResources.resx @@ -540,6 +540,9 @@ If tuple element names are specified, the number of element names must match the cardinality of the tuple. + + Tuple element name cannot be an empty string. + If tuple element locations are specified, the number of locations must match the cardinality of the tuple. diff --git a/src/Compilers/Core/Portable/Compilation/Compilation.cs b/src/Compilers/Core/Portable/Compilation/Compilation.cs index 86c65f41a8c..c3c92ce4f06 100644 --- a/src/Compilers/Core/Portable/Compilation/Compilation.cs +++ b/src/Compilers/Core/Portable/Compilation/Compilation.cs @@ -878,6 +878,14 @@ protected static ImmutableArray CheckTupleElementNames(int cardinality, throw new ArgumentException(CodeAnalysisResources.TupleElementNameCountMismatch, nameof(elementNames)); } + for (int i = 0; i < elementNames.Length; i++) + { + if (elementNames[i] == "") + { + throw new ArgumentException(CodeAnalysisResources.TupleElementNameEmpty, $"{nameof(elementNames)}[{i}]"); + } + } + if (elementNames.All(n => n == null)) { return default(ImmutableArray); diff --git a/src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs b/src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs index bf2b570a235..e61c93f1e61 100644 --- a/src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs +++ b/src/Compilers/Core/Portable/Symbols/ISymbolExtensions.cs @@ -58,5 +58,35 @@ public static IMethodSymbol GetConstructedReducedFrom(this IMethodSymbol method) return reducedFrom.Construct(typeArgs); } + + /// + /// Returns true if a given field is a nondefault tuple element + /// + internal static bool IsDefaultTupleElement(this IFieldSymbol field) + { + return (object)field == field.CorrespondingTupleField; + } + + /// + /// Returns true if a given field is a tuple element + /// + internal static bool IsTupleElement(this IFieldSymbol field) + { + return (object)field.CorrespondingTupleField != null; + } + + /// + /// Return the name of the field if the field is an explicitly named tuple element. + /// Otherwise returns null. + /// + /// + /// Note that it is possible for an element to be both "Default" and to have a user provided name. + /// That could happen if the provided name matches the default name such as "Item10" + /// + internal static string ProvidedTupleElementNameOrNull(this IFieldSymbol field) + { + return field.IsTupleElement() && !field.IsImplicitlyDeclared ? field.Name : null; + } + } } diff --git a/src/Compilers/VisualBasic/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.vb b/src/Compilers/VisualBasic/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.vb index 40da56a9420..ae84ec68f17 100644 --- a/src/Compilers/VisualBasic/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.vb +++ b/src/Compilers/VisualBasic/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.vb @@ -322,7 +322,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic End Function Private Shared Function HasNonDefaultTupleElements(tupleSymbol As INamedTypeSymbol) As Boolean - Return tupleSymbol.TupleElements.Any(Function(e) e IsNot e.CorrespondingTupleField) + Return tupleSymbol.TupleElements.Any(Function(e) Not e.IsDefaultTupleElement) End Function Private Sub AddTupleTypeName(symbol As INamedTypeSymbol) diff --git a/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleErrorFieldSymbol.vb b/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleErrorFieldSymbol.vb index f80d979dd26..4e620f36104 100644 --- a/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleErrorFieldSymbol.vb +++ b/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleErrorFieldSymbol.vb @@ -47,7 +47,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols Me._tupleElementIndex = If(correspondingDefaultFieldOpt Is Nothing, tupleElementIndex << 1, (tupleElementIndex << 1) + 1) Me._isImplicitlyDeclared = isImplicitlyDeclared - Debug.Assert(correspondingDefaultFieldOpt IsNot Nothing OrElse Me.IsDefaultTupleElement) + Debug.Assert(correspondingDefaultFieldOpt Is Nothing = Me.IsDefaultTupleElement) Debug.Assert(correspondingDefaultFieldOpt Is Nothing OrElse correspondingDefaultFieldOpt.IsDefaultTupleElement) _correspondingDefaultField = If(correspondingDefaultFieldOpt, Me) @@ -107,6 +107,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols End Get End Property + Public Overrides ReadOnly Property CorrespondingTupleField As FieldSymbol + Get + Return _correspondingDefaultField + End Get + End Property + Public Overrides ReadOnly Property Type As TypeSymbol Get Return Me._type diff --git a/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleFieldSymbol.vb b/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleFieldSymbol.vb index 0b2386a733d..ca700628d13 100644 --- a/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleFieldSymbol.vb +++ b/src/Compilers/VisualBasic/Portable/Symbols/Tuples/TupleFieldSymbol.vb @@ -143,7 +143,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols Me._locations = If((location Is Nothing), ImmutableArray(Of Location).Empty, ImmutableArray.Create(Of Location)(location)) Me._isImplicitlyDeclared = isImplicitlyDeclared - Debug.Assert(correspondingDefaultFieldOpt IsNot Nothing OrElse Me.IsDefaultTupleElement) + Debug.Assert(correspondingDefaultFieldOpt Is Nothing = Me.IsDefaultTupleElement) Debug.Assert(correspondingDefaultFieldOpt Is Nothing OrElse correspondingDefaultFieldOpt.IsDefaultTupleElement) _correspondingDefaultField = If(correspondingDefaultFieldOpt, Me) diff --git a/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb b/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb index 1e7d9fe3d14..fc7a82b8246 100644 --- a/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb +++ b/src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb @@ -6496,10 +6496,11 @@ End Class Dim intType As NamedTypeSymbol = comp.GetSpecialType(SpecialType.System_Int32) Dim vt2 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).Construct(intType, intType) + Dim vt3 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T3).Construct(intType, intType, intType) - ' Illegal VB identifier and blank - Dim tuple2 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("123", " ")) - Assert.Equal({"123", " "}, GetTupleElementNames(tuple2)) + ' Illegal VB identifier, space and null + Dim tuple2 = comp.CreateTupleTypeSymbol(vt3, ImmutableArray.Create("123", " ", Nothing)) + Assert.Equal({"123", " ", Nothing}, GetTupleElementNames(tuple2)) ' Reserved keywords Dim tuple3 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("return", "class")) @@ -6514,6 +6515,25 @@ End Class End Sub + + Public Sub CreateTupleTypeSymbol_EmptyNames() + + Dim comp = VisualBasicCompilation.Create("test", references:={MscorlibRef}) + + Dim intType As NamedTypeSymbol = comp.GetSpecialType(SpecialType.System_Int32) + Dim vt2 = comp.GetWellKnownType(WellKnownType.System_ValueTuple_T2).Construct(intType, intType) + + Try + ' Illegal VB identifier and empty + Dim tuple2 = comp.CreateTupleTypeSymbol(vt2, ImmutableArray.Create("123", "")) + Assert.True(False) + Catch ex As ArgumentException + Assert.Contains(CodeAnalysisResources.TupleElementNameEmpty, ex.Message) + Assert.Contains("1", ex.Message) + End Try + + End Sub + Public Sub CreateTupleTypeSymbol_CSharpElements() @@ -6787,7 +6807,7 @@ End Class Return Nothing End If - Return elements.SelectAsArray(Function(e) If(e.IsImplicitlyDeclared, Nothing, e.Name)) + Return elements.SelectAsArray(Function(e) e.ProvidedTupleElementNameOrNull) End Function diff --git a/src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.GetHashCodeVisitor.cs b/src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.GetHashCodeVisitor.cs index 3542210c4a7..e087abac5b8 100644 --- a/src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.GetHashCodeVisitor.cs +++ b/src/Workspaces/Core/Portable/Shared/Utilities/SymbolEquivalenceComparer.GetHashCodeVisitor.cs @@ -189,12 +189,7 @@ private int CombineNamedTypeHashCode(INamedTypeSymbol x, int currentHash) { if (x.IsTupleType) { - foreach(var element in x.TupleElements) - { - currentHash = Hash.Combine(element.Type, currentHash); - } - - return currentHash; + return Hash.Combine(currentHash, Hash.CombineValues(x.TupleElements)); } // If we want object and dynamic to be the same, and this is 'object', then return diff --git a/src/Workspaces/Core/Portable/SymbolKey/SymbolKey.TupleTypeSymbolKey.cs b/src/Workspaces/Core/Portable/SymbolKey/SymbolKey.TupleTypeSymbolKey.cs index b4ebe938c8d..484a6ff21e3 100644 --- a/src/Workspaces/Core/Portable/SymbolKey/SymbolKey.TupleTypeSymbolKey.cs +++ b/src/Workspaces/Core/Portable/SymbolKey/SymbolKey.TupleTypeSymbolKey.cs @@ -15,16 +15,16 @@ public static void Create(INamedTypeSymbol symbol, SymbolKeyWriter visitor) Debug.Assert(symbol.IsTupleType); visitor.WriteSymbolKey(symbol.TupleUnderlyingType); - var frieldlyNames = ArrayBuilder.GetInstance(); + var friendlyNames = ArrayBuilder.GetInstance(); var locations = ArrayBuilder.GetInstance(); - foreach(var element in symbol.TupleElements) + foreach (var element in symbol.TupleElements) { - frieldlyNames.Add(element.IsImplicitlyDeclared? null: element.Name); + friendlyNames.Add(element.IsImplicitlyDeclared ? null : element.Name); locations.Add(element.Locations.FirstOrDefault()); } - visitor.WriteStringArray(frieldlyNames.ToImmutableAndFree()); + visitor.WriteStringArray(friendlyNames.ToImmutableAndFree()); visitor.WriteLocationArray(locations.ToImmutableAndFree()); } -- GitLab