From 5c7780398bdc4b08aee228b9af4f7b3ae5a3306a Mon Sep 17 00:00:00 2001 From: Jared Parsons Date: Fri, 12 Jul 2024 10:07:02 -0700 Subject: [PATCH] Use correct get / set inside ref safety analysis (#73842) This fixes a few cases where our code looked at the wrong accessor when doing ref safety analysis. closes #35606 closes #73550 related #73872 related #8253 --- .../Portable/Binder/Binder.ValueChecks.cs | 654 ++++++--- .../Portable/Binder/Binder_Expressions.cs | 4 + .../Portable/Binder/RefSafetyAnalysis.cs | 66 +- .../Portable/BoundTree/BoundExpression.cs | 24 + .../CSharp/Portable/BoundTree/BoundNodes.xml | 3 + .../CSharp/Portable/BoundTree/Constructors.cs | 9 +- .../Generated/BoundNodes.xml.Generated.cs | 35 +- ...ocalRewriter_CompoundAssignmentOperator.cs | 1 + .../LocalRewriter_IndexerAccess.cs | 48 +- ...ObjectOrCollectionInitializerExpression.cs | 5 +- .../Lowering/MethodToClassRewriter.cs | 2 +- .../Test/Emit2/Semantics/InlineArrayTests.cs | 1 + .../Emit2/Semantics/PatternMatchingTests.cs | 2 +- .../Test/Emit3/RefStructInterfacesTests.cs | 4 +- ...eAnalysis.CSharp.Semantic.UnitTests.csproj | 1 + .../Semantic/Semantics/RecordStructTests.cs | 8 +- .../Semantic/Semantics/RefEscapingTests.cs | 1256 ++++++++++++++++- .../Test/Semantic/Semantics/RefFieldTests.cs | 64 +- .../Semantics/StructConstructorTests.cs | 8 +- .../Test/Core/CompilerTraitAttribute.cs | 2 +- .../Test/Core/Traits/CompilerFeature.cs | 1 + src/Tools/Source/RunTests/TestRunner.cs | 2 + 22 files changed, 1901 insertions(+), 299 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs index ed092ad9a51e7..df47e679fd665 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs @@ -22,6 +22,72 @@ private enum EscapeLevel : uint ReturnOnly = ReturnOnlyScope, } + /// + /// Encapsulates a symbol used in ref safety analysis. For properties and indexers this + /// captures the accessor(s) on it that were used. The particular accessor used is + /// important as it can impact ref safety analysis. + /// + private readonly struct MethodInfo + { + internal Symbol Symbol { get; } + + /// + /// This is the primary used in ref safety analysis. + /// + /// + /// This will be null in error scenarios. For example when an indexer with only a set + /// method is used in a get scenario. That will lead to a non-null + /// but a null value here. + /// + internal MethodSymbol? Method { get; } + + /// + /// In the case of a compound operation on non-ref return property or indexer + /// will represent the `get` accessor and this will + /// represent the `set` accessor. + /// + internal MethodSymbol? SetMethod { get; } + + internal bool UseUpdatedEscapeRules => Method?.UseUpdatedEscapeRules == true; + internal bool ReturnsRefToRefStruct => + Method is { RefKind: not RefKind.None, ReturnType: { } returnType } && + returnType.IsRefLikeOrAllowsRefLikeType(); + + private MethodInfo(Symbol symbol, MethodSymbol? method, MethodSymbol? setMethod) + { + Symbol = symbol; + Method = method; + SetMethod = setMethod; + } + + internal static MethodInfo Create(MethodSymbol method) + { + return new MethodInfo(method, method, null); + } + + internal static MethodInfo Create(PropertySymbol property) + { + return new MethodInfo( + property, + property.GetOwnOrInheritedGetMethod() ?? property.GetOwnOrInheritedSetMethod(), + null); + } + + internal static MethodInfo Create(PropertySymbol property, AccessorKind accessorKind) => + accessorKind switch + { + AccessorKind.Get => new MethodInfo(property, property.GetOwnOrInheritedGetMethod(), setMethod: null), + AccessorKind.Set => new MethodInfo(property, property.GetOwnOrInheritedSetMethod(), setMethod: null), + AccessorKind.Both => new MethodInfo(property, property.GetOwnOrInheritedGetMethod(), property.GetOwnOrInheritedSetMethod()), + _ => throw ExceptionUtilities.UnexpectedValue(accessorKind), + }; + + internal static MethodInfo Create(BoundIndexerAccess expr) => + Create(expr.Indexer, expr.AccessorKind); + + public override string? ToString() => Method?.ToString(); + } + /// /// The destination in a method arguments must match (MAMM) check. This is /// created primarily for ref and out arguments of a ref struct. It also applies @@ -164,6 +230,11 @@ public void Deconstruct(out ParameterSymbol? parameter, out BoundExpression argu /// n + 1 corresponds to scopes immediately inside a scope of depth n. /// Since sibling scopes do not intersect and a value cannot escape from one to another without /// escaping to a wider scope, we can use simple depth numbering without ambiguity. + /// + /// Generally these values are expressed via the following parameters: + /// - escapeFrom: the scope in which an expression is being evaluated. Usually the current local + /// scope + /// - escapeTo: the scope to which the values are being escaped to. /// private const uint CallingMethodScope = 0; private const uint ReturnOnlyScope = 1; @@ -310,9 +381,27 @@ private static bool RequiresRefOrOut(BindValueKind kind) #nullable enable + private static AccessorKind GetIndexerAccessorKind(BoundIndexerAccess indexerAccess, BindValueKind valueKind) + { + if (indexerAccess.Indexer.RefKind != RefKind.None) + { + return AccessorKind.Get; + } + + var coreValueKind = valueKind & ValueKindSignificantBitsMask; + return coreValueKind switch + { + BindValueKind.CompoundAssignment => AccessorKind.Both, + BindValueKind.Assignable => AccessorKind.Set, + _ => AccessorKind.Get, + }; + } + private BoundIndexerAccess BindIndexerDefaultArgumentsAndParamsCollection(BoundIndexerAccess indexerAccess, BindValueKind valueKind, BindingDiagnosticBag diagnostics) { - var useSetAccessor = valueKind == BindValueKind.Assignable && !indexerAccess.Indexer.ReturnsByRef; + var coreValueKind = valueKind & ValueKindSignificantBitsMask; + AccessorKind accessorKind = GetIndexerAccessorKind(indexerAccess, valueKind); + var useSetAccessor = coreValueKind == BindValueKind.Assignable && indexerAccess.Indexer.RefKind != RefKind.Ref; var accessorForDefaultArguments = useSetAccessor ? indexerAccess.Indexer.GetOwnOrInheritedSetMethod() : indexerAccess.Indexer.GetOwnOrInheritedGetMethod(); @@ -385,14 +474,17 @@ private BoundIndexerAccess BindIndexerDefaultArgumentsAndParamsCollection(BoundI argumentNamesOpt, refKindsBuilderOpt?.ToImmutableOrNull() ?? default, indexerAccess.Expanded, + accessorKind, argsToParams, defaultArguments, indexerAccess.Type); refKindsBuilderOpt?.Free(); + + return indexerAccess; } - return indexerAccess; + return indexerAccess.Update(accessorKind); } #nullable disable @@ -409,10 +501,12 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind switch (expr.Kind) { case BoundKind.PropertyGroup: - expr = BindIndexedPropertyAccess((BoundPropertyGroup)expr, mustHaveAllOptionalParameters: false, diagnostics: diagnostics); - if (expr is BoundIndexerAccess indexerAccess) { - expr = BindIndexerDefaultArgumentsAndParamsCollection(indexerAccess, valueKind, diagnostics); + expr = BindIndexedPropertyAccess((BoundPropertyGroup)expr, mustHaveAllOptionalParameters: false, diagnostics: diagnostics); + if (expr is BoundIndexerAccess indexerAccess) + { + expr = BindIndexerDefaultArgumentsAndParamsCollection(indexerAccess, valueKind, diagnostics); + } } break; @@ -433,6 +527,24 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind expr = BindIndexerDefaultArgumentsAndParamsCollection((BoundIndexerAccess)expr, valueKind, diagnostics); break; + case BoundKind.ImplicitIndexerAccess: + { + var implicitIndexer = (BoundImplicitIndexerAccess)expr; + if (implicitIndexer.IndexerOrSliceAccess is BoundIndexerAccess indexerAccess) + { + var kind = GetIndexerAccessorKind(indexerAccess, valueKind); + expr = implicitIndexer.Update( + implicitIndexer.Receiver, + implicitIndexer.Argument, + implicitIndexer.LengthOrCountAccess, + implicitIndexer.ReceiverPlaceholder, + indexerAccess.Update(kind), + implicitIndexer.ArgumentPlaceholders, + implicitIndexer.Type); + } + } + break; + case BoundKind.UnconvertedObjectCreationExpression: if (valueKind == BindValueKind.RValue) { @@ -1811,7 +1923,7 @@ internal uint GetInterpolatedStringHandlerConversionEscapeScope( /// local variables declared at the scope of the invocation. /// private uint GetInvocationEscapeScope( - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -1826,9 +1938,9 @@ bool isRefEscape Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt)); #endif - if (UseUpdatedEscapeRulesForInvocation(symbol)) + if (methodInfo.UseUpdatedEscapeRules) { - return GetInvocationEscapeWithUpdatedRules(symbol, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, scopeOfTheContainingExpression, isRefEscape); + return GetInvocationEscapeWithUpdatedRules(methodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, scopeOfTheContainingExpression, isRefEscape); } // SPEC: (also applies to the CheckInvocationEscape counterpart) @@ -1843,19 +1955,12 @@ bool isRefEscape //• the safe-to-escape of all argument expressions(including the receiver) // - if (!symbol.RequiresInstanceReceiver()) - { - // ignore receiver when symbol is static - receiver = null; - } - - //by default it is safe to escape uint escapeScope = CallingMethodScope; - - var escapeArguments = ArrayBuilder.GetInstance(); - GetInvocationArgumentsForEscape( - symbol, - receiver: null, // receiver handled explicitly below + var escapeValues = ArrayBuilder.GetInstance(); + GetEscapeValuesForOldRules( + in methodInfo, + // Receiver handled explicitly below + receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, parameters, argsOpt, @@ -1865,11 +1970,11 @@ bool isRefEscape // __refvalue is not ref-returnable, so ref varargs can't come back from a call ignoreArglistRefKinds: true, mixableArguments: null, - escapeArguments); + escapeValues); try { - foreach (var (parameter, argument, effectiveRefKind) in escapeArguments) + foreach (var (parameter, argument, _, argumentIsRefEscape) in escapeValues) { // ref escape scope is the narrowest of // - ref escape of all byref arguments @@ -1877,13 +1982,14 @@ bool isRefEscape // // val escape scope is the narrowest of // - val escape of all byval arguments (refs cannot be wrapped into values, so their ref escape is irrelevant, only use val escapes) + uint argumentEscape = (isRefEscape, argumentIsRefEscape) switch + { + (true, true) => GetRefEscape(argument, scopeOfTheContainingExpression), + (false, false) => GetValEscape(argument, scopeOfTheContainingExpression), + _ => escapeScope + }; - var argEscape = effectiveRefKind != RefKind.None && isRefEscape ? - GetRefEscape(argument, scopeOfTheContainingExpression) : - GetValEscape(argument, scopeOfTheContainingExpression); - - escapeScope = Math.Max(escapeScope, argEscape); - + escapeScope = Math.Max(escapeScope, argumentEscape); if (escapeScope >= scopeOfTheContainingExpression) { // can't get any worse @@ -1893,11 +1999,11 @@ bool isRefEscape } finally { - escapeArguments.Free(); + escapeValues.Free(); } // check receiver if ref-like - if (receiver?.Type?.IsRefLikeOrAllowsRefLikeType() == true) + if (methodInfo.Method?.RequiresInstanceReceiver == true && receiver?.Type?.IsRefLikeOrAllowsRefLikeType() == true) { escapeScope = Math.Max(escapeScope, GetValEscape(receiver, scopeOfTheContainingExpression)); } @@ -1906,7 +2012,7 @@ bool isRefEscape } private uint GetInvocationEscapeWithUpdatedRules( - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -1921,7 +2027,7 @@ private uint GetInvocationEscapeWithUpdatedRules( var argsAndParamsAll = ArrayBuilder.GetInstance(); GetFilteredInvocationArgumentsForEscapeWithUpdatedRules( - symbol, + methodInfo, receiver, receiverIsSubjectToCloning, parameters, @@ -1932,7 +2038,7 @@ private uint GetInvocationEscapeWithUpdatedRules( ignoreArglistRefKinds: true, // https://github.com/dotnet/roslyn/issues/63325: for compatibility with C#10 implementation. argsAndParamsAll); - var returnsRefToRefStruct = ReturnsRefToRefStruct(symbol); + var returnsRefToRefStruct = methodInfo.ReturnsRefToRefStruct; foreach (var (param, argument, _, isArgumentRefEscape) in argsAndParamsAll) { // SPEC: @@ -1961,21 +2067,6 @@ private uint GetInvocationEscapeWithUpdatedRules( return escapeScope; } - private static bool ReturnsRefToRefStruct(Symbol symbol) - { - var method = symbol switch - { - MethodSymbol m => m, - // We are only getting the method in order to handle a special condition where the method returns by-ref. - // It is an error for a property to have a setter and return by-ref, so we only bother looking for a getter here. - PropertySymbol p => p.GetMethod, - _ => null - }; - - return method is { RefKind: not RefKind.None, ReturnType: { } returnType } && - returnType.IsRefLikeOrAllowsRefLikeType(); - } - /// /// Validates whether given invocation can allow its results to escape from level to level. /// The result indicates whether the escape is possible. @@ -1986,7 +2077,7 @@ private static bool ReturnsRefToRefStruct(Symbol symbol) /// private bool CheckInvocationEscape( SyntaxNode syntax, - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2004,9 +2095,9 @@ bool isRefEscape Debug.Assert(AllParametersConsideredInEscapeAnalysisHaveArguments(argsOpt, parameters, argsToParamsOpt)); #endif - if (UseUpdatedEscapeRulesForInvocation(symbol)) + if (methodInfo.UseUpdatedEscapeRules) { - return CheckInvocationEscapeWithUpdatedRules(syntax, symbol, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, checkingReceiver, escapeFrom, escapeTo, diagnostics, isRefEscape); + return CheckInvocationEscapeWithUpdatedRules(syntax, methodInfo, receiver, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, checkingReceiver, escapeFrom, escapeTo, diagnostics, isRefEscape); } // SPEC: @@ -2015,6 +2106,7 @@ bool isRefEscape // o no ref or out argument(excluding the receiver and arguments of ref-like types) may have a narrower ref-safe-to-escape than E1; and // o no argument(including the receiver) may have a narrower safe-to-escape than E1. + var symbol = methodInfo.Symbol; if (!symbol.RequiresInstanceReceiver()) { // ignore receiver when symbol is static @@ -2023,7 +2115,7 @@ bool isRefEscape var escapeArguments = ArrayBuilder.GetInstance(); GetInvocationArgumentsForEscape( - symbol, + methodInfo, receiver: null, // receiver handled explicitly below receiverIsSubjectToCloning: ThreeState.Unknown, parameters, @@ -2078,7 +2170,7 @@ bool isRefEscape private bool CheckInvocationEscapeWithUpdatedRules( SyntaxNode syntax, - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2095,7 +2187,7 @@ private bool CheckInvocationEscapeWithUpdatedRules( var argsAndParamsAll = ArrayBuilder.GetInstance(); GetFilteredInvocationArgumentsForEscapeWithUpdatedRules( - symbol, + methodInfo, receiver, receiverIsSubjectToCloning, parameters, @@ -2106,7 +2198,8 @@ private bool CheckInvocationEscapeWithUpdatedRules( ignoreArglistRefKinds: true, // https://github.com/dotnet/roslyn/issues/63325: for compatibility with C#10 implementation. argsAndParamsAll); - var returnsRefToRefStruct = ReturnsRefToRefStruct(symbol); + var symbol = methodInfo.Symbol; + var returnsRefToRefStruct = methodInfo.ReturnsRefToRefStruct; foreach (var (param, argument, _, isArgumentRefEscape) in argsAndParamsAll) { // SPEC: @@ -2142,14 +2235,15 @@ private bool CheckInvocationEscapeWithUpdatedRules( } /// - /// Returns the set of arguments to be considered for escape analysis of a method invocation. - /// Each argument is returned with the corresponding parameter and ref kind. Arguments are not - /// filtered - all arguments are included exactly once in the array, and the caller is responsible for - /// determining which arguments affect escape analysis. This method is used for method invocation - /// analysis, regardless of whether UseUpdatedEscapeRules is set. + /// Returns the set of arguments to be considered for escape analysis of a method invocation. This + /// set potentially includes the receiver of the method call. Each argument is returned (only once) + /// with the corresponding parameter and ref kind. + /// + /// No filtering like removing non-reflike types is done by this method. It is the responsibility of + /// the caller to determine which arguments impact escape analysis. /// private void GetInvocationArgumentsForEscape( - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2162,15 +2256,9 @@ private void GetInvocationArgumentsForEscape( { if (receiver is { }) { - var method = symbol switch - { - MethodSymbol m => m, - PropertySymbol p => p.GetMethod ?? p.SetMethod, - _ => throw ExceptionUtilities.UnexpectedValue(symbol) - }; - Debug.Assert(receiver.Type is { }); Debug.Assert(receiverIsSubjectToCloning != ThreeState.Unknown); + var method = methodInfo.Method; if (receiverIsSubjectToCloning == ThreeState.True) { Debug.Assert(receiver is not BoundValuePlaceholderBase && method is not null && receiver.Type?.IsReferenceType == false); @@ -2181,7 +2269,7 @@ private void GetInvocationArgumentsForEscape( receiver = new BoundCapturedReceiverPlaceholder(receiver.Syntax, receiver, _localScopeDepth, receiver.Type).MakeCompilerGenerated(); } - var tuple = getReceiver(method, receiver); + var tuple = getReceiver(methodInfo, receiver); escapeArguments.Add(tuple); if (mixableArguments is not null && isMixableParameter(tuple.Parameter)) @@ -2254,7 +2342,33 @@ static bool isMixableArgument(BoundExpression argument) return true; } - static EscapeArgument getReceiver(MethodSymbol? method, BoundExpression receiver) + static EscapeArgument getReceiver(in MethodInfo methodInfo, BoundExpression receiver) + { + // When there is compound usage the receiver is used once but both the get and + // set methods are invoked. This will prefer an accessor that has a writable + // `this` as it's more dangerous from a ref safety standpoint. + if (methodInfo.Method is not null && methodInfo.SetMethod is not null) + { + var getArgument = getReceiverCore(methodInfo.Method, receiver); + if (getArgument.RefKind == RefKind.Ref) + { + return getArgument; + } + + var setArgument = getReceiverCore(methodInfo.SetMethod, receiver); + if (setArgument.RefKind == RefKind.Ref) + { + return setArgument; + } + + Debug.Assert(!getArgument.RefKind.IsWritableReference()); + return getArgument; + } + + return getReceiverCore(methodInfo.Method, receiver); + } + + static EscapeArgument getReceiverCore(MethodSymbol? method, BoundExpression receiver) { if (method is FunctionPointerMethodSymbol) { @@ -2306,7 +2420,7 @@ static void getArgList( /// are included, and some arguments may be included twice - once for value, once for ref. /// private void GetFilteredInvocationArgumentsForEscapeWithUpdatedRules( - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2335,13 +2449,13 @@ private void GetFilteredInvocationArgumentsForEscapeWithUpdatedRules( // If we're not invoking with ref or returning a ref struct then the spec does not consider // any arguments hence the filter is always empty. - if (!isInvokedWithRef && !hasRefLikeReturn(symbol)) + if (!isInvokedWithRef && !hasRefLikeReturn(methodInfo.Symbol)) { return; } GetEscapeValuesForUpdatedRules( - symbol, + methodInfo, receiver, receiverIsSubjectToCloning, parameters, @@ -2371,6 +2485,53 @@ static bool hasRefLikeReturn(Symbol symbol) } } + /// + /// Returns the set of to an invocation that impact ref analysis. + /// This will filter out everything that could never meaningfully contribute to ref analysis. + /// + private void GetEscapeValues( + in MethodInfo methodInfo, + BoundExpression? receiver, + ThreeState receiverIsSubjectToCloning, + ImmutableArray parameters, + ImmutableArray argsOpt, + ImmutableArray argRefKindsOpt, + ImmutableArray argsToParamsOpt, + bool ignoreArglistRefKinds, + ArrayBuilder? mixableArguments, + ArrayBuilder escapeValues) + { + if (methodInfo.UseUpdatedEscapeRules) + { + GetEscapeValuesForUpdatedRules( + methodInfo, + receiver, + receiverIsSubjectToCloning, + parameters, + argsOpt, + argRefKindsOpt, + argsToParamsOpt, + ignoreArglistRefKinds, + mixableArguments, + escapeValues); + } + else + { + GetEscapeValuesForOldRules( + methodInfo, + receiver, + receiverIsSubjectToCloning, + parameters, + argsOpt, + argRefKindsOpt, + argsToParamsOpt, + ignoreArglistRefKinds, + mixableArguments, + escapeValues); + } + + } + /// /// Returns the set of to an invocation that impact ref analysis. /// This will filter out everything that could never meaningfully contribute to ref analysis. For @@ -2384,7 +2545,7 @@ static bool hasRefLikeReturn(Symbol symbol) /// result from this invocation. That is useful for MAMM analysis. /// private void GetEscapeValuesForUpdatedRules( - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiver, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2395,7 +2556,7 @@ private void GetEscapeValuesForUpdatedRules( ArrayBuilder? mixableArguments, ArrayBuilder escapeValues) { - if (!symbol.RequiresInstanceReceiver()) + if (!methodInfo.Symbol.RequiresInstanceReceiver()) { // ignore receiver when symbol is static receiver = null; @@ -2403,7 +2564,7 @@ private void GetEscapeValuesForUpdatedRules( var escapeArguments = ArrayBuilder.GetInstance(); GetInvocationArgumentsForEscape( - symbol, + methodInfo, receiver, receiverIsSubjectToCloning, parameters, @@ -2448,6 +2609,75 @@ private void GetEscapeValuesForUpdatedRules( escapeArguments.Free(); } + /// + /// Returns the set of to an invocation that impact ref analysis. + /// This will filter out everything that could never meaningfully contribute to ref analysis. For + /// example: + /// - For ref arguments it will return an for both ref and + /// value escape. + /// - It will remove value escape for non-ref struct. + /// - It will remove ref escape for args which correspond to any refs as old rules couldn't + /// escape refs + /// Note: this does not consider scoped-ness as it was not present in old rules + /// + private void GetEscapeValuesForOldRules( + in MethodInfo methodInfo, + BoundExpression? receiver, + ThreeState receiverIsSubjectToCloning, + ImmutableArray parameters, + ImmutableArray argsOpt, + ImmutableArray argRefKindsOpt, + ImmutableArray argsToParamsOpt, + bool ignoreArglistRefKinds, + ArrayBuilder? mixableArguments, + ArrayBuilder escapeValues) + { + if (!methodInfo.Symbol.RequiresInstanceReceiver()) + { + // ignore receiver when symbol is static + receiver = null; + } + + var escapeArguments = ArrayBuilder.GetInstance(); + GetInvocationArgumentsForEscape( + methodInfo, + receiver, + receiverIsSubjectToCloning, + parameters, + argsOpt, + argRefKindsOpt, + argsToParamsOpt, + ignoreArglistRefKinds, + mixableArguments, + escapeArguments); + + foreach (var (parameter, argument, refKind) in escapeArguments) + { + // This means it's part of an __arglist or function pointer receiver. + if (parameter is null) + { + if (argument.Type?.IsRefLikeOrAllowsRefLikeType() == true) + { + escapeValues.Add(new EscapeValue(parameter: null, argument, EscapeLevel.CallingMethod, isRefEscape: false)); + } + + continue; + } + + if (parameter.Type.IsRefLikeOrAllowsRefLikeType()) + { + escapeValues.Add(new EscapeValue(parameter, argument, EscapeLevel.CallingMethod, isRefEscape: false)); + } + + if (parameter.RefKind != RefKind.None) + { + escapeValues.Add(new EscapeValue(parameter, argument, EscapeLevel.CallingMethod, isRefEscape: true)); + } + } + + escapeArguments.Free(); + } + private static string GetInvocationParameterName(ParameterSymbol? parameter) { if (parameter is null) @@ -2474,17 +2704,6 @@ private static void ReportInvocationEscapeError( Error(diagnostics, errorCode, syntax, symbol, parameterName); } - private bool UseUpdatedEscapeRulesForInvocation(Symbol symbol) - { - var method = symbol switch - { - MethodSymbol m => m, - PropertySymbol p => p.GetMethod ?? p.SetMethod, - _ => throw ExceptionUtilities.UnexpectedValue(symbol) - }; - return method?.UseUpdatedEscapeRules == true; - } - private bool ShouldInferDeclarationExpressionValEscape(BoundExpression argument, [NotNullWhen(true)] out SourceLocalSymbol? localSymbol) { var symbol = argument switch @@ -2516,7 +2735,7 @@ private bool ShouldInferDeclarationExpressionValEscape(BoundExpression argument, /// private bool CheckInvocationArgMixing( SyntaxNode syntax, - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiverOpt, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2526,9 +2745,9 @@ private bool CheckInvocationArgMixing( uint scopeOfTheContainingExpression, BindingDiagnosticBag diagnostics) { - if (UseUpdatedEscapeRulesForInvocation(symbol)) + if (methodInfo.UseUpdatedEscapeRules) { - return CheckInvocationArgMixingWithUpdatedRules(syntax, symbol, receiverOpt, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, scopeOfTheContainingExpression, diagnostics); + return CheckInvocationArgMixingWithUpdatedRules(syntax, methodInfo, receiverOpt, receiverIsSubjectToCloning, parameters, argsOpt, argRefKindsOpt, argsToParamsOpt, scopeOfTheContainingExpression, diagnostics); } // SPEC: @@ -2536,6 +2755,7 @@ private bool CheckInvocationArgMixing( // - If there is a ref or out argument of a ref struct type (including the receiver), with safe-to-escape E1, then // - no argument (including the receiver) may have a narrower safe-to-escape than E1. + var symbol = methodInfo.Symbol; if (!symbol.RequiresInstanceReceiver()) { // ignore receiver when symbol is static @@ -2546,18 +2766,9 @@ private bool CheckInvocationArgMixing( uint escapeTo = scopeOfTheContainingExpression; // collect all writeable ref-like arguments, including receiver - var receiverType = receiverOpt?.Type; - if (receiverType?.IsRefLikeOrAllowsRefLikeType() == true && !IsReceiverRefReadOnly(symbol)) - { - // https://github.com/dotnet/roslyn/issues/73550: - // We do not have a test that demonstrates that the statement below makes a difference. - // If it is commented out, not a single test fails - escapeTo = GetValEscape(receiverOpt, scopeOfTheContainingExpression); - } - var escapeArguments = ArrayBuilder.GetInstance(); GetInvocationArgumentsForEscape( - symbol, + methodInfo, receiverOpt, receiverIsSubjectToCloning, parameters, @@ -2574,8 +2785,9 @@ private bool CheckInvocationArgMixing( { if (ShouldInferDeclarationExpressionValEscape(argument, out _)) { - // assume any expression variable is a valid mixing destination, - // since we will infer a legal val-escape for it (if it doesn't already have a narrower one). + // Any variable from a declaration expression is a valid mixing destination as we + // infer a legal value escape for it. It does not contribute input as it's declared + // at this point (functions like an `out` in the new escape rules) continue; } @@ -2623,7 +2835,7 @@ private bool CheckInvocationArgMixing( private bool CheckInvocationArgMixingWithUpdatedRules( SyntaxNode syntax, - Symbol symbol, + in MethodInfo methodInfo, BoundExpression? receiverOpt, ThreeState receiverIsSubjectToCloning, ImmutableArray parameters, @@ -2636,7 +2848,7 @@ private bool CheckInvocationArgMixingWithUpdatedRules( var mixableArguments = ArrayBuilder.GetInstance(); var escapeValues = ArrayBuilder.GetInstance(); GetEscapeValuesForUpdatedRules( - symbol, + methodInfo, receiverOpt, receiverIsSubjectToCloning, parameters, @@ -2673,7 +2885,7 @@ private bool CheckInvocationArgMixingWithUpdatedRules( if (!valid) { string parameterName = GetInvocationParameterName(fromParameter); - Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, symbol, parameterName); + Error(diagnostics, ErrorCode.ERR_CallArgMixing, syntax, methodInfo.Symbol, parameterName); break; } } @@ -2712,17 +2924,6 @@ void inferDeclarationExpressionValEscape() } } - private static bool IsReceiverRefReadOnly(Symbol methodOrPropertySymbol) => methodOrPropertySymbol switch - { - MethodSymbol m => m.IsEffectivelyReadOnly, - // TODO: val escape checks should be skipped for property accesses when - // we can determine the only accessors being called are readonly. - // For now we are pessimistic and check escape if any accessor is non-readonly. - // Tracking in https://github.com/dotnet/roslyn/issues/35606 - PropertySymbol p => p.GetMethod?.IsEffectivelyReadOnly != false && p.SetMethod?.IsEffectivelyReadOnly != false, - _ => throw ExceptionUtilities.UnexpectedValue(methodOrPropertySymbol) - }; - #if DEBUG private static bool AllParametersConsideredInEscapeAnalysisHaveArguments( ImmutableArray argsOpt, @@ -3198,7 +3399,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres } return GetInvocationEscapeScope( - call.Method, + MethodInfo.Create(call.Method), call.ReceiverOpt, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -3220,7 +3421,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres } return GetInvocationEscapeScope( - methodSymbol, + MethodInfo.Create(methodSymbol), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, methodSymbol.Parameters, @@ -3237,7 +3438,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres var indexerSymbol = indexerAccess.Indexer; return GetInvocationEscapeScope( - indexerSymbol, + MethodInfo.Create(indexerAccess), indexerAccess.ReceiverOpt, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3259,7 +3460,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres var indexerSymbol = indexerAccess.Indexer; return GetInvocationEscapeScope( - indexerSymbol, + MethodInfo.Create(indexerAccess), implicitIndexerAccess.Receiver, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3281,7 +3482,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres } return GetInvocationEscapeScope( - call.Method, + MethodInfo.Create(call.Method), implicitIndexerAccess.Receiver, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -3313,7 +3514,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres Debug.Assert(equivalentSignatureMethod.RefKind != RefKind.None); return GetInvocationEscapeScope( - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -3329,7 +3530,7 @@ internal uint GetRefEscape(BoundExpression expr, uint scopeOfTheContainingExpres // not passing any arguments/parameters return GetInvocationEscapeScope( - propertyAccess.PropertySymbol, + MethodInfo.Create(propertyAccess.PropertySymbol), propertyAccess.ReceiverOpt, propertyAccess.InitialBindingReceiverIsSubjectToCloning, default, @@ -3516,7 +3717,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( call.Syntax, - methodSymbol, + MethodInfo.Create(methodSymbol), call.ReceiverOpt, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -3542,7 +3743,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( indexerAccess.Syntax, - indexerSymbol, + MethodInfo.Create(indexerAccess), indexerAccess.ReceiverOpt, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3573,7 +3774,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( indexerAccess.Syntax, - indexerSymbol, + MethodInfo.Create(indexerAccess), implicitIndexerAccess.Receiver, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3599,7 +3800,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( call.Syntax, - methodSymbol, + MethodInfo.Create(methodSymbol), implicitIndexerAccess.Receiver, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -3635,7 +3836,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( elementAccess.Syntax, - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -3660,7 +3861,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( functionPointerInvocation.Syntax, - signature, + MethodInfo.Create(signature), functionPointerInvocation.InvokedExpression, receiverIsSubjectToCloning: ThreeState.False, signature.Parameters, @@ -3685,7 +3886,7 @@ internal bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint escapeF // not passing any arguments/parameters return CheckInvocationEscape( propertyAccess.Syntax, - propertySymbol, + MethodInfo.Create(propertySymbol), propertyAccess.ReceiverOpt, propertyAccess.InitialBindingReceiverIsSubjectToCloning, default, @@ -3907,7 +4108,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var call = (BoundCall)expr; return GetInvocationEscapeScope( - call.Method, + MethodInfo.Create(call.Method), call.ReceiverOpt, call.InitialBindingReceiverIsSubjectToCloning, call.Method.Parameters, @@ -3923,7 +4124,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var ptrSymbol = ptrInvocation.FunctionPointer.Signature; return GetInvocationEscapeScope( - ptrSymbol, + MethodInfo.Create(ptrSymbol), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, ptrSymbol.Parameters, @@ -3939,7 +4140,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var indexerSymbol = indexerAccess.Indexer; return GetInvocationEscapeScope( - indexerSymbol, + MethodInfo.Create(indexerAccess), indexerAccess.ReceiverOpt, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3961,7 +4162,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var indexerSymbol = indexerAccess.Indexer; return GetInvocationEscapeScope( - indexerSymbol, + MethodInfo.Create(indexerAccess), implicitIndexerAccess.Receiver, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -3977,7 +4178,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres case BoundCall call: return GetInvocationEscapeScope( - call.Method, + MethodInfo.Create(call.Method), implicitIndexerAccess.Receiver, call.InitialBindingReceiverIsSubjectToCloning, call.Method.Parameters, @@ -4000,7 +4201,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres SignatureOnlyMethodSymbol equivalentSignatureMethod = GetInlineArrayAccessEquivalentSignatureMethod(elementAccess, out arguments, out refKinds); return GetInvocationEscapeScope( - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -4016,7 +4217,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres // not passing any arguments/parameters return GetInvocationEscapeScope( - propertyAccess.PropertySymbol, + MethodInfo.Create(propertyAccess.PropertySymbol), propertyAccess.ReceiverOpt, propertyAccess.InitialBindingReceiverIsSubjectToCloning, default, @@ -4032,7 +4233,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var constructorSymbol = objectCreation.Constructor; var escape = GetInvocationEscapeScope( - constructorSymbol, + MethodInfo.Create(constructorSymbol), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, constructorSymbol.Parameters, @@ -4077,7 +4278,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres if (unaryOperator.MethodOpt is { } unaryMethod) { return GetInvocationEscapeScope( - unaryMethod, + MethodInfo.Create(unaryMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, unaryMethod.Parameters, @@ -4113,7 +4314,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres SignatureOnlyMethodSymbol equivalentSignatureMethod = GetInlineArrayConversionEquivalentSignatureMethod(conversion, out arguments, out refKinds); return GetInvocationEscapeScope( - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -4130,7 +4331,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres Debug.Assert(operatorMethod is not null); return GetInvocationEscapeScope( - operatorMethod, + MethodInfo.Create(operatorMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, operatorMethod.Parameters, @@ -4162,7 +4363,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres if (compound.Operator.Method is { } compoundMethod) { return GetInvocationEscapeScope( - compoundMethod, + MethodInfo.Create(compoundMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, compoundMethod.Parameters, @@ -4182,7 +4383,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres if (binary.Method is { } binaryMethod) { return GetInvocationEscapeScope( - binaryMethod, + MethodInfo.Create(binaryMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, binaryMethod.Parameters, @@ -4206,7 +4407,7 @@ internal uint GetValEscape(BoundExpression expr, uint scopeOfTheContainingExpres var uo = (BoundUserDefinedConditionalLogicalOperator)expr; return GetInvocationEscapeScope( - uo.LogicalOperator, + MethodInfo.Create(uo.LogicalOperator), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, uo.LogicalOperator.Parameters, @@ -4332,7 +4533,6 @@ private bool HasLocalScope(BoundCollectionExpression expr) throw ExceptionUtilities.UnexpectedValue(collectionTypeKind); // ref struct collection type with unexpected type kind } } -#nullable disable private uint GetTupleValEscape(ImmutableArray elements, uint scopeOfTheContainingExpression) { @@ -4345,32 +4545,112 @@ private uint GetTupleValEscape(ImmutableArray elements, uint sc return narrowestScope; } + /// + /// The escape value of an object initializer is calculated by looking at all of the + /// expressions that can be stored into the implicit receiver. That means arguments + /// passed to an indexer for example only matter if they can escape into the receiver + /// as a stored field. + /// private uint GetValEscapeOfObjectInitializer(BoundObjectInitializerExpression initExpr, uint scopeOfTheContainingExpression) { var result = CallingMethodScope; - foreach (var expression in initExpr.Initializers) + foreach (var expr in initExpr.Initializers) + { + var exprResult = GetValEscapeOfObjectMemberInitializer(expr, scopeOfTheContainingExpression); + result = Math.Max(result, exprResult); + } + + return result; + } + + private uint GetValEscapeOfObjectMemberInitializer(BoundExpression expr, uint scopeOfTheContainingExpression) + { + uint result; + if (expr.Kind == BoundKind.AssignmentOperator) { - if (expression.Kind == BoundKind.AssignmentOperator) + var assignment = (BoundAssignmentOperator)expr; + var rightEscape = assignment.IsRef + ? GetRefEscape(assignment.Right, scopeOfTheContainingExpression) + : GetValEscape(assignment.Right, scopeOfTheContainingExpression); + + var left = (BoundObjectInitializerMember)assignment.Left; + result = left.MemberSymbol switch { - var assignment = (BoundAssignmentOperator)expression; - var rightValEscape = assignment.IsRef - ? GetRefEscape(assignment.Right, scopeOfTheContainingExpression) - : GetValEscape(assignment.Right, scopeOfTheContainingExpression); + PropertySymbol { IsIndexer: true } indexer => getIndexerEscape(indexer, left, rightEscape), + PropertySymbol property => getPropertyEscape(property, rightEscape), + _ => rightEscape + }; + } + else + { + result = GetValEscape(expr, scopeOfTheContainingExpression); + } - result = Math.Max(result, rightValEscape); + return result; - var left = (BoundObjectInitializerMember)assignment.Left; - result = Math.Max(result, GetValEscape(left.Arguments, scopeOfTheContainingExpression)); + uint getIndexerEscape( + PropertySymbol indexer, + BoundObjectInitializerMember expr, + uint rightEscapeScope) + { + Debug.Assert(expr.AccessorKind != AccessorKind.Unknown); + var methodInfo = MethodInfo.Create(indexer, expr.AccessorKind); + if (methodInfo.Method is null) + { + return CallingMethodScope; } - else + + // If the indexer is readonly then none of the arguments can contribute to + // the receiver escape + if (methodInfo.Method.IsEffectivelyReadOnly) + { + return CallingMethodScope; + } + + var escapeValues = ArrayBuilder.GetInstance(); + GetEscapeValues( + methodInfo, + // This is calculating the actual receiver scope + null, + ThreeState.Unknown, + methodInfo.Method.Parameters, + expr.Arguments, + expr.ArgumentRefKindsOpt, + expr.ArgsToParamsOpt, + ignoreArglistRefKinds: true, + mixableArguments: null, + escapeValues); + + uint receiverEscapeScope = CallingMethodScope; + foreach (var escapeValue in escapeValues) { - result = Math.Max(result, GetValEscape(expression, scopeOfTheContainingExpression)); + uint escapeScope = escapeValue.IsRefEscape + ? GetRefEscape(escapeValue.Argument, scopeOfTheContainingExpression) + : GetValEscape(escapeValue.Argument, scopeOfTheContainingExpression); + receiverEscapeScope = Math.Max(escapeScope, receiverEscapeScope); } + + escapeValues.Free(); + return Math.Max(receiverEscapeScope, rightEscapeScope); } - return result; + uint getPropertyEscape( + PropertySymbol property, + uint rightEscapeScope) + { + var accessorKind = property.RefKind == RefKind.None ? AccessorKind.Set : AccessorKind.Get; + var methodInfo = MethodInfo.Create(property, accessorKind); + if (methodInfo.Method is null || methodInfo.Method.IsEffectivelyReadOnly) + { + return CallingMethodScope; + } + + return rightEscapeScope; + } } +#nullable disable + private uint GetValEscape(ImmutableArray expressions, uint scopeOfTheContainingExpression) { var result = CallingMethodScope; @@ -4533,7 +4813,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( call.Syntax, - methodSymbol, + MethodInfo.Create(methodSymbol), call.ReceiverOpt, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -4553,7 +4833,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( ptrInvocation.Syntax, - ptrSymbol, + MethodInfo.Create(ptrSymbol), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, ptrSymbol.Parameters, @@ -4573,7 +4853,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( indexerAccess.Syntax, - indexerSymbol, + MethodInfo.Create(indexerAccess), indexerAccess.ReceiverOpt, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -4599,7 +4879,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( indexerAccess.Syntax, - indexerSymbol, + MethodInfo.Create(indexerAccess), implicitIndexerAccess.Receiver, indexerAccess.InitialBindingReceiverIsSubjectToCloning, indexerSymbol.Parameters, @@ -4621,7 +4901,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( call.Syntax, - methodSymbol, + MethodInfo.Create(methodSymbol), implicitIndexerAccess.Receiver, call.InitialBindingReceiverIsSubjectToCloning, methodSymbol.Parameters, @@ -4648,7 +4928,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( elementAccess.Syntax, - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -4668,7 +4948,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF // not passing any arguments/parameters return CheckInvocationEscape( propertyAccess.Syntax, - propertyAccess.PropertySymbol, + MethodInfo.Create(propertyAccess.PropertySymbol), propertyAccess.ReceiverOpt, propertyAccess.InitialBindingReceiverIsSubjectToCloning, default, @@ -4688,7 +4968,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF var escape = CheckInvocationEscape( objectCreation.Syntax, - constructorSymbol, + MethodInfo.Create(constructorSymbol), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, constructorSymbol.Parameters, @@ -4755,7 +5035,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF { return CheckInvocationEscape( unary.Syntax, - unaryMethod, + MethodInfo.Create(unaryMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, unaryMethod.Parameters, @@ -4802,7 +5082,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( conversion.Syntax, - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -4823,7 +5103,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( conversion.Syntax, - operatorMethod, + MethodInfo.Create(operatorMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, operatorMethod.Parameters, @@ -4858,7 +5138,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF { return CheckInvocationEscape( compound.Syntax, - compoundMethod, + MethodInfo.Create(compoundMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, compoundMethod.Parameters, @@ -4887,7 +5167,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF { return CheckInvocationEscape( binary.Syntax, - binaryMethod, + MethodInfo.Create(binaryMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, binaryMethod.Parameters, @@ -4919,7 +5199,7 @@ internal bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint escapeF return CheckInvocationEscape( uo.Syntax, - uo.LogicalOperator, + MethodInfo.Create(uo.LogicalOperator), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, uo.LogicalOperator.Parameters, @@ -5232,40 +5512,24 @@ private bool CheckTupleValEscape(ImmutableArray elements, uint return true; } +#nullable enable + private bool CheckValEscapeOfObjectInitializer(BoundObjectInitializerExpression initExpr, uint escapeFrom, uint escapeTo, BindingDiagnosticBag diagnostics) { - foreach (var expression in initExpr.Initializers) + foreach (var expr in initExpr.Initializers) { - if (expression.Kind == BoundKind.AssignmentOperator) + if (GetValEscapeOfObjectMemberInitializer(expr, escapeFrom) > escapeTo) { - var assignment = (BoundAssignmentOperator)expression; - bool valid = assignment.IsRef - ? CheckRefEscape(expression.Syntax, assignment.Right, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics) - : CheckValEscape(expression.Syntax, assignment.Right, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics); - - if (!valid) - { - return false; - } - - var left = (BoundObjectInitializerMember)assignment.Left; - if (!CheckValEscape(left.Arguments, escapeFrom, escapeTo, diagnostics: diagnostics)) - { - return false; - } - } - else - { - if (!CheckValEscape(expression.Syntax, expression, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics)) - { - return false; - } + Error(diagnostics, _inUnsafeRegion ? ErrorCode.WRN_EscapeVariable : ErrorCode.ERR_EscapeVariable, initExpr.Syntax, expr.Syntax); + return false; } } return true; } +#nullable disable + private bool CheckValEscape(ImmutableArray expressions, uint escapeFrom, uint escapeTo, BindingDiagnosticBag diagnostics) { foreach (var expression in expressions) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs index 7b600508a1c15..c19f2399a4cc0 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @@ -5762,6 +5762,7 @@ private BoundExpression BindObjectInitializerMember( ImmutableArray argumentRefKindsOpt = default; BitVector defaultArguments = default; bool expanded = false; + AccessorKind accessorKind = AccessorKind.Unknown; switch (boundMemberKind) { @@ -5800,6 +5801,7 @@ private BoundExpression BindObjectInitializerMember( argumentRefKindsOpt = indexer.ArgumentRefKindsOpt; defaultArguments = indexer.DefaultArguments; expanded = indexer.Expanded; + accessorKind = indexer.AccessorKind; // If any of the arguments is an interpolated string handler that takes the receiver as an argument for creation, // we disallow this. During lowering, indexer arguments are evaluated before the receiver for this scenario, and @@ -5876,6 +5878,7 @@ private BoundExpression BindObjectInitializerMember( argsToParamsOpt, defaultArguments, resultKind, + accessorKind, implicitReceiver.Type, type: boundMember.Type, hasErrors: hasErrors); @@ -9766,6 +9769,7 @@ private BoundExpression BindIndexerOrIndexedPropertyAccessContinued( argumentNames, argumentRefKinds, expanded: resolutionResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm, + AccessorKind.Unknown, argsToParams, defaultArguments: default, property.Type, diff --git a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs index 44f2e3406ca1a..3b49411795218 100644 --- a/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs +++ b/src/Compilers/CSharp/Portable/Binder/RefSafetyAnalysis.cs @@ -667,7 +667,7 @@ protected override void VisitArguments(BoundCall node) var method = node.Method; CheckInvocationArgMixing( node.Syntax, - method, + MethodInfo.Create(method), node.ReceiverOpt, node.InitialBindingReceiverIsSubjectToCloning, method.Parameters, @@ -764,9 +764,10 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase var constructor = node.Constructor; if (constructor is { }) { + var methodInfo = MethodInfo.Create(constructor); CheckInvocationArgMixing( node.Syntax, - constructor, + in methodInfo, receiverOpt: null, receiverIsSubjectToCloning: ThreeState.Unknown, constructor.Parameters, @@ -775,6 +776,58 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase node.ArgsToParamsOpt, _localScopeDepth, _diagnostics); + + if (node.InitializerExpressionOpt is { }) + { + // Object initializers are different than a normal constructor in that the + // scope of the receiver is determined by evaluating the inputs to the constructor + // *and* all of the initializers. Another way of thinking about this is that + // every argument in an initializer that can escape to the receiver is + // effectively an argument to the constructor. That means we need to do + // a second mixing pass here where we consider the receiver escaping + // back into the ref parameters of the constructor. + // + // At the moment this is only a hypothetical problem. Because the language + // doesn't support ref field of ref struct mixing like this could not actually + // happen in practice. At the same time we want to error on this now so that + // in a future when we do have ref field to ref struct this is not a breaking + // change. Customers can respond to failures like this by putting scoped on + // such parameters in the constructor. + var escapeValues = ArrayBuilder.GetInstance(); + var escapeFrom = GetValEscape(node.InitializerExpressionOpt, _localScopeDepth); + GetEscapeValues( + in methodInfo, + receiver: null, + receiverIsSubjectToCloning: ThreeState.Unknown, + constructor.Parameters, + node.Arguments, + node.ArgumentRefKindsOpt, + node.ArgsToParamsOpt, + ignoreArglistRefKinds: false, + mixableArguments: null, + escapeValues); + + foreach (var (parameter, argument, _, isRefEscape) in escapeValues) + { + if (!isRefEscape) + { + continue; + } + + if (parameter?.Type?.IsRefLikeOrAllowsRefLikeType() != true || + !parameter.RefKind.IsWritableReference()) + { + continue; + } + + if (escapeFrom > GetValEscape(argument, _localScopeDepth)) + { + Error(_diagnostics, ErrorCode.ERR_CallArgMixing, argument.Syntax, constructor, parameter.Name); + } + } + + escapeValues.Free(); + } } } } @@ -796,7 +849,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase var indexer = node.Indexer; CheckInvocationArgMixing( node.Syntax, - indexer, + MethodInfo.Create(node), node.ReceiverOpt, node.InitialBindingReceiverIsSubjectToCloning, indexer.Parameters, @@ -819,7 +872,7 @@ private void VisitObjectCreationExpressionBase(BoundObjectCreationExpressionBase var method = node.FunctionPointer.Signature; CheckInvocationArgMixing( node.Syntax, - method, + MethodInfo.Create(method), receiverOpt: null, receiverIsSubjectToCloning: ThreeState.Unknown, method.Parameters, @@ -920,7 +973,7 @@ private void VisitDeconstructionArguments(ArrayBuilder v CheckInvocationArgMixing( syntax, - deconstructMethod, + MethodInfo.Create(deconstructMethod), invocation.ReceiverOpt, invocation.InitialBindingReceiverIsSubjectToCloning, parameters, @@ -1011,7 +1064,7 @@ private static ImmutableArray GetDeconstructionRightParts(Bound out arguments, out refKinds); collectionEscape = GetInvocationEscapeScope( - equivalentSignatureMethod, + MethodInfo.Create(equivalentSignatureMethod), receiver: null, receiverIsSubjectToCloning: ThreeState.Unknown, equivalentSignatureMethod.Parameters, @@ -1038,6 +1091,7 @@ private static ImmutableArray GetDeconstructionRightParts(Bound { placeholders.Add((targetPlaceholder, collectionEscape)); } + if (node.AwaitOpt is { } awaitableInfo) { GetAwaitableInstancePlaceholders(placeholders, awaitableInfo, collectionEscape); diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs b/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs index 5100cea5bcaa9..9fff5020e041f 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs @@ -327,6 +327,14 @@ public override Symbol? ExpressionSymbol } } + internal enum AccessorKind : byte + { + Unknown, + Get, + Set, + Both + } + internal partial class BoundIndexerAccess { public override Symbol? ExpressionSymbol @@ -341,6 +349,22 @@ public override LookupResultKind ResultKind return !this.OriginalIndexersOpt.IsDefault ? LookupResultKind.OverloadResolutionFailure : base.ResultKind; } } + + public BoundIndexerAccess Update(AccessorKind accessorKind) + { + return this.Update( + ReceiverOpt, + InitialBindingReceiverIsSubjectToCloning, + Indexer, + Arguments, + ArgumentNamesOpt, + ArgumentRefKindsOpt, + Expanded, + accessorKind, + ArgsToParamsOpt, + DefaultArguments, + Type); + } } internal partial class BoundDynamicIndexerAccess diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml b/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml index 54fadfbd7872f..289afc739f4f2 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml @@ -38,6 +38,7 @@ + @@ -2020,6 +2021,7 @@ + @@ -2208,6 +2210,7 @@ +