Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include a bit in DocumentId to indicate if it corresponds to a SG document or not #69952

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

CyrusNajmabadi
Copy link
Member

Currently, the IDE has to always go down an expensive path when asked for teh Document corresponding to a particular DocumentId. This is because we don't know if the doc-id refers to a real user-documetn or a potential SG document. As such, if we don't find the real document, we have to go generate all the SG docs, even if they may not match teh doc id either.

This changes the internal representation of a DocId to include if it was synthesized to correspond to an SG doc or not. If not, we can shortcircuit those calls immediately, returning that no doc was found at all after hte fast user-code lookup. If it is an sg doc id, then we do go and do the more expensive work like we did before.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner September 14, 2023 18:53
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 14, 2023
ToddGrun
ToddGrun previously approved these changes Sep 14, 2023
Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@ToddGrun ToddGrun dismissed their stale review September 14, 2023 19:08

revoking review

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit 8f8abc3 into dotnet:main Sep 15, 2023
24 checks passed
@ghost ghost added this to the Next milestone Sep 15, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the docIdSG branch September 15, 2023 01:05
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
@JoeRobich
Copy link
Member

@CyrusNajmabadi This PR broke OmniSharp's ability to create DocumentIds for source generated documents. I will expose the new CreateFromSerlialized method from the O# EA, but wondered if perhaps the IsSourceGenerated property could be a nullable bool. In the null case the document id would be treated with the legacy behavior.

@CyrusNajmabadi
Copy link
Member Author

@JoeRobich ick :)

  1. we should have an EA layer for omnisharp if they need this.
  2. omnisharp should def pass along the bit saying if a doc is source generated or not (i didn't even know omnisharp could make source generated docs).
  3. It's def weird for anything other than us to be making source generated docs :)
  4. i don't want a nullable bool here. This is really core functionality that is valuable (we can add a lot of asserts/fast-paths with this bit). I would prefer everything just move onto it.

LMK what you thnik.

@JoeRobich
Copy link
Member

@CyrusNajmabadi It isn't that we are making source generated docs but requesting them for use with GoToDef, etc... (see https://github.com/OmniSharp/omnisharp-roslyn/blob/22424c7a9635e5de0794ad97854ce37e7fdda1ad/src/OmniSharp.Roslyn.CSharp/Services/Navigation/SourceGeneratedFileService.cs#L37). O# has an EA lib and I was planning on updating it. Didn't know if there are potentially other users of this API who might break. grep.app doesn't know of many https://grep.app/search?q=DocumentId.CreateFromSerialized, github finds a few more https://github.com/search?q=DocumentId.CreateFromSerialized+language%3AC%23&type=code&l=C%23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants