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

Cohost Razor and Roslyn to better share object types and object instances #9519

Open
phil-allen-msft opened this issue Nov 1, 2023 · 0 comments

Comments

@phil-allen-msft
Copy link
Contributor

phil-allen-msft commented Nov 1, 2023

Individual endpoint status: #10364

In no particular order:

  1. Moving off IDynamciFileInfo
  2. Razor calling Roslyn via C# API
  3. Pull data from SG
  4. Miscellaneous files, and custom or non-Razor SDK usage and how that could work
    • Scenario is projects that don’t reference the source generator, and don’t have .cshtml and .razor files as AdditionalDocuments.
    • Can we have a service that allows customization/custom creation of a project in Misc Files workspace – Razor could add additional document and source generator reference
    • Could Project System lie to Roslyn and add an additional file, instead/as well as dynamic
      • Doesn't help in the C# extension without devkit, where that code doesn't exist
      • Doing it in Roslyn would solve for DevKit and non
      • We could however do it through https://github.com/dotnet/project-system/blob/main/src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/DesignTimeTargets/Microsoft.CSharp.DesignTime.targets (or equiv in VS Code)
        • We can add .razor and .cshtml as additional documents, and we can reference the source generator, and Roslyn and everything else will be none the wiser
        • Need to ensure nothing conflicts with the real Web and Razor sdks, and what they do
        • Should also create a <DefaultRazorFileIdeSupport>false</DefaultRazorFileIdeSupport> get-out-of-jail-free property that people can specify to disable the behaviour if it causes problems
          • At the end of the day, people who have "weird" set ups can solve their issues with MS Build goo (add additional documents, reference source gen), but the upgrade experience is bad. Is it better if we do that for them, in a way that only affects design time, and give them an opt-out if it causes issues?
    • Could Roslyn just add all dynamic files as additional documents?
      • Needs to be sensitive to duplicates
      • Gets difficult to thread things through, as projects change SDKs and people modify things.
      • Using misc files is easier as every individual file is isolated. Is that an acceptable tooling experience though?
    • We can hook into miscellaneous file creation easily enough:
      • Currently every individual misc file in Roslyn gets its own project, logic is here which is called from here
      • These methods won't be called for .razor files now, because Roslyn doesn't own the buffer, so we'd have to call them through EA.
      • Possibly just as easy for us to create our own misc files project(s), with the right references etc., and expose a way to add them to the Roslyn misc workspace. As long as the LSP server can find them again, the rest shouldn't really matter
      • Will need separate hooks for VS Code
      • Won't help if a file is an AdditionalDocument, but the source generator isn't referenced
  5. Dynamic registration
  6. Legacy Editor
    • Still needs Razor Project System?
      • ANSWER: Yes
    • Still needs IDynamicFileInfo?
      • ANSWER: Yes, for background compilation so local tag helpers are found
      • To discuss: Suppress generator in Legacy only, but not LSP?
        • Also needed when Cohost feature flag is off
  7. Faults and telemetry etc.
    • Need to make sure faults from cohost end points can be routed to the Razor team easily
      • Currently being reported by the Roslyn LSP server
      • Should Razor report them too?
      • Can we have a mechanism where Roslyn knows to categorize as razor (LSP request language name?) or override the fault event name itself?
  8. Roslyn options
    • They don't sync to OOP automatically sadly, so code in OOP always gets defaults
    • Roslyn uses callbacks for getting options values on demand. Razor services don't have callbacks, and if they did, they couldn't get the Roslyn options
    • To Discuss: Can we create a generic system so our EA can fill in options somehow?
@ghost ghost added the untriaged label Nov 1, 2023
@phil-allen-msft phil-allen-msft added this to the 17.9 Planning milestone Nov 2, 2023
@ghost ghost removed the untriaged label Nov 2, 2023
davidwengier added a commit that referenced this issue Jan 4, 2024
First part of #9519
Razor side of dotnet/roslyn#70819

This PR consumes the new Cohost server from Roslyn, and when enabled,
moves DocumentColor requests from being handled by our existing Razor
Language Server, to instead be handled by the new server. There is
currently no sharing of project system data or anything like that yet. A
few TODOs for follow ups, ~can't be merged, and indeed won't build,
until a Roslyn package is available etc. but~ it's reviewable while we
keep working, and more important its shippable in its "off" state.

~Ignore the last commit, obviously won't be merged :)~
davidwengier added a commit that referenced this issue Jan 5, 2024
Part of #9519

Now that the initial cohost PR has been merged, we don't need to use the
feature branch any more. That means this PR, which is big, but all of
the code in it has been reviewed, in
#9761,
#9766 and
#9767.
davidwengier added a commit that referenced this issue Jan 26, 2024
…9831)

Part of #9519

Right now code generation, in co-hosting and non, comes from document
snapshots in the Razor project system which directly call the Razor
compiler. This change means that all Html and C# code generation in
co-hosting is done based on the TextDocument, which means it will come
from the source generator when that part is ready.

This is the Razor side of, and needs a Roslyn bump to,
dotnet/roslyn#71671
davidwengier added a commit that referenced this issue Jan 30, 2024
Part of #9519

As per our meeting the other day, there are a few things on
`IProjectSnapshot` that are problematic. This PR resolves all of the
issues in one of two ways:

* For `Configuration`, `GetProjectEngine()` and `ProjectWorkspaceState`
the cohost implementations now simply throw
* There was some usage of these inside cohosting, because it re-used
methods on `DocumentState` to do jobs that the source gen will
eventually take over. To make this clearer, I modified the helper
methods so they took the underlying data, rather than pulling it from
`IProjectSnapshot` directly
* Changes the `TagHelpers` property into a
`GetTagHelpersAsync(CancellationToken)` method
* Because of the virality of async, I recommend reviewing this PR
commit-at-a-time, just so this last commit is seen separately, for your
sanity.
* There were only two places where moving to async would have been very
very annoying, so I added a `GetTagHelpersSynchronously()` extension
method that validates that its not called from cohosting. It's only
called from legacy editor code.

Note that this isn't the proposed solution in the aforementioned
meeting, that change can still come later. I was hoping to take that
idea and use DI to enforce something better than just a runtime check,
but it turned out that wasn't possible because some MEF services need
these properties in non-cohosting. This PR at least introduces the
runtime check, so we can continue moving forward.

Integration tests are running and should show up in in the checks list.
davidwengier added a commit that referenced this issue Feb 21, 2024
I'm mainly putting this up so @DustinCampbell can tell me what I did
wrong :)

As part of cohosting we're going to need a lot more things to run in
OOP, so its probably best to move away from these hand-constructed
services that it has. This PR introduced MEF, in a reasonably
unsophisticated way, into the procedure so that services can at least
get some things from an `ExportProvider`. Once in that world, obviously
everything works as expected with attributes etc.

Along the way I also found that our telemetry reporting in OOP wasn't
doing anything, so fixed that and moved it to the MEF composition too.
Also improved the `launchSettings.json` file a little, and found it was
previously in the `.gitignore`. This seemed wrong to me, but maybe
@dotnet/razor-compiler will have something different to say about that?
I can always move the compiler one back in if necessary.

Part of #9519
davidwengier added a commit that referenced this issue Mar 6, 2024
The next PR that I'm working on for cohosting is getting to be too big
to review, but cohosting is in a weird place where we can't really have
the old and the new co-existing, so lets start with a clean slate.

Part of #9519
davidwengier added a commit that referenced this issue Jul 10, 2024
Part of #9519

Finally some cohosting tests! Some people would say that I'm being lazy
in adding tests for the simplest endpoint we have. Those people have a
point.

The test infra here almost entirely avoids ServiceHub, and certainly
avoids the Roslyn solution sync mechanism, but it _does_ use our real
services and service factories, including the OOP services' separate MEF
composition, so the services themselves are partying on a real Roslyn
(test) solution and are using real implementations of all of their
dependencies.

Things not covered by this test infra yet:
* OOP initialization
* Adding generated C# files to the Roslyn solution (IDynamicFile does
this in real life)
* Dealing with Html documents in any way

Future PRs to add tests for more endpoints should add these as needed.
davidwengier added a commit that referenced this issue Jul 17, 2024
Part of #9519 and
#10603

This was fairly straight forward too, though adding MVC files to the mix
found a bug in our test data, which is kind of humorous.

I decided not to copy all of the semantic tokens tests we have, but
rather just create something reasonably all-encompassing. The core
engine is shared so both sets of tests exercise it anyway.
davidwengier added a commit that referenced this issue Jul 17, 2024
Part of #9519 and
#10603

This time I did copy the existing tests, because everything was inline
so it was easier.
davidwengier added a commit that referenced this issue Jul 18, 2024
Part of #9519 and
#10603
Requires dotnet/roslyn#74402

Removes a little more dodginess in the cohosting tests by actually using
the `RazorPinnedSolutionInfoWrapper` for solution checksums, just like
the real OOP services.
davidwengier added a commit that referenced this issue Jul 19, 2024
Requires dotnet/roslyn#74280, and won't even
compile without it.
Part of #9519

This brings signature help to Cohosting. It's a pretty simply PR, for a
pretty simple endpoint, as we just delegate, and there is no translation
of delegated info. The interesting part here is that we use
`System.Text.Json` for the remote signature help service, because it
makes more sense to take advantage of the existing Json converters for
the potential complexity of the `SignatureHelp` result type.
davidwengier added a commit that referenced this issue Jul 19, 2024
Part of #9519 and
#10603

After this, all cohost endpoints have test coverage 😁
davidwengier added a commit that referenced this issue Jul 23, 2024
Part of #9519

Brings document highlight to cohosting, including tests. Also added some
basic tests for `RazorServices` and `Services.props`.
davidwengier added a commit that referenced this issue Jul 31, 2024
Part of #9519
Needs dotnet/roslyn#74548 before it will compile
Needs
https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/567229
before it will work in VS

There were a few side quests on this one:
* Roslyn OOP, at least the way we access it, doesn't have any options
set, so took a few tries to get the Roslyn side of this right for our
needs
* I wrote this feature test-first so only discovered the lack of options
after I modified Roslyn and our test infra to allow us to set global
options, so ended up removing most of that code, Kept the bit about
isolated workspaces because it just makes sense.
* Had to re-write one of the `DocumentMappingService` methods to use
`TextChange` instead of `TextEdit` so I could use Roslyn LSP types
* The hacky, and fortunately temporary, way we were doing generated C#
content was causing cache misses in Roslyn in tests, so fixed that up
* When I finally got up to manual testing I found a bug in the platform
that meant inlay hints just don't work with dynamic registration, so
filed the above linked PR to fix that

If reviewing commit-at-a-time please note that the first commit was
written before the reworking of extension methods and LSP types, and the
fixes for that are in the last commit.
davidwengier added a commit that referenced this issue Aug 13, 2024
Part of #9519
Fixes #10688

Tried not to go on too many side quests with this one, but I guess
review commit-at-a-time if you want to double check :)
davidwengier added a commit that referenced this issue Aug 30, 2024
I nerd sniped myself thinking about how to get formatting into
cohosting, given the limitations Alex ran into doing the relayering for
auto insert, and this is the result. I was going to go further and port
the actual formatting endpoint to cohosting, but that would have ran
into the same issue that Alex did with auto insert, so I figured I'd
wait for that to merge, and put this up in the meantime.

This unblocks the formatting, code action and completion end points from
being ported.
Part of #10743
Part of #9519

I **strongly** recommend reviewing commit-at-a-time, as I did this
deliberately in an order, and in order to (hopefully) make reviewing
easier. Though granted, there are a lot of commits.
davidwengier added a commit that referenced this issue Sep 5, 2024
Fixes #10743
Part of #9519

Brings formatting to cohosting. Relatively simple because of previous
PRs. Have left sharing full test coverage of the formatting engine for
later
davidwengier added a commit that referenced this issue Sep 6, 2024
Needs dotnet/roslyn#74978
Fixes #10695
Part of #9519

Pretty straightforward. A tiny bit of code moved to be shared with Go To
Def, but that's it.
davidwengier added a commit that referenced this issue Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants