From aa6b8de54bd0177c0162de27a06d3a6caffe6f6b Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Tue, 25 Jan 2022 16:34:05 -0800 Subject: [PATCH 1/7] Avoid assuming containing assembly is from source in DefiniteAssignment --- .../FlowAnalysis/DefiniteAssignment.cs | 21 ++++---- .../FlowAnalysis/RegionAnalysisTests.cs | 53 +++++++++++++++++++ .../ExtractMethod/ExtractMethodTests.cs | 21 ++++++++ 3 files changed, 83 insertions(+), 12 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 9661ded9d326e..3b1f930f4aa9e 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -#nullable disable - #if DEBUG // We use a struct rather than a class to represent the state for efficiency // for data flow analysis, with 32 bits of data inline. Merely copying the state @@ -59,7 +57,7 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< /// /// Some variables that should be considered initially assigned. Used for region analysis. /// - private readonly HashSet initiallyAssignedVariables; + private readonly HashSet? initiallyAssignedVariables; /// /// Variables that were used anywhere, in the sense required to suppress warnings about @@ -67,12 +65,10 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< /// private readonly PooledHashSet _usedVariables = PooledHashSet.GetInstance(); -#nullable enable /// /// Parameters of record primary constructors that were read anywhere. /// private PooledHashSet? _readParameters; -#nullable disable /// /// Variables that were used anywhere, in the sense required to suppress warnings about @@ -105,12 +101,12 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< /// /// The current source assembly. /// - private readonly SourceAssemblySymbol _sourceAssembly; + private readonly SourceAssemblySymbol? _sourceAssembly; /// /// A set of address-of expressions for which the operand is not definitely assigned. /// - private readonly HashSet _unassignedVariableAddressOfSyntaxes; + private readonly HashSet? _unassignedVariableAddressOfSyntaxes; /// /// Tracks variables for which we have already reported a definite assignment error. This @@ -136,7 +132,7 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< /// /// The topmost method of this analysis. /// - protected MethodSymbol topLevelMethod; + protected MethodSymbol? topLevelMethod; protected bool _convertInsufficientExecutionStackExceptionToCancelledByStackGuardException = false; // By default, just let the original exception to bubble up. @@ -151,7 +147,7 @@ internal DefiniteAssignmentPass( BoundNode node, bool strictAnalysis, bool trackUnassignments = false, - HashSet unassignedVariableAddressOfSyntaxes = null, + HashSet? unassignedVariableAddressOfSyntaxes = null, bool requireOutParamsAssigned = true, bool trackClassFields = false, bool trackStaticMembers = false) @@ -160,7 +156,7 @@ internal DefiniteAssignmentPass( trackUnassignments) { this.initiallyAssignedVariables = null; - _sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly; + _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; _unassignedVariableAddressOfSyntaxes = unassignedVariableAddressOfSyntaxes; _requireOutParamsAssigned = requireOutParamsAssigned; _trackClassFields = trackClassFields; @@ -175,11 +171,11 @@ internal DefiniteAssignmentPass( BoundNode node, EmptyStructTypeCache emptyStructs, bool trackUnassignments = false, - HashSet initiallyAssignedVariables = null) + HashSet? initiallyAssignedVariables = null) : base(compilation, member, node, emptyStructs, trackUnassignments) { this.initiallyAssignedVariables = initiallyAssignedVariables; - _sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly; + _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; this.CurrentSymbol = member; _unassignedVariableAddressOfSyntaxes = null; _requireOutParamsAssigned = true; @@ -237,6 +233,7 @@ protected override int AddVariable(VariableIdentifier identifier) return slot; } +#nullable disable protected Symbol GetNonMemberSymbol(int slot) { VariableIdentifier variableId = variableBySlot[slot]; diff --git a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs index 552d59dbf7a25..2016656db7c10 100644 --- a/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/FlowAnalysis/RegionAnalysisTests.cs @@ -8032,6 +8032,59 @@ static void Main() Assert.Equal("px", GetSymbolNamesJoined(analysis.WrittenOutside)); } + [WorkItem(57428, "https://github.com/dotnet/roslyn/issues/57428")] + [Fact] + public void AttributeArgumentWithLambdaBody_01() + { + var source = +@"using System.Runtime.InteropServices; +class Program +{ + static void F([DefaultParameterValue(() => { return 0; })] object obj) + { + } +}"; + var compilation = CreateCompilation(source); + compilation.VerifyDiagnostics( + // (4,42): error CS0182: An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type + // static void F([DefaultParameterValue(() => { return 0; })] object obj) + Diagnostic(ErrorCode.ERR_BadAttributeArgument, "() => { return 0; }").WithLocation(4, 42)); + + var tree = compilation.SyntaxTrees[0]; + var model = compilation.GetSemanticModel(tree); + var expr = tree.GetRoot().DescendantNodes().OfType().Single(); + var analysis = model.AnalyzeDataFlow(expr); + Assert.False(analysis.Succeeded); + } + + [Fact] + public void AttributeArgumentWithLambdaBody_02() + { + var source = +@"using System; +class A : Attribute +{ + internal A(object o) { } +} +class Program +{ + static void F([A(() => { return 0; })] object obj) + { + } +}"; + var compilation = CreateCompilation(source); + compilation.VerifyDiagnostics( + // (8,22): error CS0182: An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type + // static void F([A(() => { return 0; })] object obj) + Diagnostic(ErrorCode.ERR_BadAttributeArgument, "() => { return 0; }").WithLocation(8, 22)); + + var tree = compilation.SyntaxTrees[0]; + var model = compilation.GetSemanticModel(tree); + var expr = tree.GetRoot().DescendantNodes().OfType().Single(); + var analysis = model.AnalyzeDataFlow(expr); + Assert.False(analysis.Succeeded); + } + #endregion #region "Used Local Functions" diff --git a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs index 258e4eba49d4e..5a8356a7795b8 100644 --- a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs +++ b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs @@ -4617,5 +4617,26 @@ class Ignored2 { } CodeActionEquivalenceKey = nameof(FeaturesResources.Extract_method), }.RunAsync(); } + + [WorkItem(57428, "https://github.com/dotnet/roslyn/issues/57428")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod)] + public async Task AttributeArgumentWithLambdaBody() + { + await TestInRegularAndScript1Async( +@"using System.Runtime.InteropServices; +class Program +{ + static void F([DefaultParameterValue(() => { return [|null|]; })] object obj) + { + } +}", +@"using System.Runtime.InteropServices; +class Program +{ + static void F([DefaultParameterValue(() => { return [|null|]; })] object obj) + { + } +}"); + } } } From 565ef936467ef91c335f540c0cffcc83601e75b7 Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Tue, 25 Jan 2022 21:14:34 -0800 Subject: [PATCH 2/7] Fix test --- .../CodeActions/ExtractMethod/ExtractMethodTests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs index 5a8356a7795b8..fab6019cea8a2 100644 --- a/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs +++ b/src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs @@ -4633,8 +4633,13 @@ class Program @"using System.Runtime.InteropServices; class Program { - static void F([DefaultParameterValue(() => { return [|null|]; })] object obj) + static void F([DefaultParameterValue(() => { return {|Rename:NewMethod|}(); })] object obj) + { + } + + private static object NewMethod() { + return null; } }"); } From 21c196bb001342a124c564169334b9913ced2333 Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Tue, 25 Jan 2022 21:28:26 -0800 Subject: [PATCH 3/7] PR feedback --- .../Portable/FlowAnalysis/DefiniteAssignment.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 3b1f930f4aa9e..971982790efb3 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -156,7 +156,7 @@ internal DefiniteAssignmentPass( trackUnassignments) { this.initiallyAssignedVariables = null; - _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; + _sourceAssembly = GetSourceAssembly(compilation, member); _unassignedVariableAddressOfSyntaxes = unassignedVariableAddressOfSyntaxes; _requireOutParamsAssigned = requireOutParamsAssigned; _trackClassFields = trackClassFields; @@ -175,7 +175,7 @@ internal DefiniteAssignmentPass( : base(compilation, member, node, emptyStructs, trackUnassignments) { this.initiallyAssignedVariables = initiallyAssignedVariables; - _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; + _sourceAssembly = GetSourceAssembly(compilation, member); this.CurrentSymbol = member; _unassignedVariableAddressOfSyntaxes = null; _requireOutParamsAssigned = true; @@ -204,6 +204,15 @@ internal DefiniteAssignmentPass( _shouldCheckConverted = this.GetType() == typeof(DefiniteAssignmentPass); } + private static SourceAssemblySymbol? GetSourceAssembly(CSharpCompilation compilation, Symbol member) + { + Debug.Assert(member is null || + member.ContainingAssembly is SourceAssemblySymbol || + (member is TypeSymbol type && compilation.IsAttributeType(type))); + + return member?.ContainingAssembly as SourceAssemblySymbol; + } + protected override void Free() { variableBySlot.Free(); From 3915323b8c7d69b6b98a3910a1f45ee2c162c37f Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Tue, 25 Jan 2022 21:55:51 -0800 Subject: [PATCH 4/7] Avoid using assembly from attribute type --- .../FlowAnalysis/DefiniteAssignment.cs | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 971982790efb3..a286cfe2da7df 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -156,7 +156,7 @@ internal DefiniteAssignmentPass( trackUnassignments) { this.initiallyAssignedVariables = null; - _sourceAssembly = GetSourceAssembly(compilation, member); + _sourceAssembly = GetSourceAssembly(compilation, member, node); _unassignedVariableAddressOfSyntaxes = unassignedVariableAddressOfSyntaxes; _requireOutParamsAssigned = requireOutParamsAssigned; _trackClassFields = trackClassFields; @@ -175,7 +175,7 @@ internal DefiniteAssignmentPass( : base(compilation, member, node, emptyStructs, trackUnassignments) { this.initiallyAssignedVariables = initiallyAssignedVariables; - _sourceAssembly = GetSourceAssembly(compilation, member); + _sourceAssembly = GetSourceAssembly(compilation, member, node); this.CurrentSymbol = member; _unassignedVariableAddressOfSyntaxes = null; _requireOutParamsAssigned = true; @@ -204,13 +204,24 @@ internal DefiniteAssignmentPass( _shouldCheckConverted = this.GetType() == typeof(DefiniteAssignmentPass); } - private static SourceAssemblySymbol? GetSourceAssembly(CSharpCompilation compilation, Symbol member) + private static SourceAssemblySymbol? GetSourceAssembly( + CSharpCompilation compilation, + Symbol member, + BoundNode node) { - Debug.Assert(member is null || - member.ContainingAssembly is SourceAssemblySymbol || - (member is TypeSymbol type && compilation.IsAttributeType(type))); - - return member?.ContainingAssembly as SourceAssemblySymbol; + if (member is null) + { + return null; + } + if (node.Kind == BoundKind.Attribute) + { + // member is the attribute type, not the symbol where the attribute is applied. + Debug.Assert(member is TypeSymbol type && + (type.IsErrorType() || compilation.IsAttributeType(type))); + return null; + } + Debug.Assert(member.ContainingAssembly is SourceAssemblySymbol); + return member.ContainingAssembly as SourceAssemblySymbol; } protected override void Free() From 77ef06046e1ff636b183ebe41982785aefbcca98 Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Wed, 26 Jan 2022 08:44:28 -0800 Subject: [PATCH 5/7] Use Compilation.SourceAssembly --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 1 + .../FlowAnalysis/DefiniteAssignment.cs | 20 ++++++------------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index 9515734437ab1..f993702e31bf1 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -187,6 +187,7 @@ protected AbstractFlowPass( bool trackRegions = false, bool nonMonotonicTransferFunction = false) { + Debug.Assert(compilation != null); Debug.Assert(node != null); if (firstInRegion != null && lastInRegion != null) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index a286cfe2da7df..2ab503c95d58a 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -204,24 +204,16 @@ internal DefiniteAssignmentPass( _shouldCheckConverted = this.GetType() == typeof(DefiniteAssignmentPass); } - private static SourceAssemblySymbol? GetSourceAssembly( + private static SourceAssemblySymbol GetSourceAssembly( CSharpCompilation compilation, Symbol member, BoundNode node) { - if (member is null) - { - return null; - } - if (node.Kind == BoundKind.Attribute) - { - // member is the attribute type, not the symbol where the attribute is applied. - Debug.Assert(member is TypeSymbol type && - (type.IsErrorType() || compilation.IsAttributeType(type))); - return null; - } - Debug.Assert(member.ContainingAssembly is SourceAssemblySymbol); - return member.ContainingAssembly as SourceAssemblySymbol; + Debug.Assert(member is null || + node.Kind == BoundKind.Attribute || + member.ContainingAssembly == compilation.SourceAssembly); + + return compilation.SourceAssembly; } protected override void Free() From 5767bd77904090c0c0ed3c5319ea8757ba6e569f Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Wed, 26 Jan 2022 08:45:41 -0800 Subject: [PATCH 6/7] Misc. --- .../CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index 2ab503c95d58a..cbd99d9c2acce 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -211,7 +211,7 @@ private static SourceAssemblySymbol GetSourceAssembly( { Debug.Assert(member is null || node.Kind == BoundKind.Attribute || - member.ContainingAssembly == compilation.SourceAssembly); + (object)member.ContainingAssembly == compilation.SourceAssembly); return compilation.SourceAssembly; } From 963f0b21628b6d18d9c642a51aca99a5cd0939cf Mon Sep 17 00:00:00 2001 From: Charles Stoner <10732005+cston@users.noreply.github.com> Date: Wed, 26 Jan 2022 13:36:36 -0800 Subject: [PATCH 7/7] Revert to 'member.ContainingAssembly as SourceAssemblySymbol' --- .../Portable/FlowAnalysis/AbstractFlowPass.cs | 1 - .../FlowAnalysis/DefiniteAssignment.cs | 20 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs index f993702e31bf1..9515734437ab1 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs @@ -187,7 +187,6 @@ protected AbstractFlowPass( bool trackRegions = false, bool nonMonotonicTransferFunction = false) { - Debug.Assert(compilation != null); Debug.Assert(node != null); if (firstInRegion != null && lastInRegion != null) diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs index cbd99d9c2acce..45e47e130cc92 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/DefiniteAssignment.cs @@ -204,16 +204,24 @@ internal DefiniteAssignmentPass( _shouldCheckConverted = this.GetType() == typeof(DefiniteAssignmentPass); } - private static SourceAssemblySymbol GetSourceAssembly( + private static SourceAssemblySymbol? GetSourceAssembly( CSharpCompilation compilation, Symbol member, BoundNode node) { - Debug.Assert(member is null || - node.Kind == BoundKind.Attribute || - (object)member.ContainingAssembly == compilation.SourceAssembly); - - return compilation.SourceAssembly; + if (member is null) + { + return null; + } + if (node.Kind == BoundKind.Attribute) + { + // member is the attribute type, not the symbol where the attribute is applied. + Debug.Assert(member is TypeSymbol type && + (type.IsErrorType() || compilation.IsAttributeType(type))); + return null; + } + Debug.Assert((object)member.ContainingAssembly == compilation?.SourceAssembly); + return member.ContainingAssembly as SourceAssemblySymbol; } protected override void Free()