VB: Align design around speculative semantic models with C#. #64751
Conversation
| @@ -129,7 +129,7 @@ internal sealed class SpeculativeSemanticModelWithMemberModel : PublicSemanticMo | |||
|
|
|||
| public override bool IgnoresAccessibility => _parentSemanticModel.IgnoresAccessibility; | |||
|
|
|||
| internal sealed override SemanticModel ContainingModelOrSelf => this; | |||
| internal sealed override SemanticModel ContainingPublicModelOrSelf => this; | |||
There was a problem hiding this comment.
Feels like this should be moved up into PublicSemanticModel and sealed there. #Pending
| Debug.Assert(semanticModel.ContainingModelOrSelf.ContainingModelOrSelf == semanticModel.ContainingModelOrSelf); | ||
| } | ||
| Debug.Assert(semanticModel.ContainingPublicModelOrSelf != null); | ||
| Debug.Assert(semanticModel.ContainingPublicModelOrSelf != semanticModel); |
There was a problem hiding this comment.
nit: Is it actually important that this not be a public semantic model? #Resolved
There was a problem hiding this comment.
Is it actually important that this not be a public semantic model?
I think implementation actually depends on that somewhere. I don't remember exact details, but, for example, operation nodes are cached by the internal semantic model. In any case, this was a legacy behavior and I do not intend to change it in this PR.
|
@dotnet/roslyn-compiler For the second review |
|
|
||
| Private Sub New(parentSemanticModel As SyntaxTreeSemanticModel, | ||
| speculatedPosition As Integer) | ||
| Debug.Assert(Not parentSemanticModel.IsSpeculativeSemanticModel, VBResources.ChainingSpeculativeModelIsNotSupported) |
There was a problem hiding this comment.
Nit: I don't think we should use a localized resource for a debug.assert. If it fails on the spanish test leg, we'll get a spanish assert message.
There was a problem hiding this comment.
Nit: I don't think we should use a localized resource for a debug.assert. If it fails on the spanish test leg, we'll get a spanish assert message.
This is an adjusted copy of an original assert (definitely impossible conditions were removed).
Debug.Assert(parentSemanticModelOpt Is Nothing OrElse Not parentSemanticModelOpt.IsSpeculativeSemanticModel, VBResources.ChainingSpeculativeModelIsNotSupported)
I'll leave the code as is, it is unlikely the assert will fail only on spanish leg, and, even if that happens, there will be a call stack clearly pointing at the root cause. Most of our asserts do not include any clarifying messages.
ghost
added this to the 
This is a follow-up for #64634