From 64007b6938472970e638f2d62a46cd6aec694346 Mon Sep 17 00:00:00 2001 From: Andy Gocke Date: Wed, 10 Jul 2019 18:08:40 -0700 Subject: [PATCH] Fix crash in semantic model when using unmanaged constraint (#37022) The semantic model often creates ReducedExtensionMethods for the public API, but the creation of ReducedExtensionMethod does constraint checking, which requires a compilation, which isn't always available. This change removes constraint checking from all creation of ReducedExtensionMethod and adds asserts that we always have a compilation during constraint checking. Fixes #37021 --- .../Portable/Binder/Binder_Expressions.cs | 15 +- .../Portable/Symbols/ConstraintsHelper.cs | 5 + .../CSharp/Portable/Symbols/MethodSymbol.cs | 5 +- .../Symbols/MethodSymbolExtensions.cs | 138 ------------------ .../Symbols/ReducedExtensionMethodSymbol.cs | 77 +++++++++- .../Test/Semantic/Semantics/NameOfTests.cs | 8 +- .../Compilation/SemanticModelAPITests.cs | 55 +++++++ .../SemanticModelGetDeclaredSymbolAPITests.cs | 4 +- .../Symbol/Symbols/ExtensionMethodTests.cs | 10 +- 9 files changed, 150 insertions(+), 167 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs index 61b4ca83eae..32c4e36e558 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @@ -6286,20 +6286,9 @@ private static void CombineExtensionMethodArguments(BoundExpression receiver, An // Arguments will be null if the caller is resolving to the first method group that can accept // that receiver, regardless of arguments, when the signature cannot // be inferred. (In the case of nameof(o.M) or the error case of o.M = null; for instance.) - if (analyzedArguments == null) + if (analyzedArguments == null && methodGroup.Methods.Count != 0) { - if (expression == EnclosingNameofArgument) - { - for (int i = methodGroup.Methods.Count - 1; i >= 0; i--) - { - if ((object)methodGroup.Methods[i].ReduceExtensionMethod(left.Type) == null) methodGroup.Methods.RemoveAt(i); - } - } - - if (methodGroup.Methods.Count != 0) - { - return new MethodGroupResolution(methodGroup, diagnostics.ToReadOnlyAndFree()); - } + return new MethodGroupResolution(methodGroup, diagnostics.ToReadOnlyAndFree()); } if (methodGroup.Methods.Count == 0) diff --git a/src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs b/src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs index e0f4bf16e27..eb19fb1acac 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ConstraintsHelper.cs @@ -511,6 +511,8 @@ void populateDiagnosticsAndClear(ArrayBuilder build { Debug.Assert(!type.IsTupleType); Debug.Assert(typeArgumentsSyntax.Count == 0 /*omitted*/ || typeArgumentsSyntax.Count == type.Arity); + Debug.Assert(currentCompilation is object); + if (!RequiresChecking(type)) { return true; @@ -551,6 +553,8 @@ void populateDiagnosticsAndClear(ArrayBuilder build Location location, DiagnosticBag diagnostics) { + Debug.Assert(currentCompilation is object); + // We do not report element locations in method parameters and return types // so we will simply unwrap the type if it was a tuple. We are relying on // TypeSymbolExtensions.VisitType to dig into the "Rest" tuple so that they @@ -817,6 +821,7 @@ private static bool HasDuplicateInterfaces(NamedTypeSymbol type, ConsList ignoreTypeConstraintsDependentOnTypeParametersOpt) { Debug.Assert(substitution != null); + Debug.Assert(currentCompilation is object); // The type parameters must be original definitions of type parameters from the containing symbol. Debug.Assert(ReferenceEquals(typeParameter.ContainingSymbol, containingSymbol.OriginalDefinition)); diff --git a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs index aaad2c469da..e933e12113a 100644 --- a/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/MethodSymbol.cs @@ -688,10 +688,7 @@ public MethodSymbol ReduceExtensionMethod(TypeSymbol receiverType) return null; } - // To give optimal diagnostics, we should really pass the "current" compilation. - // However, this is never used in batch scenarios, so it doesn't matter - // (modulo future changes to the API). - return ReducedExtensionMethodSymbol.Create(this, receiverType, compilation: null); + return ReducedExtensionMethodSymbol.Create(this, receiverType); } /// diff --git a/src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs b/src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs index c6fc6e91a2e..74b20620653 100644 --- a/src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs +++ b/src/Compilers/CSharp/Portable/Symbols/MethodSymbolExtensions.cs @@ -17,144 +17,6 @@ public static bool IsParams(this MethodSymbol method) return method.ParameterCount != 0 && method.Parameters[method.ParameterCount - 1].IsParams; } - /// - /// If the extension method is applicable based on the "this" argument type, return - /// the method constructed with the inferred type arguments. If the method is not an - /// unconstructed generic method, type inference is skipped. If the method is not - /// applicable, or if constraints when inferring type parameters from the "this" type - /// are not satisfied, the return value is null. - /// - public static MethodSymbol InferExtensionMethodTypeArguments(this MethodSymbol method, TypeSymbol thisType, Compilation compilation, ref HashSet useSiteDiagnostics) - { - Debug.Assert(method.IsExtensionMethod); - Debug.Assert((object)thisType != null); - - if (!method.IsGenericMethod || method != method.ConstructedFrom) - { - return method; - } - - // We never resolve extension methods on a dynamic receiver. - if (thisType.IsDynamic()) - { - return null; - } - - var containingAssembly = method.ContainingAssembly; - var errorNamespace = containingAssembly.GlobalNamespace; - var conversions = new TypeConversions(containingAssembly.CorLibrary); - - // There is absolutely no plausible syntax/tree that we could use for these - // synthesized literals. We could be speculatively binding a call to a PE method. - var syntaxTree = CSharpSyntaxTree.Dummy; - var syntax = (CSharpSyntaxNode)syntaxTree.GetRoot(); - - // Create an argument value for the "this" argument of specific type, - // and pass the same bad argument value for all other arguments. - var thisArgumentValue = new BoundLiteral(syntax, ConstantValue.Bad, thisType) { WasCompilerGenerated = true }; - var otherArgumentType = new ExtendedErrorTypeSymbol(errorNamespace, name: string.Empty, arity: 0, errorInfo: null, unreported: false); - var otherArgumentValue = new BoundLiteral(syntax, ConstantValue.Bad, otherArgumentType) { WasCompilerGenerated = true }; - - var paramCount = method.ParameterCount; - var arguments = new BoundExpression[paramCount]; - - for (int i = 0; i < paramCount; i++) - { - var argument = (i == 0) ? thisArgumentValue : otherArgumentValue; - arguments[i] = argument; - } - - var typeArgs = MethodTypeInferrer.InferTypeArgumentsFromFirstArgument( - conversions, - method, - arguments.AsImmutable(), - useSiteDiagnostics: ref useSiteDiagnostics); - - if (typeArgs.IsDefault) - { - return null; - } - - int firstNullInTypeArgs = -1; - - // For the purpose of constraint checks we use error type symbol in place of type arguments that we couldn't infer from the first argument. - // This prevents constraint checking from failing for corresponding type parameters. - var notInferredTypeParameters = PooledHashSet.GetInstance(); - var typeParams = method.TypeParameters; - var typeArgsForConstraintsCheck = typeArgs; - for (int i = 0; i < typeArgsForConstraintsCheck.Length; i++) - { - if (!typeArgsForConstraintsCheck[i].HasType) - { - firstNullInTypeArgs = i; - var builder = ArrayBuilder.GetInstance(); - builder.AddRange(typeArgsForConstraintsCheck, firstNullInTypeArgs); - - for (; i < typeArgsForConstraintsCheck.Length; i++) - { - var typeArg = typeArgsForConstraintsCheck[i]; - if (!typeArg.HasType) - { - notInferredTypeParameters.Add(typeParams[i]); - builder.Add(TypeWithAnnotations.Create(ErrorTypeSymbol.UnknownResultType)); - } - else - { - builder.Add(typeArg); - } - } - - typeArgsForConstraintsCheck = builder.ToImmutableAndFree(); - break; - } - } - - // Check constraints. - var diagnosticsBuilder = ArrayBuilder.GetInstance(); - var substitution = new TypeMap(typeParams, typeArgsForConstraintsCheck); - ArrayBuilder useSiteDiagnosticsBuilder = null; - var success = method.CheckConstraints(conversions, includeNullability: false, substitution, typeParams, typeArgsForConstraintsCheck, compilation, diagnosticsBuilder, nullabilityDiagnosticsBuilderOpt: null, ref useSiteDiagnosticsBuilder, - ignoreTypeConstraintsDependentOnTypeParametersOpt: notInferredTypeParameters.Count > 0 ? notInferredTypeParameters : null); - diagnosticsBuilder.Free(); - notInferredTypeParameters.Free(); - - if (useSiteDiagnosticsBuilder != null && useSiteDiagnosticsBuilder.Count > 0) - { - if (useSiteDiagnostics == null) - { - useSiteDiagnostics = new HashSet(); - } - - foreach (var diag in useSiteDiagnosticsBuilder) - { - useSiteDiagnostics.Add(diag.DiagnosticInfo); - } - } - - if (!success) - { - return null; - } - - // For the purpose of construction we use original type parameters in place of type arguments that we couldn't infer from the first argument. - var typeArgsForConstruct = typeArgs; - if (firstNullInTypeArgs != -1) - { - var builder = ArrayBuilder.GetInstance(); - builder.AddRange(typeArgs, firstNullInTypeArgs); - - for (int i = firstNullInTypeArgs; i < typeArgsForConstruct.Length; i++) - { - var typeArgForConstruct = typeArgsForConstruct[i]; - builder.Add(typeArgForConstruct.HasType ? typeArgForConstruct : TypeWithAnnotations.Create(typeParams[i])); - } - - typeArgsForConstruct = builder.ToImmutableAndFree(); - } - - return method.Construct(typeArgsForConstruct); - } - internal static bool IsSynthesizedLambda(this MethodSymbol method) { Debug.Assert((object)method != null); diff --git a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs index f899c2f333f..ac029e0840f 100644 --- a/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs +++ b/src/Compilers/CSharp/Portable/Symbols/ReducedExtensionMethodSymbol.cs @@ -4,9 +4,11 @@ using System.Collections.Immutable; using System.Diagnostics; using System.Globalization; +using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.CSharp.Symbols; using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Text; using Roslyn.Utilities; @@ -29,7 +31,7 @@ internal sealed class ReducedExtensionMethodSymbol : MethodSymbol /// is applicable, and satisfies type parameter constraints, based on the /// "this" argument type. Otherwise, returns null. /// - public static MethodSymbol Create(MethodSymbol method, TypeSymbol receiverType, Compilation compilation) + public static MethodSymbol Create(MethodSymbol method, TypeSymbol receiverType) { Debug.Assert(method.IsExtensionMethod && method.MethodKind != MethodKind.ReducedExtension); Debug.Assert(method.ParameterCount > 0); @@ -37,7 +39,7 @@ public static MethodSymbol Create(MethodSymbol method, TypeSymbol receiverType, HashSet useSiteDiagnostics = null; - method = method.InferExtensionMethodTypeArguments(receiverType, compilation, ref useSiteDiagnostics); + method = InferExtensionMethodTypeArguments(method, receiverType, ref useSiteDiagnostics); if ((object)method == null) { return null; @@ -96,6 +98,77 @@ private ReducedExtensionMethodSymbol(MethodSymbol reducedFrom) _typeArguments = _typeMap.SubstituteTypes(reducedFrom.TypeArgumentsWithAnnotations); } + /// + /// If the extension method is applicable based on the "this" argument type, return + /// the method constructed with the inferred type arguments. If the method is not an + /// unconstructed generic method, type inference is skipped. If the method is not + /// applicable, or if constraints when inferring type parameters from the "this" type + /// are not satisfied, the return value is null. + /// + private static MethodSymbol InferExtensionMethodTypeArguments(MethodSymbol method, TypeSymbol thisType, ref HashSet useSiteDiagnostics) + { + Debug.Assert(method.IsExtensionMethod); + Debug.Assert((object)thisType != null); + + if (!method.IsGenericMethod || method != method.ConstructedFrom) + { + return method; + } + + // We never resolve extension methods on a dynamic receiver. + if (thisType.IsDynamic()) + { + return null; + } + + var containingAssembly = method.ContainingAssembly; + var errorNamespace = containingAssembly.GlobalNamespace; + var conversions = new TypeConversions(containingAssembly.CorLibrary); + + // There is absolutely no plausible syntax/tree that we could use for these + // synthesized literals. We could be speculatively binding a call to a PE method. + var syntaxTree = CSharpSyntaxTree.Dummy; + var syntax = (CSharpSyntaxNode)syntaxTree.GetRoot(); + + // Create an argument value for the "this" argument of specific type, + // and pass the same bad argument value for all other arguments. + var thisArgumentValue = new BoundLiteral(syntax, ConstantValue.Bad, thisType) { WasCompilerGenerated = true }; + var otherArgumentType = new ExtendedErrorTypeSymbol(errorNamespace, name: string.Empty, arity: 0, errorInfo: null, unreported: false); + var otherArgumentValue = new BoundLiteral(syntax, ConstantValue.Bad, otherArgumentType) { WasCompilerGenerated = true }; + + var paramCount = method.ParameterCount; + var arguments = new BoundExpression[paramCount]; + + for (int i = 0; i < paramCount; i++) + { + var argument = (i == 0) ? thisArgumentValue : otherArgumentValue; + arguments[i] = argument; + } + + var typeArgs = MethodTypeInferrer.InferTypeArgumentsFromFirstArgument( + conversions, + method, + arguments.AsImmutable(), + useSiteDiagnostics: ref useSiteDiagnostics); + + if (typeArgs.IsDefault) + { + return null; + } + + // For the purpose of construction we use original type parameters in place of type arguments that we couldn't infer from the first argument. + ImmutableArray typeArgsForConstruct = typeArgs; + if (typeArgs.Any(t => !t.HasType)) + { + typeArgsForConstruct = typeArgs.ZipAsArray( + method.TypeParameters, + (t, tp) => t.HasType ? t : TypeWithAnnotations.Create(tp)); + } + + return method.Construct(typeArgsForConstruct); + } + + internal override MethodSymbol CallsiteReducedFromMethod { get { return _reducedFrom.ConstructIfGeneric(_typeArguments); } diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs index 64232fef719..dcdab498c93 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NameOfTests.cs @@ -842,9 +842,9 @@ public static void Main(string[] args) }"; var compilation = CreateCompilationWithMscorlib40AndSystemCore(source); compilation.VerifyDiagnostics( - // (14,22): error CS1061: 'A' does not contain a definition for 'Extension' and no extension method 'Extension' accepting a first argument of type 'A' could be found (are you missing a using directive or an assembly reference?) + // (14,20): error CS8093: Extension method groups are not allowed as an argument to 'nameof'. // Use(nameof(a.Extension)); - Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "Extension").WithArguments("A", "Extension").WithLocation(14, 22) + Diagnostic(ErrorCode.ERR_NameofExtensionMethod, "a.Extension").WithLocation(14, 20) ); var tree = compilation.SyntaxTrees[0]; var model = compilation.GetSemanticModel(tree); @@ -996,9 +996,9 @@ public static void Main(string[] args) "; var compilation = CreateCompilationWithMscorlib40AndSystemCore(source); compilation.VerifyDiagnostics( - // (16,22): error CS1061: 'A' does not contain a definition for 'Extension' and no extension method 'Extension' accepting a first argument of type 'A' could be found (are you missing a using directive or an assembly reference?) + // (16,20): error CS8093: Extension method groups are not allowed as an argument to 'nameof'. // Use(nameof(a.Extension)); - Diagnostic(ErrorCode.ERR_NoSuchMemberOrExtension, "Extension").WithArguments("A", "Extension").WithLocation(16, 22) + Diagnostic(ErrorCode.ERR_NameofExtensionMethod, "a.Extension").WithLocation(16, 20) ); } diff --git a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs index 1c2c89cfd4a..1d01aa5ef39 100644 --- a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs @@ -16,6 +16,61 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests { public class SemanticTests : CSharpTestBase { + [Fact] + public void UnmanagedConstraintOnExtensionMethod() + { + var src = @" +public static class Ext +{ + public static T GenericExtension(ref this T self) where T : unmanaged + { + return self; + } +} +"; + var src2 = @" +public class Program +{ + public struct S where T : unmanaged + { + public T t; + } + + static void M(S s) where T : unmanaged + { + s.GenericExtension(); + } +}"; + var comp1 = CreateCompilation(src, parseOptions: TestOptions.Regular8); + + var comp2 = CreateCompilation(src2, parseOptions: TestOptions.Regular8, + references: new[] { comp1.ToMetadataReference() }); + comp2.VerifyDiagnostics(); + Assert.NotNull(checkSymbolInfo(comp2).Symbol); + + comp2 = CreateCompilation(src2, parseOptions: TestOptions.Regular7_3, + references: new[] { comp1.ToMetadataReference() }); + comp2.VerifyDiagnostics( + // (11,11): error CS8652: The feature 'unmanaged constructed types' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version. + // s.GenericExtension(); + Diagnostic(ErrorCode.ERR_FeatureInPreview, "GenericExtension").WithArguments("unmanaged constructed types").WithLocation(11, 11)); + + var info = checkSymbolInfo(comp2); + Assert.Null(info.Symbol); + Assert.Equal(CandidateReason.OverloadResolutionFailure, info.CandidateReason); + + static SymbolInfo checkSymbolInfo(CSharpCompilation comp) + { + SyntaxTree tree = comp.SyntaxTrees[0]; + var model = comp.GetSemanticModel(tree); + SyntaxNode root = tree.GetRoot(); + var invoke = root.DescendantNodes().OfType() + .Where(e => e.Name.ToString() == "GenericExtension").First(); + + return model.GetSymbolInfo(invoke); + } + } + [Fact] public void PatternIndexAndRangeIndexers() { diff --git a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs index 89e71bf0dec..b899cc506ac 100644 --- a/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs @@ -1938,7 +1938,7 @@ static void M(A a, B b) // Type not satisfying constraint. symbols = model.LookupSymbols(position, container: method.Parameters[1].Type, name: "F", includeReducedExtensionMethods: true); - CheckSymbolsUnordered(symbols); + CheckSymbolsUnordered(symbols, "void B.F()"); // Same tests as above but with position outside of // static class defining extension methods. @@ -1978,7 +1978,7 @@ static class E // Type not satisfying constraint. symbols = model.LookupSymbols(position, container: method.Parameters[1].Type, name: "F", includeReducedExtensionMethods: true); - CheckSymbols(symbols); + CheckSymbolsUnordered(symbols, "void B.F()"); } [Fact] diff --git a/src/Compilers/CSharp/Test/Symbol/Symbols/ExtensionMethodTests.cs b/src/Compilers/CSharp/Test/Symbol/Symbols/ExtensionMethodTests.cs index 4d83e0cf587..f422add9bb7 100644 --- a/src/Compilers/CSharp/Test/Symbol/Symbols/ExtensionMethodTests.cs +++ b/src/Compilers/CSharp/Test/Symbol/Symbols/ExtensionMethodTests.cs @@ -3831,8 +3831,9 @@ public class BaseClass var baseClass = model.GetTypeInfo(instance).Type; Assert.Equal("BaseClass", baseClass.ToTestDisplayString()); - Assert.Empty(model.LookupSymbols(instance.Position, baseClass, "SetMember", includeReducedExtensionMethods: true)); - Assert.Empty(model.LookupSymbols(instance.Position, baseClass, includeReducedExtensionMethods: true).Where(s => s.Name == "SetMembers")); + var setMember = model.LookupSymbols(instance.Position, baseClass, "SetMember", includeReducedExtensionMethods: true).Single(); + Assert.Equal("BaseClass BaseClass.SetMember, TMember>(TMember NewValue)", setMember.ToTestDisplayString()); + Assert.Contains(setMember, model.LookupSymbols(instance.Position, baseClass, includeReducedExtensionMethods: true)); } [Fact] @@ -3926,8 +3927,9 @@ public class BaseClass : I1 var baseClass = model.GetTypeInfo(instance).Type; Assert.Equal("BaseClass", baseClass.ToTestDisplayString()); - Assert.Empty(model.LookupSymbols(instance.Position, baseClass, "SetMember", includeReducedExtensionMethods: true)); - Assert.Empty(model.LookupSymbols(instance.Position, baseClass, includeReducedExtensionMethods: true).Where(s => s.Name == "SetMembers")); + var setMember = model.LookupSymbols(instance.Position, baseClass, "SetMember", includeReducedExtensionMethods: true).Single(); + Assert.Equal("BaseClass BaseClass.SetMember, TMember>(TMember NewValue)", setMember.ToTestDisplayString()); + Assert.Contains(setMember, model.LookupSymbols(instance.Position, baseClass, includeReducedExtensionMethods: true)); } [Fact] -- GitLab