Speculative semantic model - create child member models for attributes and parameter default values. #64634
Conversation
This could occur when member semantic models were used to support/represent speculative semantic models. This change gets us one step closer to addressing dotnet#60801.
…for attributes and parameter default values. Fixes dotnet#60801. Fixes dotnet#24135.
|
@jasonmalinowski anything in particular you wanted me to hone in on? Overall this looks fine to me :) |
|
@CyrusNajmabadi No; this needed some IDE reviewer and you seemed like a good choice. |
|
Gotcha. LGTM. |
|
@jcouv, @dotnet/roslyn-compiler For the second review. |
|
@jcouv, @dotnet/roslyn-compiler For the second review. |
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp | ||
| { |
There was a problem hiding this comment.
Consider adding xml doc.
nit: extra blank line below #Resolved
There was a problem hiding this comment.
Consider adding xml doc.
I think the name is descriptive enough and captures everything that the doc comment would say.
| namespace Microsoft.CodeAnalysis.CSharp | ||
| { | ||
| /// <summary> | ||
| /// Instances of this <see cref="SemanticModel"/> can be exposed to external consumers. |
There was a problem hiding this comment.
Do we have a comment somewhere explaining why some instances should not be exposed to external customers? #Resolved
There was a problem hiding this comment.
Do we have a comment somewhere explaining why some instances should not be exposed to external customers?
This would violate the design.
There was a problem hiding this comment.
I didn't follow how having a comment (I'm not talking about xml doc) would violate the design.
There was a problem hiding this comment.
I didn't follow how having a comment (I'm not talking about xml doc) would violate the design.
Sorry for the confusion. I was not referring to having a comment, but to exposing other instances to external consumers. Doing that would violate the design.
| (binder: containing.GetEnclosingBinder(attribute.SpanStart), model: containing)); | ||
| } | ||
|
|
||
| private MemberSemanticModel GetOrAddModelForParameter(SyntaxNode node, MemberSemanticModel containing, ParameterSyntax paramDecl) |
There was a problem hiding this comment.
This seem very close or duplicated from SyntaxTreeSemanticModel.GetOrAddModelForParameter. Could we factorize? #Resolved
There was a problem hiding this comment.
Could we factorize?
I considered that, but didn't think the refactoring worth the trouble.
| _parentSemanticModel = parentSemanticModel; | ||
| _position = position; | ||
| _parentSnapshotManagerOpt = snapshotManagerOpt; | ||
| _memberModel = null!; |
There was a problem hiding this comment.
How is this safe? Never mind, this ctor is private, and other constructors that depend on it make sure to initialize _memberModel. #Closed
ghost
added this to the This is a follow-up for #64634
Fixes #60801.
Fixes #24135.