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.

Sign up for GitHub

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

Jump to bottom

Turn on language-service integration rewrite #4419

Merged
merged 71 commits into from
Jan 4, 2019
Merged

Turn on language-service integration rewrite #4419

merged 71 commits into from
Jan 4, 2019

Conversation

davkean
Copy link
Member

@davkean davkean commented Dec 29, 2018 •
edited
Loading

This merges the language-service integration rewrite into master and makes it replace the existing integration. All of these changes have already been reviewed or are out for review, with exception to e4b0c17 and a7e646f.

This fixes: #2733.

Currently broken in Preview 2 bits (unrelated to this change):

Roslyn

Roslyn leaking AbstractProjects
Project Context switch icon changes when moving caret
Multi-targeting context is broken in 16.0 Preview 1
With "Full Solution Analysis" turned on, certain syntax errors get duplicated
Analyzers warning/errors/messages are not correctly grouped for multi-targeting projects

Project System

Build warnings & errors from build always get applied to the first TFM in the project

Testing done

Performance

  • Solution-load time loading Roslyn.sln
  • Solution-load time loading ProjectSystem.sln
  • Solution-load time File -> New -> Console App

Error List

Project manipulation

  • Add/remove a reference and Roslyn sees change (single-TFM)
  • Add/remove a reference and Roslyn sees change (multi-TFM)
  • Add/remove a source file and Roslyn sees change (single-TFM)
  • Add/remove a source file and Roslyn sees change (multi-TFM)
  • Add/remove a Razor file and Roslyn sees change (single-TFM)
  • Add/remove an analyzer and Roslyn sees change (single-TFM)
  • Add/remove an analyzer and Roslyn sees change (multi-TFM)
  • Add/remove an additional file and Roslyn sees change (single-TFM)
  • Add/remove an additional file and Roslyn sees change (multi-TFM)
  • Change conditional compilation symbols and Roslyn sees change (single-TFM)
  • Change conditional compilation symbols and Roslyn sees change (multi-TFM)
  • Retarget project from one TFM -> another
  • Change configuration (single-TFM) [note, this and next scenario are affected by https://github.com/Roslyn leaking AbstractProjects roslyn#32036]
  • Change configuration (multi-TFM)

Features

@davkean
Rename ILanguageServiceHost -> IActiveWorkspaceProjectContextHost
e91a1fb
@davkean
Add IWorkspaceProjectContextHost
35c6402 
This lets consumers (error list, edit & continue, etc) run code against context while protecting against being disposed.
@davkean
Merge pull request dotnet#4046 from davkean/ActiveContext
75ab36f 
Rename ILanguageServiceHost -> IActiveWorkspaceProjectContextHost
@davkean
Merge pull request dotnet#4047 from davkean/MoveContext
1af85aa 
Add IWorkspaceProjectContextHost
@davkean
Stop using IWorkspaceProjectContext version of the project file name
bc7c4c6 
Unneeded dependency here, just parent the files with the current project file.
@davkean
Stop using IWorkspaceProjectContext version of the project file name
fcbf662 
Unneeded dependency here, just parent the files with the current project file.
@davkean
Add IWorkspaceProjectContextAccessor
95e3dc9 
Provides access to IWorkspaceProjectContext and associated services.
@davkean
Remove ForgroundWorkspaceProjectContext in lieu of check in accessor
5d5cd2b
@davkean
Move active context registration to WorkspaceContextHostInstance
b9efb0d
@davkean
Add Task<T> overload for ExecuteUnderLockAsync
e0322bc
@davkean
Add Task<T> overload for OpenContextForWriteAsync
477fc48
@davkean
Merge pull request dotnet#4098 from davkean/1_ProjectAccessor
11ba391 
Add IWorkspaceProjectContextAccessor to let services use error list/edit & continue services under a lock.
@davkean
Merge IActiveWorkspaceProjectContextHost and IWorkspaceProjectContext…
ce49d0f 
…Host

This moves all consumers of the old language service APIs over to the new APIs and shims the old language service under them.
@davkean
Merge pull request dotnet#4049 from davkean/RemoveFileName
9f52e74 
Stop using IWorkspaceProjectContext version of the project file name
@davkean
Merge pull request dotnet#4103 from davkean/MergeActiveContext
f4d0872 
 Merge IActiveWorkspaceProjectContextHost and IWorkspaceProjectContextHost
@davkean
Remove thread-safeness from ApplyChangesToWorkspaceContext
ac635be 
This class needs to be made async, which prevents lock from being used in async methods. It does not need to be thread-safe as the only consumer of it, WorkspaceContextHostInstance, prevents overlap.
@davkean
Make IApplyChangesToWorkspaceContext async
79d7b4a 
In preparation of making IWorkspaceProjectContext async, plumb through async in ApplyChangesToWorkspaceContext. Handlers will come next.
@davkean
Merge pull request dotnet#4187 from davkean/ApplyWorkspaceContext
bac82af 
Remove thread-safeness from ApplyChangesToWorkspaceContext
@davkean
Merge pull request dotnet#4188 from davkean/AddAsync
9f343e2 
 Make IApplyChangesToWorkspaceContext async
@davkean
merge features/languageservice -> master
d4ac2ee
@davkean
Fix build breaks after merge
b1607c0
@davkean
Populate/Dispose context on a background thread
5f50054 
Fixes: https://github.com/dotnet/project-system/issues/353.

Note, creation is still UI thread based.
@davkean
Remove usage of AbstractProject
3c827dd 
We can use the new IWorkspaceProjectContext.Id property.
@davkean
Merge pull request dotnet#4229 from davkean/BackgroundThread
6db5465 
Populate/Dispose context on a background thread
@davkean
Wrap context usage in a batch
2c36e1c 
Roslyn will avoid updating VisualStudioWorkspace until after a batch has finished.
@davkean
Merge pull request dotnet#4230 from davkean/RemoveAbstractProjectUsage
2bd245c 
Remove usage of AbstractProject
@davkean
Merge pull request dotnet#4228 from davkean/Batch
f74381b 
Wrap context usage in a batch
@davkean
Wrap context usage in a batch
834b4e8 
Roslyn will avoid updating VisualStudioWorkspace until after a batch has finished.
@davkean
Release context on UI-thread
f3126f3 
IWorkspaceProjectContext.Dispose is not actually free-threaded:

Microsoft.CodeAnalysis.Workspaces.dll!Roslyn.Utilities.Contract.ThrowIfFalse(bool condition, string message)    Unknown
    Microsoft.CodeAnalysis.EditorFeatures.dll!Microsoft.CodeAnalysis.Editor.Shared.Utilities.ForegroundThreadAffinitizedObject.AssertIsForeground()    Unknown
    Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl.SetDocumentContext(Microsoft.CodeAnalysis.DocumentId documentId)    Unknown
    Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.ClearOpenDocument(Microsoft.CodeAnalysis.DocumentId documentId, bool isSolutionClosing)    Unknown
    Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.ClearOpenDocuments(Microsoft.CodeAnalysis.ProjectId projectId)    Unknown
    Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.ClearProjectData(Microsoft.CodeAnalysis.ProjectId projectId)    Unknown
    Microsoft.CodeAnalysis.Workspaces.dll!Microsoft.CodeAnalysis.Workspace.OnProjectRemoved(Microsoft.CodeAnalysis.ProjectId projectId)    Unknown
    Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl.OnProjectRemoved(Microsoft.CodeAnalysis.ProjectId projectId)    Unknown
    Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioProject.RemoveFromWorkspace.AnonymousMethod__86_0(Microsoft.CodeAnalysis.Workspace w)    Unknown
    Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioWorkspaceImpl.ApplyChangeToWorkspace(System.Action<Microsoft.CodeAnalysis.Workspace> action)    Unknown
    Microsoft.VisualStudio.LanguageServices.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.VisualStudioProject.RemoveFromWorkspace()    Unknown
    Microsoft.VisualStudio.LanguageServices.Implementation.dll!Microsoft.VisualStudio.LanguageServices.Implementation.ProjectSystem.CPS.CPSProject.Dispose()    Unknown
@davkean
Avoid UI-thread check through accessor
a81eb44 
Components that were calling through accessor where hitting the UI-thread check.
davkean added 12 commits December 13, 2018 17:31
@davkean
Ensure language service is initialized before project load
444bffe 
This ensures that the project is loaded before project load has completed. This brings back the original experience of the previous language-service integration.
@davkean
Doc exception
34824a4
@davkean
Merge pull request dotnet#4380 from davkean/ActiveHost2
82c6ef0 
 Add an implemention of IActiveWorkspaceProjectContext for new language service
@davkean
Allow design-time build logging to be turned on Dev16/suffixes
b960a81
@davkean
Make sure to apply changes under batch
b707f92 
We weren't waiting for changes to be applied before we closed the batch. This path is currently synchronous so it's just by change that this currently works.
@davkean
Remove unneeded async state machine
6da6555
@davkean
Merge pull request dotnet#4412 from davkean/DesignTimeBuildLoggingFor…
217149b 
…Dev16

Allow design-time build logging to be turned on Dev16/suffixes
@davkean
Merge pull request dotnet#4413 from davkean/UnderBatch
90be1b8 
 Make sure to apply changes under batch
@davkean
Block publishing language-service until active config has been determ…
7351797 
…ined
@davkean
Merge features/languageservice -> master
83b5c96
@davkean
Fix build errors/warning after merge
e4b0c17
@davkean
merge upstream/master
811cb75
@davkean davkean requested a review from a team as a code owner December 29, 2018 03:31
@davkean davkean added this to the 16.0 Preview 2 milestone Dec 29, 2018
@davkean
Doing multiple builds in a row would cause Visual Studio to OOM
a7e646f 
ClearAllErrors was the only method calling this method and would never break out.
@davkean davkean mentioned this pull request Dec 29, 2018
Closed
@davkean davkean added the Blocked This issue is blocked from making progress due to another issue. label Dec 29, 2018
@davkean
Copy link
Member Author

davkean commented Dec 29, 2018

This is blocked by dotnet/roslyn#31365.

davidwengier
davidwengier approved these changes Dec 29, 2018
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Nice work 👍

This was referenced Jan 4, 2019
Merged
Open
@davkean davkean mentioned this pull request Jan 4, 2019
Merged
@davkean
Always return a value for ActiveIntellisenseProjectContext
196a145 
When we've never been set an active editor context, just return the one based on the active configuration.

This combined with dotnet/roslyn#32153, unblocks Razor.
@davkean davkean force-pushed the LanguageServiceMerge branch from e0cf5a5 to 196a145 Compare January 4, 2019 13:41
jasonmalinowski pushed a commit to dotnet/roslyn that referenced this pull request Jan 4, 2019
@davkean
Handle GetIntellisenseProjectName returning empty
057904d 
CPS-based projects have moved from a "fake" IVsHierarchy being passed to the Roslyn to returning the "real" hiearchy representing the project. This results in GetItemContext actually returning a real implementation of "IWebApplicationCtxSvc" instead of previously failing.

This implementation returns S_OK and an "empty" project name - to indicate that it doesn't actually handle the call (with a giant TODO). Handle that situation and treated it as failed, so that we proceed onto IVsContainedLanguageProjectNameProvider which provides the real underlying value.

This is blocking dotnet/project-system#4419.
@jmarolf
Copy link
Contributor

jmarolf commented Jan 4, 2019

@davkean I assume you are asleep but I plan to merge this in the next 15 minutes unless I hear from you

@jmarolf jmarolf merged commit b77ecef into dotnet:master Jan 4, 2019
@davkean davkean mentioned this pull request Jan 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwengier davidwengier davidwengier approved these changes

Assignees
No one assigned
Labels
Blocked This issue is blocked from making progress due to another issue.
Projects
None yet
Milestone
16.0 Preview 2
Development

Successfully merging this pull request may close these issues.

Editor doesn't handle conditional compilation symbol with netstandard or .net core
4 participants
@davkean @jmarolf @drewnoakes @davidwengier