Use document with partial semantic for completion

[genlu]

Closes #51407

[genlu]

@sharwell @CyrusNajmabadi @jasonmalinowski Please take a look. thanks!

[jasonmalinowski]

So it's a bit worrisome for me that we're throwing away the SemanticModel here. If the full Compilation was available and we didn't actually do the freeze operation, the fact we've thrown away the SemanticModel could mean that theoretically the SemanticModel and Compilation could GC, and then the request for the SemanticModel has to do extra work. It won't run generators. Put another way, throwing away the SemanticModel here is potentially undoing the optimization that GetPartialSemanticModelAsync is trying to do in the first place. This would require a pretty badly timed GC though.

I'm suspicious of GetPartialSemanticModelAsync in general so maybe we should just delete it if that seems easier, but I'm happy for that to be done in another PR.

[genlu]

That's a good point, I will hold on to the reference to returned model until completion providers finished their work

[genlu]

and I will leave cleaning up GetPartialSemanticModelAsync to a follow up PR

[jasonmalinowski]

You don't still want a partial semantic model in this case? Note that a "partial" model is different than the "speculative" model that the comment mentions; the speculative guarantees the tree you have will be up to date for the file, which won't run into what I think that comment is worried about.

If you really do still need the full model, then revise the comment since the comment doesn't match the code anymore.

[genlu]

The document passed in is frozen, so I'd think GetRequiredSemanticModelAsync would return partial model, right? Providers don't know if they are getting full model or partial model, so regular here is in contrast to speculative, and I believe the comment is still accurate in that sense

[jasonmalinowski]

Same comment as C# side.
#55745 (comment)

[jasonmalinowski]

@CyrusNajmabadi Any concern how this will interact with the work you did in #45565? It's news to me that we had stopped using partial semantics in completion that this PR is fixing.

[jasonmalinowski]

@CyrusNajmabadi More generally, it occurred to me that the optimization the speculative semantic model is doing is technically broken in the face of source generators, if you have a generator producing new top-level types based on code written inside a method body, it's now the case that a method body edit can impact top-level semantics. I'm not sure if that's actually a real-world concern though, or at least common enough to warrant adjusting that somehow.

[jasonmalinowski]

You need to GC.KeepAlive this.

[sharwell]

Oh, interesting... Let me try someting.

[sharwell]

I'd like to see this:

Suggested change

	(document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);
	(document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);
	using var _ = new KeepAlive(semanticModel);

Should be easy with this helper type:

internal readonly struct KeepAlive : IDisposable
{
    private readonly object? _instance;

    public KeepAlive(object? instance)
        => _instance = instance;

    public void Dispose()
        => GC.KeepAlive(_instance);
}

[genlu]

        //     References the specified object, which makes it ineligible for garbage collection
        //     from the start of the current routine to the point where this method is called.

Based on the doc, the helper type only guarantee _instance to be GC ineligible after Dispose is called at the end of current method, right? It can still get GC'd before then, unless Dispose is inlined

[sharwell]

The call to Dispose will occur immediately before returning, which is where we want the call to KeepAlive placed.

[genlu]

from the start of the current routine

Wouldn't that make current routine be Dispose instead in this case?

[genlu]

Any concern how this will interact with the work you did in #45565?

The partial semantic snapshot has different version, so we are good here. See test added in
2c3e6bb

[jasonmalinowski]

It seems like this code generally needs a cleanup to be a bit more consistent with how we're acquiring documents and stuff, but this will work.