Public APIs for opened and closed event handling for non-source documents

[mavasani]

Background and Motivation

We currently have a few workspace level public APIs and events for source documents being opened/closed in the workspace. This issue proposes adding similar APIs and events for non-source documents (additional documents and analyzer config documents).

Current API

Current public APIs for source documents opened/closed events:

Microsoft.CodeAnalysis.Workspace.DocumentOpened -> System.EventHandler<Microsoft.CodeAnalysis.DocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.DocumentClosed -> System.EventHandler<Microsoft.CodeAnalysis.DocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.RaiseDocumentOpenedEventAsync(Microsoft.CodeAnalysis.Document document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.Workspace.RaiseDocumentClosedEventAsync(Microsoft.CodeAnalysis.Document document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.DocumentEventArgs
Microsoft.CodeAnalysis.DocumentEventArgs.DocumentEventArgs(Microsoft.CodeAnalysis.Document document) -> void
Microsoft.CodeAnalysis.DocumentEventArgs.Document.get -> Microsoft.CodeAnalysis.Document

Proposed API

There are 3 possible API shapes that we can use to expose these events for non-source documents.

Proposal 1

---

Replicate each of the above APIs for every non-source document type, i.e. have distinct and duplicated APIs for AdditionalDocument and AnalyzerConfigDocument. This is the API shape chosen in the current PR: https://github.com/dotnet/roslyn/pull/61377/files

# AdditionalDocument APIs

Microsoft.CodeAnalysis.Workspace.AdditionalDocumentOpened -> System.EventHandler<Microsoft.CodeAnalysis.AdditionalDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.AdditionalDocumentClosed -> System.EventHandler<Microsoft.CodeAnalysis.AdditionalDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.RaiseAdditionalDocumentOpenedEventAsync(Microsoft.CodeAnalysis.AdditionalDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.Workspace.RaiseAdditionalDocumentClosedEventAsync(Microsoft.CodeAnalysis.AdditionalDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.AdditionalDocumentEventArgs
Microsoft.CodeAnalysis.AdditionalDocumentEventArgs.AdditionalDocumentEventArgs(Microsoft.CodeAnalysis.AdditionalDocument document) -> void
Microsoft.CodeAnalysis.AdditionalDocumentEventArgs.Document.get -> Microsoft.CodeAnalysis.AdditionalDocument

# AnalyzerConfigDocument APIs

Microsoft.CodeAnalysis.Workspace.AnalyzerConfigDocumentOpened -> System.EventHandler<Microsoft.CodeAnalysis.AnalyzerConfigDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.AnalyzerConfigDocumentClosed -> System.EventHandler<Microsoft.CodeAnalysis.AnalyzerConfigDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.RaiseAnalyzerConfigDocumentOpenedEventAsync(Microsoft.CodeAnalysis.AnalyzerConfigDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.Workspace.RaiseAnalyzerConfigDocumentClosedEventAsync(Microsoft.CodeAnalysis.AnalyzerConfigDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.AnalyzerConfigDocumentEventArgs
Microsoft.CodeAnalysis.AnalyzerConfigDocumentEventArgs.AnalyzerConfigDocumentEventArgs(Microsoft.CodeAnalysis.AnalyzerConfigDocument document) -> void
Microsoft.CodeAnalysis.AnalyzerConfigDocumentEventArgs.Document.get -> Microsoft.CodeAnalysis.AnalyzerConfigDocument

Proposal 2

---

Introduce a single set of APIs for non-source text documents, i.e. have shared APIs for AdditionalDocument and AnalyzerConfigDocument, and any other future non-source document type that we may add.

# NonSourceDocument APIs

Microsoft.CodeAnalysis.Workspace.NonSourceDocumentOpened -> System.EventHandler<Microsoft.CodeAnalysis.TextDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.NonSourceDocumentClosed -> System.EventHandler<Microsoft.CodeAnalysis.TextDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.RaiseNonSourceDocumentOpenedEventAsync(Microsoft.CodeAnalysis.TextDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.Workspace.RaiseNonSourceDocumentClosedEventAsync(Microsoft.CodeAnalysis.TextDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.TextDocumentEventArgs
Microsoft.CodeAnalysis.TextDocumentEventArgs.TextDocumentEventArgs(Microsoft.CodeAnalysis.TextDocument document) -> void
Microsoft.CodeAnalysis.TextDocumentEventArgs.Document.get -> Microsoft.CodeAnalysis.TextDocument

Proposal 3

---

Introduce a single set of APIs for all (source and non-source) text documents, i.e. have shared APIs for Document, AdditionalDocument and AnalyzerConfigDocument, and any other future document type that we may add.

# TextDocument APIs

Microsoft.CodeAnalysis.Workspace.TextDocumentOpened -> System.EventHandler<Microsoft.CodeAnalysis.TextDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.TextDocumentClosed -> System.EventHandler<Microsoft.CodeAnalysis.TextDocumentEventArgs>
Microsoft.CodeAnalysis.Workspace.RaiseTextDocumentOpenedEventAsync(Microsoft.CodeAnalysis.TextDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.Workspace.RaiseTextDocumentClosedEventAsync(Microsoft.CodeAnalysis.TextDocument document) -> System.Threading.Tasks.Task
Microsoft.CodeAnalysis.TextDocumentEventArgs
Microsoft.CodeAnalysis.TextDocumentEventArgs.TextDocumentEventArgs(Microsoft.CodeAnalysis.TextDocument document) -> void
Microsoft.CodeAnalysis.TextDocumentEventArgs.Document.get -> Microsoft.CodeAnalysis.TextDocument

This last proposal will lead to us generating duplicate events for source documents, existing DocumentOpened and DocumentClosed events, plus the new TextDocumentOpened and TextDocumentClosed events. We can consider deprecating the existing APIs/events specific to source documents, if we feel that the duplication would be confusing OR we can retain the existing APIs for clients interested in only source document opening/closing.

[mavasani]

@jasonmalinowski @shyamnamboodiripad @333fred

[jasonmalinowski]

So as far as which option seems best, nothing really jumps out other than thinking about what use cases we see. Chatting with @shyamnamboodiripad he was imagining that they only needed to track additional files for Razor scenarios and .editorconfig files were unimportant since they're not running anlayzers. However, technically .editorconfig files can be read by a generator which could change the generated output too, so I think any handling they do for additional files may also apply to .editorconfig files too, although I won't know the fuller details.

@shyamnamboodiripad Does that sound right or did I mangle something here?

[shyamnamboodiripad]

Yes @jasonmalinowski, that is correct.

I don't have a strong preference between the options. However, I like Proposal 3. It seems like a good longer term direction to have a single API that can handle all types of documents especially if we deprecate the current DocumentOpened / DocumentClosed APIs. (Even otherwise, the duplicate notifications should be easy to explain with good documentation comments which explain that DocumentOpened and DocumentClosed are preserved for backwards compatibility and that all new usages should prefer the new TextDocumentOpened and TextDocumentClosed.)

If we can't choose Proposal 3, I would prefer Proposal 1 over Proposal 2. Proposal 2 treats non-source documents specially and may be problematic if we end up introducing a new document type in the future that is not a Document but could still somehow be considered a 'source' document. It would also be preferrable to avoid the need for a new categorization of documents (source v/s non-source) that is only relevant in this API, but is not relevant in other parts of the Roslyn API surface.

Proposal 1 would force us to introduce new APIs for every future document type (which we hope will never happen :)). However, the resulting APIs would always be intuitive to consumers. This is probably also the easiest to explain alternative amongst the three options above.

[shyamnamboodiripad]

@mavasani @jasonmalinowski I just discovered a couple of things as I was trying out the internal API and thought I would mention them here for consideration.

1. The TextDocumentKind enumeration which can be used to differentiate between different kinds of TextDocuments is currently public. However, looks like TextDocument.Kind property is currently internal. As part of the above changes, it would be great to make this public too.
2. Workspace exposes a couple of APIs GetOpenDocumentIds and IsDocumentOpen both of which operate on DocumentId. However, it is not obvious that these APIs also support AdditionalDocuments AnalyzerConfigDocuments in addition to Documents. At the very least, it would be great to update the documentation comments.

Nice to haves: The following may be out-of-scope for current proposal but would be nice to support so that it is possible to do a little more with AdditionalDocuments and AnalyzerConfigDocuments -

1. It is currently not easy to determine the document kind (e.g., razor v/s something else) for non-source documents. I am guessing this is by design because this information is extraneous and generally not useful for Roslyn-based analysis. However, it would be great to support some kind of (open ended) property bag / tags API on TextDocument that could be populated with this information.
2. There are some other public APIs like Workspace.GetDocumentIdInCurrentContext, Workspace.GetRelatedDocumentIds as well as corresponding public extension methods that translate from text buffers and text snapshots to Documents (such as GetOpenDocumentInCurrentContextWithChanges, GetRelatedDocuments etc.). I am not sure how applicable these would be for AdditionalDocuments and AnalyzerConfigDocuments (or whether some of them already work for non-source documents), but it would be nice to have something similar. (Update: Looks like Workspace.GetDocumentIdInCurrentContext supports non-source documents already! So, we are only missing the extension methods for text buffers etc. which seems lower priority. This may be another case where updating the documentation comments would be useful 😄).

[chsienki]

API Review Notes

• Proposal 3 is accepted as-is.
• We won't deprecate the existing APIs (yet) as it seems useful for consumers who aren't interested in non source texts.

[jasonmalinowski]

@shyamnamboodiripad To your concerns there we should get follow-ups on those; we didn't think it in any way impacted this API design.

It is currently not easy to determine the document kind (e.g., razor v/s something else) for non-source documents. I am guessing this is by design because this information is extraneous and generally not useful for Roslyn-based analysis. However, it would be great to support some kind of (open ended) property bag / tags API on TextDocument that could be populated with this information.

What sort of information did you want to imagine and where would it come from? Note that since we do serialization/cross process sync we'd have to think carefully about the types involved here.

However, it is not obvious that these APIs also support AdditionalDocuments AnalyzerConfigDocuments in addition to Documents. At the very least, it would be great to update the documentation comments.

I would expect they do, were you able to confirm that?

[shyamnamboodiripad]

To your concerns there we should get follow-ups on those; we didn't think it in any way impacted this API design.

@jasonmalinowski At the very least it would be great to expose the TextDocumentKind since the enum itself is already public.

What sort of information did you want to imagine and where would it come from? Note that since we do serialization/cross process sync we'd have to think carefully about the types involved here.

The main thing I was interested in knowing for my feature is whether the document in question is a razor file (i.e., ContentType). I would imagine that would be populated by whichever component creates the document. Without knowing the ContentType I either have to rely on file extension OR handle non-razor files the same way as razor files (which is ok for my use case - but may not be ok more generally).

I would expect they do, were you able to confirm that?

Yes, as I hinted in an "Update" to my comment above, turns out most of the APIs I listed above do support AnalyzerConfigDocuments and AdditionalDocuments. It is just that that is not obvious from the doc comments. So, adding more details to the comments may be all that may be needed for this.

[CyrusNajmabadi]

Closing out due to lack of movement.