Why the switch to SemanticModel?

➡️ AnalyzerRunner was going to need it anyway. Document wasn't providing much. It was mentioned in earlier review comments, and I knew the change was eventually needed, so I implemented it.

I guess can the docs say if includeMethod is false, then methodName should be ignored? I guess I'm not sure if "can be document separately" is really helping the clarity here.

We don't know the signature to use until Signature Help tells us.

Exactly -- this method isn't running until you know the signature, but by this you can just directly put the values in.

but it increases coupling and would be even more difficult to understand.

I'd say this is far more coupled and more difficult to understand, since there's an implicit handshake between this code and the function implementation.

Any reason this can't just be Clear()? Yeah, it'll clear the value you're overwriting in the next line, but would mean it's more impervious to any other refactoring where somebody puts some other non-symbol state in that matters.

➡️ The expansion session is still active at this point. Clear would not accurately reflect the state.

Which session? It's not starting until the next line, right? If there was a prior session it seems like we aren't caring about it at this point?

It's possible I'm just missing the scenario here -- I would have presumed this method is only called on the snippet picker UI and so at that point any session going away is moot, and there's nothing to preserve because it's not going to be a method expansion snippet being invoked.

The presence of an active snippet session does not prevent the snippet picker from working, so while it's not common for this case to be hit with an active non-null session it is possible. Even if it's valid to clear the active session while it remains active, it would require substantially more explanation as to why doing so is OK.

In other words, among the available options for clearing state here, none of them is obviously correct and this one is the least confusing.

We feel confident there's no S_FALSE returns? 😄

➡️ The documentation for IVsExpansionSession.GetFieldValue does not allow for S_FALSE returns, and the current code worked in practice.

It feels like this really needs a comment here somewhere to explain the intent of this. It's...not obvious.

Also making a constant out of "placeholder" ("PlaceholderForEmptyArguments"?) would make this a bit less magic.

➡️ will document and create a field

Now in https://github.com/dotnet/roslyn/compare/57a96019a9c04f7006d206d0c61c1f33a060b5fa..41a26127bdaa8225c0bd8c705f1878feb88e3398

Want to at least change the comment since "It's unclear if/how this state would occur" isn't the case?

So I don't really see how _methods is being used here -- I see it being used in IsFullMethodCallSnippet but it seems like can just be a boolean state of whether we're in this mode or not?

➡️ It probably could be changed, but this is going to have to change anyway as part of nested call support. Leaving it for now since changes in this space have caused undesired "iteration" churn (subtle fallout) that I don't want to go through again knowing the current form works.

Oh I think I see the bit I might be missing: if you do tab + tab on a method name we'll stick in parenthesis, we stick the methods here to recognize the name later. And this line:

var symbolName = _state._method?.Name ?? _state._methods.FirstOrDefault()?.Name;

Is just doing FirstOrDefault() because all the methods in the group have the same name? Yeah, this is really confusing. 😦

Is this just a paranoia check that we don't imagine this could happen, or can this happen in some way?

When is that coming?

So it's not clear to me why _preserveSymbols is necessary here -- you're already holding onto the local to assert the symbols aren't changing, why not just instead just reassign the _state? Then you can get rid of _preserveSymbols?

The API is reentrant (other methods in this class run during a deeply-nested call stack) and eventually end up back here. _preserveSymbols keeps those nested calls from changing state they aren't supposed to touch.

So as best I can tell from the review is the reentrancy concern here is when you call insert specific the prior session will be dismissed, which if we don't set that wipes out state. But what other call backs are happening that need the state? Is it just the snippet functions which we think can be removed?

Which session? It's not starting until the next line, right? If there was a prior session it seems like we aren't caring about it at this point?

It's possible I'm just missing the scenario here -- I would have presumed this method is only called on the snippet picker UI and so at that point any session going away is moot, and there's nothing to preserve because it's not going to be a method expansion snippet being invoked.

Tracking item?

Oh I think I see the bit I might be missing: if you do tab + tab on a method name we'll stick in parenthesis, we stick the methods here to recognize the name later. And this line:

var symbolName = _state._method?.Name ?? _state._methods.FirstOrDefault()?.Name;

Is just doing FirstOrDefault() because all the methods in the group have the same name? Yeah, this is really confusing. 😦