Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid assuming containing assembly is from source in DefiniteAssignment #59069

Merged
merged 7 commits into from
Jan 27, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -59,20 +57,18 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass<
/// <summary>
/// Some variables that should be considered initially assigned. Used for region analysis.
/// </summary>
private readonly HashSet<Symbol> initiallyAssignedVariables;
private readonly HashSet<Symbol>? initiallyAssignedVariables;

/// <summary>
/// Variables that were used anywhere, in the sense required to suppress warnings about
/// unused variables.
/// </summary>
private readonly PooledHashSet<LocalSymbol> _usedVariables = PooledHashSet<LocalSymbol>.GetInstance();

#nullable enable
/// <summary>
/// Parameters of record primary constructors that were read anywhere.
/// </summary>
private PooledHashSet<ParameterSymbol>? _readParameters;
#nullable disable

/// <summary>
/// Variables that were used anywhere, in the sense required to suppress warnings about
Expand Down Expand Up @@ -105,12 +101,12 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass<
/// <summary>
/// The current source assembly.
/// </summary>
private readonly SourceAssemblySymbol _sourceAssembly;
private readonly SourceAssemblySymbol? _sourceAssembly;

/// <summary>
/// A set of address-of expressions for which the operand is not definitely assigned.
/// </summary>
private readonly HashSet<PrefixUnaryExpressionSyntax> _unassignedVariableAddressOfSyntaxes;
private readonly HashSet<PrefixUnaryExpressionSyntax>? _unassignedVariableAddressOfSyntaxes;

/// <summary>
/// Tracks variables for which we have already reported a definite assignment error. This
Expand All @@ -136,7 +132,7 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass<
/// <summary>
/// The topmost method of this analysis.
/// </summary>
protected MethodSymbol topLevelMethod;
protected MethodSymbol? topLevelMethod;

protected bool _convertInsufficientExecutionStackExceptionToCancelledByStackGuardException = false; // By default, just let the original exception to bubble up.

Expand All @@ -151,7 +147,7 @@ internal DefiniteAssignmentPass(
BoundNode node,
bool strictAnalysis,
bool trackUnassignments = false,
HashSet<PrefixUnaryExpressionSyntax> unassignedVariableAddressOfSyntaxes = null,
HashSet<PrefixUnaryExpressionSyntax>? unassignedVariableAddressOfSyntaxes = null,
bool requireOutParamsAssigned = true,
bool trackClassFields = false,
bool trackStaticMembers = false)
Expand All @@ -160,7 +156,7 @@ internal DefiniteAssignmentPass(
trackUnassignments)
{
this.initiallyAssignedVariables = null;
_sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly;
_sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol;
Copy link
Member

@333fred 333fred Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is member in this case? And why is its assembly not from source? #Resolved

Copy link
Member Author

@cston cston Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The failure case is an expression within a lambda used as an attribute argument. The member in that case is the attribute (obtained from SyntaxTreeSemanticModel.GetMemberModel(expression)) where the attribute type is from metadata.

Copy link
Member

@333fred 333fred Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fascinating. Can we add an assert that it's basically either this case, or from source (or the member is null altogether)? It feels like this could potentially hide a bug later on if another case slipped in here.

Copy link
Contributor

@AlekseyTs AlekseyTs Jan 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

member?.ContainingAssembly as SourceAssemblySymbol

It feels like we might want to strengthen the logic here to get ContainingAssembly only from a symbol that is actually a member or belongs to a member. Otherwise, we might still get a SourceAssemblySymbol that doesn't match the context and start changing its state based on the analysis that we perform. #Closed

_unassignedVariableAddressOfSyntaxes = unassignedVariableAddressOfSyntaxes;
_requireOutParamsAssigned = requireOutParamsAssigned;
_trackClassFields = trackClassFields;
Expand All @@ -175,11 +171,11 @@ internal DefiniteAssignmentPass(
BoundNode node,
EmptyStructTypeCache emptyStructs,
bool trackUnassignments = false,
HashSet<Symbol> initiallyAssignedVariables = null)
HashSet<Symbol>? 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;
Expand Down Expand Up @@ -237,6 +233,7 @@ protected override int AddVariable(VariableIdentifier identifier)
return slot;
}

#nullable disable
protected Symbol GetNonMemberSymbol(int slot)
{
VariableIdentifier variableId = variableBySlot[slot];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<LiteralExpressionSyntax>().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<LiteralExpressionSyntax>().Single();
var analysis = model.AnalyzeDataFlow(expr);
Assert.False(analysis.Succeeded);
}

#endregion

#region "Used Local Functions"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
}
}");
}
}
}