if we have an open file, will this give the itextsnapshot for that open file? if so, we can map that right to a preparsed roslyn document.

Yes, it should. But there's no guarantee that this is an open file.

i def get that. but we can easily try to get the corresponding doc if it is open, and fall back to this approach if it is not.

that said, if it's a closed file, i would far prefer we get passed a path. That way we can see if we already have the doc, and the parsed tree, for it.

we ccan always fall back to this. but i would prefer we do everything we can to avoid parsing for files we have already.

That seems like it'd be incorrect in some cases - what guarantees you'll be using the same text as the snapshot we're passing to you?

After chatting with Kayle, we've decided not to change the API. Instead, if you find that you need the file path and can use it safely, we can add it into the snapshot's buffer's property bag.

That works!

Unresolving this since I saw this after replying to the other comment: why aren't we getting an ITextDocument then or something to better pass along the file name? Also should the snapshot be an ITextImage?

question about this api in a general sense. does the UI block on getting this information? or is it added lazily to the UI once computed?

i'm very wary about slowing down the UI with something potentially expensive.

As Find in Files is implemented today, this is part of a pipeline that blocks on a file-by-file basis.

EDIT: Nope, Kayle is right. For the Find case, it is in fact happening on the threadpool. For the Replace All case, though, it is currently on the UI thread.

given that, i would consider even going simpler here and looking at the line and ignoring it if the line doesn't start with <spaces>//<spaces> or <spaces>*<spaces>. Yes, this is slightly inaccurate. But it's a good filter to avoid unnecessary work.

There's multiple tasks running on the threadpool for this - it won't block the UI thread

Kayle is right, edited my comment for accuracy.

skipped to this point.

from a design perspective, i would like us to have the roslyn side not be c# specific. this should work for VB as well, and it would not be hard to make that work, given how little of this is lang specific. i can help with that.

Thanks, I was mostly hoping to just dump this prototype in the Roslyn team's lap and let them do whatever they want with it.

The feedback I'm mostly looking for is on the span annotator API itself. This is based on discussions we had a few months ago, and the outcome of design review, but I'm still open to tweaks.

that works for me.

i think we should pass the filename. it just seems like we should let the LS use information it has if it has it. and only pay the tokenization cost if the file really isn't in memory, and the ITextSnapshot is what we should be using.

This is the more concerning call. Can we parse tokens from a snapshot directly instead of allocating a giant string?

Not at computer. But if this is creating a string, that's bad. On the other hand, we have helpers to wrap an ITextSnapshot with a SourceText, which is waht we really want to do here.

Note though that i strongly think that we should not just tokenize the entire file to find the matching token for a particular span in teh file. We should use heuristics first to only do this work if we have a strong indication that we're in a comment. In the majority of cases, it would likely be fine.

Yeah I think that's fine. I'm more concerned with false positives than false negatives here (that is, I think users will be more annoyed if we erroneously mark a result as being a comment/string, rather than the other way around).

Is this code path only taken for all files found in Find in Files, or only after we've already determined we don't have the information another way? Because it's quite unfortunate that we have lost file name information here, since it's entirely possible we already have this tree parsed and ready to go if you're using find in files while a solution is open.

We'll have to sync with @dibarbet since this will have some impact to our integration tests. At the moment while I write this they're not running, but this would delay their re-enabling further. I'm not taking any position on what we want to do here, just want to call out the potential conflict.

General FYI we have an experimental attribute we might start to be able to use for this.

No concept of content types being used here? Not saying that's bad, it's just another model (unless we have other prior art and I missed the memo).

Is this code path only taken for all files found in Find in Files, or only after we've already determined we don't have the information another way? Because it's quite unfortunate that we have lost file name information here, since it's entirely possible we already have this tree parsed and ready to go if you're using find in files while a solution is open.

Unresolving this since I saw this after replying to the other comment: why aren't we getting an ITextDocument then or something to better pass along the file name? Also should the snapshot be an ITextImage?