Cloning DocumentId results in impossibility to get the valid result of GetSourceGeneratedDocumentAsync

[lsoft]

Version Used: Latest.

Steps to Reproduce:

1. have in hand a real DocumentId of source-generated document.
2. make sure GetSourceGeneratedDocumentAsync returns a valid SG-document with this DocumentId.
3. clone DocumentId via public-available constructors. (this is inevitable in some scenarios, like working with VS codelenses).
4. make GetSourceGeneratedDocumentAsync returns null.

I suspect it is because DocumentId.IsSourceGenerated is internal, and cannot be set outside of Roslyn.

Workaround exists: (await project.GetSourceGeneratedDocumentsAsync(CancellationToken.None).First(d => d.Id.Equals(clonedDocumentId));. But it is a VERY VERY confusion behavior.

Expected Behavior:

Cloning DocumentId does not break GetSourceGeneratedDocumentAsync.

Actual Behavior:

GetSourceGeneratedDocumentAsync returns null.

[sharwell]

this is inevitable in some scenarios, like working with VS codelenses

Can you clarify this?

[CyrusNajmabadi]

Note: the design here was intentional. No other parties can make source-generated-docs, so there should be no need to create the IDs for them. I too would need more info on why cloning is needed or is happening.

[lsoft]

@sharwell

Can you clarify this?

working with codelenses inside VSIX means one need to serialize data between processes (main VS process, and some codelens process, I don't know details), so project guid and document guid from "valid" DocumentId are both serialized in order to send into codelens (or back), then new instance DocumentId is created and VSIXer can't grab source generated document with this "cloned" DocumentId as described.

There is also some theoretical (at now) scenarios where DocumentId (its project+document guids) will be saved to the disk, or some small db inside the VSIX.

do I provide enough details? if no, please, give me a hint what you want to know exactly!

@CyrusNajmabadi

could I ask for explanation for the following question: source generated documents are here for years, and they will not disappear, it is big part of Roslyn ecosystem and we all are happy to see it; so, why they are not a part of public API? why Roslyn team do not want to see it as a part of public API? we are talking about one bool inside DocumentId, which we know will not lose its value in the future.

Thanks!

[lsoft]

The issue I described is a real issue for my VSIX https://github.com/lsoft/SyncToAsyncExtension.

[sharwell]

@CyrusNajmabadi We need default the IsSourceGenerated to true for the existing public constructors for back compat (this will not impact the code paths that request a non-source generated document), and optionally create a new public constructor that exposes IsSourceGenerated so it can be manually set to false.

This is a 17.8 regression introduced by #69952.

[CyrusNajmabadi]

so, why they are not a part of public API?

Because this may not be the representation we want in the future. Making something public means we cannot change it.

[CyrusNajmabadi]

We need default the IsSourceGenerated to true for the existing public constructors

I don't think this is appropriate.

What we could do is make the internal representation tri-state. So the existing constructors would say "I don't know" (versus saying that it actually is a generated document).

[lsoft]

@CyrusNajmabadi

Because this may not be the representation we want in the future. Making something public means we cannot change it.

I understand this. But please take a look at this topic from the user's persprective.

From user's perspective, user need to grab a Document having in a hand a DocumentId. Now one need to write this:

            var document = project.GetDocument(
                documentId
            );
            if(document != null)
            {
                return document;
            }

            document = (await project.GetSourceGeneratedDocumentsAsync(CancellationToken.None))
                .First(d => d.Id.Equals(documentId));
            return document;

after fixing this issue this code will be transformed into the following:

            var document = project.GetDocument(
                documentId
            );
            if(document != null)
            {
                return document;
            }

            document = (await project.GetSourceGeneratedDocumentAsync(documentId, CancellationToken.None);
            return document;

but having bool IsSourceGenerated will result into a more efficient code like:

       if(!documentId.IsSourceGenerated)
            return project.GetDocument(
                documentId
            );
      else
           return (await project.GetSourceGeneratedDocumentAsync(documentId, CancellationToken.None);

This is, probably, good.

[CyrusNajmabadi]

@lsoft i'm not saying i don't get the use cases. I'm saying that public APIs come with significant costs. Especially in the case where we decide later it's not the representation we want. As an internal API we can trivially change things, as a public API, it is much much harder. :)

[CyrusNajmabadi]

I'm amenable to do this in two phases. Revert the regression (by making the existing constructor initialize things in a tri-state). Then in the future decide on a public, supported, representation.