Avoid assuming containing assembly is from source in DefiniteAssignment #59069
Conversation
| @@ -160,7 +156,7 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< | |||
| trackUnassignments) | |||
| { | |||
| this.initiallyAssignedVariables = null; | |||
| _sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly; | |||
| _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; | |||
There was a problem hiding this comment.
What is member in this case? And why is its assembly not from source? #Resolved
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -160,7 +156,7 @@ internal partial class DefiniteAssignmentPass : LocalDataFlowPass< | |||
| trackUnassignments) | |||
| { | |||
| this.initiallyAssignedVariables = null; | |||
| _sourceAssembly = ((object)member == null) ? null : (SourceAssemblySymbol)member.ContainingAssembly; | |||
| _sourceAssembly = member?.ContainingAssembly as SourceAssemblySymbol; | |||
There was a problem hiding this comment.
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
|
Done with review pass (commit 1) |
| (type.IsErrorType() || compilation.IsAttributeType(type))); | ||
| return null; | ||
| } | ||
| Debug.Assert(member.ContainingAssembly is SourceAssemblySymbol); |
There was a problem hiding this comment.
BTW, if we always have compilation, why not simply always grab its SourceAssembly regardless of the node and member?
There was a problem hiding this comment.
Reverting to use member.ContainingAssembly as SourceAssemblySymbol rather than compilation.SourceAssembly since compilation may be null in the EE. (It doesn't seem like we have any tests that hit this case currently, but it seems safer to allow that.)
There was a problem hiding this comment.
since
compilationmay be null in the EE.
This doesn't match the nullable annotations on the parameters. Also, we store compilation in a protected field in the base class and it doesn't look like we protect against null value in it.
ghost
added this to the 
Fixes #57428