Skip to content

Move host analyzer references to Solution snapshot#42240

Merged
tmat merged 15 commits intodotnet:masterfrom
tmat:AnalyzersInSnapshot1
Apr 14, 2020
Merged

Move host analyzer references to Solution snapshot#42240
tmat merged 15 commits intodotnet:masterfrom
tmat:AnalyzersInSnapshot1

Conversation

@tmat
Copy link
Member

@tmat tmat commented Mar 7, 2020 •
edited
Loading

Moves host analyzer references to SolutionState.

Fixes #41127

@tmat tmat force-pushed the AnalyzersInSnapshot1 branch 7 times, most recently from 0b74b44 to e63cf13 Compare March 12, 2020 17:07
@tmat tmat force-pushed the AnalyzersInSnapshot1 branch from be4d623 to da428f8 Compare March 18, 2020 18:56
@tmat tmat force-pushed the AnalyzersInSnapshot1 branch from da428f8 to 67a75df Compare March 27, 2020 01:53
@tmat tmat force-pushed the AnalyzersInSnapshot1 branch from 89dbfc7 to eb62411 Compare March 27, 2020 21:56
@tmat tmat marked this pull request as ready for review March 27, 2020 21:58
@tmat tmat requested review from a team as code owners March 27, 2020 21:58
@tmat tmat changed the title Analyzers in snapshot1 Move host analyzer references to Solution snapshot Mar 27, 2020
@tmat
Copy link
Member Author

tmat commented Mar 27, 2020

@mavasani @CyrusNajmabadi @jasonmalinowski @dotnet/roslyn-ide PTAL

@tmat tmat force-pushed the AnalyzersInSnapshot1 branch from eb62411 to 831755b Compare March 28, 2020 00:53
@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Mar 28, 2020

Any recommendation on how to review this?

@tmat
Copy link
Member Author

tmat commented Mar 28, 2020

@CyrusNajmabadi Commit by commit should work.

@tmat tmat force-pushed the AnalyzersInSnapshot1 branch 4 times, most recently from 76b2efa to 553c0a5 Compare March 31, 2020 20:42
@jasonmalinowski
Copy link
Member

jasonmalinowski commented Apr 1, 2020

Is DocumentDiagnosticAnalyzer.cs moving between layers a problem? We don't have any IVT use of that?

@tmat
Copy link
Member Author

tmat commented Apr 1, 2020 •
edited
Loading

We don't have any IVT use of that?

Good point. I'll check. If we do, I'm gonna add type forwarder.

@mavasani
Copy link
Contributor

mavasani commented Apr 1, 2020

Pretty sure TypeScript uses DocumentDiagnosticAnalyzer.

@jasonmalinowski
Copy link
Member

jasonmalinowski commented Apr 1, 2020

If we do, I'm gonna add type forwarder.

Type forwarders are only useful if you're pushing stuff down, not up. 😦

@tmat
Copy link
Member Author

tmat commented Apr 1, 2020

I'm pushing them down from Features to Workspaces, no?

jasonmalinowski
jasonmalinowski reviewed Apr 1, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Leaving comments so far.

src/VisualStudio/Core/Def/RoslynPackage.cs Outdated
Copy link
Member

@jasonmalinowski jasonmalinowski Apr 1, 2020

Choose a reason for hiding this comment

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

Was this previously async? If it wasn't this may risk some flakiness in integration tests if we have any. Surround it with an async listener operation so tests can appropriately wait?

Copy link
Member Author

@tmat tmat Apr 3, 2020

Choose a reason for hiding this comment

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

Added. I assume integration tests need to be updated to wait for it. Where should such wait be done?

@jasonmalinowski
Copy link
Member

jasonmalinowski commented Apr 1, 2020

And oh: I carefully reviewed the workspace changes, and less carefully reviewed anything to the diagnostics system. It looked like most of it was just simply types changing as things flowed around, but if there was something "interesting" there I either missed it or didn't appreciate it's interestingness.

mavasani
mavasani approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM - I primarily looked at the diagnostics related changes.

@tmat tmat force-pushed the AnalyzersInSnapshot1 branch 2 times, most recently from c169ea3 to 8762293 Compare April 3, 2020 01:56
jasonmalinowski
jasonmalinowski requested changes Apr 3, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Initialization issue fixed, still a potential MEF deadlock risk. Easy to fix though: move the stuff running in MEF constructors just to run "later" and not until your fire-and-forget task is launched.

jasonmalinowski
jasonmalinowski approved these changes Apr 3, 2020
View reviewed changes
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmat tmat force-pushed the AnalyzersInSnapshot1 branch from 37a6c72 to 0a4b112 Compare April 13, 2020 17:04
@tmat tmat merged commit a6289eb into dotnet:master Apr 14, 2020
@ghost ghost added this to the Next milestone Apr 14, 2020
@tmat tmat deleted the AnalyzersInSnapshot1 branch April 14, 2020 06:18
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski approved these changes

+1 more reviewer

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.7.P1

Development

Successfully merging this pull request may close these issues.

Add host provided analyzers to solution snapshot

6 participants

@tmat @CyrusNajmabadi @jasonmalinowski @mavasani @sharwell @jinujoseph