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

[WIP] Language Service hookup rewrite #3423

Closed
wants to merge 4 commits into from
Closed

[WIP] Language Service hookup rewrite #3423

wants to merge 4 commits into from

Conversation

davkean
Copy link
Member

@davkean davkean commented Mar 26, 2018 •
edited
Loading

This is a rewrite of the language service hookup that moves from an all knowing single object LanguageServiceHost that is aware of all configurations and related IWorkspaceProjectContext, to instead individual configured-project-based services WorkspaceContextHostInstance that get fired up and torn down when a configuration becomes "implicitly" active. This naturally supports active config switches from debug -> release, and multi-targeting projects.

This also fixes: #2733.

Currently this lives alongside the existing language service hookup, but is intended to replace it.

Lots of TODOs:

  • Feature flag to turn it on but turn off the other one
  • Multi-targeting display names
  • All IVsHierarchy interaction with Roslyn is through the "real" hierarchy and we handle any of the context properties
  • "Active Context"
  • All external iteration; Container Language, Error List, Edit & Continue, Code Model
  • Need to push to UI thread
  • Need to avoid overlapping evalution/design-time builds
  • Prevent unloading project while updating context - but also bail early

@davkean davkean mentioned this pull request Mar 26, 2018
Closed
@codecov-io
Copy link

codecov-io commented Mar 26, 2018 •
edited
Loading

Codecov Report

Merging #3423 into master will decrease coverage by 0.22%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #3423      +/-   ##
=========================================
- Coverage   67.93%   67.7%   -0.23%     
=========================================
  Files         728     733       +5     
  Lines       36608   36668      +60     
  Branches     2128    2128              
=========================================
- Hits        24869   24827      -42     
- Misses      11331   11433     +102     
  Partials      408     408
Flag Coverage Δ
#debug 67.7% <0%> (-0.23%) ⬇️
Impacted Files Coverage Δ
...m/LanguageServices/Handlers/AnalyzerItemHandler.cs 0% <0%> (ø) ⬆️
...eServices/Handlers/ProjectPropertiesItemHandler.cs 0% <0%> (ø) ⬆️
...ageServices/Handlers/AdditionalFilesItemHandler.cs 0% <0%> (ø) ⬆️
...rkspaceContextHost.WorkspaceContextHostInstance.cs 0% <0%> (ø)
...tem/LanguageServices/Handlers/SourceItemHandler.cs 75.8% <0%> (-9.91%) ⬇️
...ProjectSystem/AbstractImplicitlyActiveComponent.cs 0% <0%> (ø)
...Services/Rewrite/ApplyChangesToWorkspaceContext.cs 0% <0%> (ø)
...eServices/Handlers/MetadataReferenceItemHandler.cs 84.93% <0%> (-7.72%) ⬇️
...m/LanguageServices/Rewrite/WorkspaceContextHost.cs 0% <0%> (ø)
...d/ProjectSystem/UnconfiguredProjectTasksService.cs 0% <0%> (-48.84%) ⬇️
... and 208 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72966b6...24ba844. Read the comment docs.

Pilchie
Pilchie reviewed Apr 3, 2018
View reviewed changes
...rosoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/AbstractImplicitlyActiveComponent.cs
_activationTracking.ImplicitlyDeactivated += OnImplicitlyDeactivated;

if (_activationTracking.IsImplicitlyActive)
return LoadAsync();
Copy link
Member

Choose a reason for hiding this comment

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

Is LoadAsync meant to be idempotent? Isn't there a race here if the event fires between subscription and here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a race here; if we deactivate after we check this property - we'll Load but never unload until project tear down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, I don't know of a good way to do this without locks. I think I need to rethink using events for this, ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've figured out a better way to do this - I'm going to drop the events.

Copy link
Member Author

Choose a reason for hiding this comment

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

#3689

...lStudio.ProjectSystem.Managed/ProjectSystem/LanguageServices/Handlers/AnalyzerItemHandler.cs Outdated
: this(project, null)
{
}

[ImportingConstructor]
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should.

...rojectSystem.Managed/ProjectSystem/LanguageServices/Handlers/ProjectPropertiesItemHandler.cs

public void Initialize(IWorkspaceProjectContext context)
{
_context = context;
Copy link
Member

Choose a reason for hiding this comment

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

It's expected that we can overwrite the once given in the constructor now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, problem is supporting both language services hosts; one grabs them via MEF, the other manually creates them. I'll add an assert.

@davkean davkean mentioned this pull request Apr 16, 2018
Closed
@davkean davkean force-pushed the LanguageServiceRewrite branch from 51aebc0 to 28d894b Compare June 5, 2018 04:49
@davkean davkean requested a review from a team as a code owner June 5, 2018 04:49
@davkean
Copy link
Member Author

davkean commented Jun 5, 2018

I had to rebase this, and will need to rebase in the future, so I'd hold off reviewing at the moment.

@davkean
Copy link
Member Author

davkean commented Jun 19, 2018

Please still hold off reviewing this.

@davkean davkean force-pushed the LanguageServiceRewrite branch from 51eaa93 to 24ba844 Compare June 22, 2018 06:34
@davkean
Copy link
Member Author

davkean commented Sep 10, 2018

This has been closed in lieu of the features/LanguageService branch.

@davkean davkean closed this Sep 10, 2018
@ghost
Copy link

ghost commented Sep 25, 2018

Hey @davkean - what does the closure of this ticket mean for #2733?

@Pilchie
Copy link
Member

Pilchie commented Sep 25, 2018

It's still being worked on, just one commit at a time in the features/languageservice branch instead of a single PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pilchie Pilchie Pilchie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Editor doesn't handle conditional compilation symbol with netstandard or .net core
3 participants
@davkean @codecov-io @Pilchie