moved waiter from diagnostics.dll to features.dll where all interfaces are defined. #24512

heejaechang · 2018-01-29T08:55:16Z

this put this (#24120) back into dev15.7.x and added shim to make old interface work until partner teams move out of them.

Customer scenario

This doesn't affect customer experience. this is purely for testing.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/547597

Workarounds, if any

install our Roslyn.Test.Setup.vsix to machines that want to test our asynchronous features.

Risk

This only affects Testing, so there shouldn't be user facing risk.

Performance impact

There should be no perf impact due to this change to users. only affects testing.

Is this a regression from a previous update?

N/A

Root cause analysis

As all other Roslyn design, we separated out test related component from product component, and used vsix and MEF to inject those when it is needed (test env). it gave us clean separation. but gave us yet one more thing to setup when running test.

it was fine for our own test env, but when it meets with other components in VS and when other team needed to run some of their test with our test hook, it became yet one more point that can fail for test setup.

so, we now put those hooks in Roslyn dlls itself which can be enabled by env variable.

How was the bug found?

during testing.

…rface

heejaechang · 2018-01-29T19:28:43Z

this is port of #24120 that got merged into dev15.6.x and then reverted due to issue with partner teams which still using old interface.

the first commit of this PR is straight revert of (2aa793e) which reverted the change in dev15.6.x. next commits add shim for old APIs.

heejaechang · 2018-01-29T19:35:53Z

@amcasey @minestarks can you take a look and move to new API once this gets checked into dev15.7.x? @brettfo does F# also use this API?

heejaechang · 2018-01-29T19:37:02Z

@jinujoseph @Pilchie this is #24120 + shim for old APIs. can I get approval for 15.7.x ? thank you

brettfo · 2018-01-29T20:22:38Z

I don't think F# uses any of these APIs

heejaechang · 2018-01-29T20:46:57Z

ping, @Pilchie ?

minestarks · 2018-01-29T21:16:11Z

@brettfo you don't use the Apex waiters here? You were the original author, I think so I wanted to double check

minestarks · 2018-01-29T21:17:02Z

@heejaechang What is the new API? What is going away? I couldn't tell from the diff, sorry.

heejaechang · 2018-01-29T21:22:21Z

src/EditorFeatures/Core.Wpf/NavigateTo/NavigateToItemProviderFactory.cs

        [ImportingConstructor]
-        public NavigateToItemProviderFactory(
-            [ImportMany] IEnumerable<Lazy<IAsynchronousOperationListener, FeatureMetadata>> asyncListeners)
+        public NavigateToItemProviderFactory(IAsynchronousOperationListenerProvider listenerProvider)


@minestarks this basically shows how you need to change code.

importing IEnumerable<IAsync, FeatureMetadata> listeners and then creating new AggregateAsync..Operation pattern goes away and changed to

import IAsync..OperationListenerProvider and calling listenerProvider.GetListener(...)

@minestarks type/members showing up here - 5d7b8ba - will go away soon. for now I didn't put [Obsolete] but I will do that in next insertion so that we can remove those soon.

brettfo · 2018-01-29T21:24:18Z

@minestarks F# doesn't ever consume that file, I made those change from purely an IDE perspective to expose the waiters for other teams.

minestarks · 2018-01-29T21:27:31Z

@heejaechang thanks! Please let me know when the package will be published and what VS branch you're inserting to. This will help us set a timeline for consuming the change and deprecating old types.

Pilchie · 2018-01-31T16:33:19Z

Approved - sorry for the delay

heejaechang · 2018-01-31T20:18:07Z

@minestarks it will insert into d15.7stg

jasonmalinowski · 2018-01-31T22:30:55Z

@heejaechang Should we get [Obsolete] attributes on these sooner to help @minestarks understand what does and doesn't need to be removed? Since their build will consume new packages on their schedule, it's OK for us to add them now and that's not a breaking change to the product. We can still insert separately.

Failing that, do we have a bug at least tracking what exactly needs to be deleted again? I think it's just reverting the last two commits in this PR?

heejaechang · 2018-01-31T22:56:18Z

@jasonmalinowski I didn't add Obsolete worrying the Apex code might fail to build which still uses FeatureMetadata. once I check this (https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/101822?_a=overview) in, I will put [Obsolete]

jasonmalinowski · 2018-02-01T00:25:25Z

@heejaechang: got it. Are we tracking that somewhere?

heejaechang · 2018-02-01T00:28:48Z

#24574

* Remove duplicate lock DocumentState.s_syntaxTreeToIdMapLock This lock is only being used to protect access to an instance which contains internal synchronization. * Better handle surrounding directives when inlining a local variable. * Add tests. * Share code between VB and C#. * Reduce allocations in UnboundLambda Fixes #23463 * Restore ReturnInferenceCacheKey as the key for _returnInferenceCache * Update code to more closely follow patterns of the original code * Cleanup from code review * basic fix for intellisense in Immediate window * better comments and cleanup * Add basic integration tests * cleanup inproc Immediate window integration test helper * fix incorrect comment * address PR feedback * create Immediate window on ImmediateWindow_InProc.GetText() * Verify MSBuild version in Developer CMD prompt Roslyn is designed to have the simplest possible contribution story: clone then build. Every pre-req needed is either located on the machine or bootstrapped via NuGet. All the way down to using an xcopy MSBuild if needed. The one case which causes a problem is the VS command prompt. In this case MSBuild is pre-installed on the machine and may or may not be suitable for building Roslyn. Previously when building from a VS command prompt we just used whatever MSBuild was provided. The assumption being a developer command prompt was an explicit statement of whath MSBuild you wanted to use. Based on all of our customer reports though this does not seem to be the assumption that consumers of our repo have. The build gave them no explicit errors about the provided toolset and hence when the build failed they assigned flakiness to our repo. Going forward we are applying the same version validation to MSBuild when provided via a developer command prompt. If it doesn't match we will refuse to build asking the user to upgrade VS or build from a normal command prompt. * Remove unneeded debugging line * Comment about pre-release * Added minimum version * Add Omit If Default style option * Add space to be like test without the omit * Add/Remove without needing a property * Reformat * PR feedback * Fix VB diagnostic based on feedback * Handle case of NotApplicable modifier and field declaration list * Fix tests * PR feedback * PR feedback * PreviewCodeAction was overriding ComputeOperations but returning a post-processed operation from original action. This results in another PostProcess being called on the codeaction. If postprocess was overriden in originalaction that'll be ignored the second time (#23920) * Support negative null-check when we are suggesting to inline type checks Fixes #21097 Fixes #24286 * fix a case where persistent storage registration fails and some clean… (#24458) * fix a case where persistent storage registration fails and some clean up code around it. * added readonly * address PR feedback * removed comments no longer relevant * renamed lock name * moved waiter from diagnostics.dll to features.dll where all interfaces are defined. (#24512) * put listener change back in (#24120) * leave old types in legacy folder until partner teams move to new interface * added legacy waiter to support partner teams * Remove methods indirecting access to _metadataFileNameToConvertedProjectReference This field is documented as being written and read from any thread, but in practice all uses are guarded by an AssertIsForeground(). Thus we can get rid of the helper methods that are trying to "help" by locking before accessing the fields, making it really hard to track all the real uses of it. * Make method static that doesn't need state * add a comment to address PR feedback * Fix up tests of P2P to metadata reference conversion It turns out we had some tests, but the tests were disabled. This was because the tests weren't working properly anyways: they were calling into UpdateProjectBinPath which only updated some (but not all) of the project state. That was an internal helper method that shouldn't be used by tests. Updating the tests to use SetBinOutputPathAndRelatedData works better. * Delete debug-only reference validation This was some legacy code that tried to verify that the references we have from the project system match up to what DTE and other sources say. This was debug-only, and the actual asserts were commented out. This is deadweight at this point, so delete it. * added and cleaned up logs around build and live diagnostics. (#24551) also added RoslynActivityLogger that can be enabled through project-system-tool * Avoid closure allocations on the BindSyntaxTreeToId fast path * CS1628 error text mentions in parameters; fixes #24584 * Small cleanup of completion logic. * Move to xunit.console for CoreClr tests Previously we were using xunit.console for desktop tests and dotnet-xunit for our CoreClr tests. This change unifies us on top of xunit.console (now that it has a netcoreapp2.0 version available). * Move unix builds to xunit.runner.console as well * Get actual directory name, not file * Fix dir name issue * fixed build break

* Remove duplicate lock DocumentState.s_syntaxTreeToIdMapLock This lock is only being used to protect access to an instance which contains internal synchronization. * Better handle surrounding directives when inlining a local variable. * Add tests. * Share code between VB and C#. * Reduce allocations in UnboundLambda Fixes #23463 * Restore ReturnInferenceCacheKey as the key for _returnInferenceCache * Update code to more closely follow patterns of the original code * Cleanup from code review * Verify MSBuild version in Developer CMD prompt Roslyn is designed to have the simplest possible contribution story: clone then build. Every pre-req needed is either located on the machine or bootstrapped via NuGet. All the way down to using an xcopy MSBuild if needed. The one case which causes a problem is the VS command prompt. In this case MSBuild is pre-installed on the machine and may or may not be suitable for building Roslyn. Previously when building from a VS command prompt we just used whatever MSBuild was provided. The assumption being a developer command prompt was an explicit statement of whath MSBuild you wanted to use. Based on all of our customer reports though this does not seem to be the assumption that consumers of our repo have. The build gave them no explicit errors about the provided toolset and hence when the build failed they assigned flakiness to our repo. Going forward we are applying the same version validation to MSBuild when provided via a developer command prompt. If it doesn't match we will refuse to build asking the user to upgrade VS or build from a normal command prompt. * Remove unneeded debugging line * Comment about pre-release * Added minimum version * Add Omit If Default style option * Add space to be like test without the omit * Add/Remove without needing a property * Reformat * PR feedback * Fix VB diagnostic based on feedback * Handle case of NotApplicable modifier and field declaration list * Fix tests * PR feedback * PR feedback * Support negative null-check when we are suggesting to inline type checks Fixes #21097 Fixes #24286 * fix a case where persistent storage registration fails and some clean… (#24458) * fix a case where persistent storage registration fails and some clean up code around it. * added readonly * address PR feedback * removed comments no longer relevant * renamed lock name * moved waiter from diagnostics.dll to features.dll where all interfaces are defined. (#24512) * put listener change back in (#24120) * leave old types in legacy folder until partner teams move to new interface * added legacy waiter to support partner teams * Remove methods indirecting access to _metadataFileNameToConvertedProjectReference This field is documented as being written and read from any thread, but in practice all uses are guarded by an AssertIsForeground(). Thus we can get rid of the helper methods that are trying to "help" by locking before accessing the fields, making it really hard to track all the real uses of it. * Make method static that doesn't need state * Fix up tests of P2P to metadata reference conversion It turns out we had some tests, but the tests were disabled. This was because the tests weren't working properly anyways: they were calling into UpdateProjectBinPath which only updated some (but not all) of the project state. That was an internal helper method that shouldn't be used by tests. Updating the tests to use SetBinOutputPathAndRelatedData works better. * Delete debug-only reference validation This was some legacy code that tried to verify that the references we have from the project system match up to what DTE and other sources say. This was debug-only, and the actual asserts were commented out. This is deadweight at this point, so delete it. * added and cleaned up logs around build and live diagnostics. (#24551) also added RoslynActivityLogger that can be enabled through project-system-tool * Avoid closure allocations on the BindSyntaxTreeToId fast path * CS1628 error text mentions in parameters; fixes #24584 * Update optimization data to 2.7.0-beta3-62526-01... * Small cleanup of completion logic. * Locate implementations for reference assemblies using the process binding path * Use GlobalAssemblyCache helper to locate assemblies directly in the GAC * Update InteractiveEditorFeatures to account for a second definition of GlobalAssemblyCache * Move to xunit.console for CoreClr tests Previously we were using xunit.console for desktop tests and dotnet-xunit for our CoreClr tests. This change unifies us on top of xunit.console (now that it has a netcoreapp2.0 version available). * Move unix builds to xunit.runner.console as well * Fixes 559223 Fix and re-enable test that would catch this error * Update LanguageServices training data again... * Get actual directory name, not file * Fix dir name issue * Cleanup based on code review feedback * Check fully-qualified names for SuppressIldasmAttribute and ReferenceAssemblyAttribute * Use correct reference location, or fail decompilation if it's not available * Fix typo... * Don't use inferred member name if that creates duplicates (#24632) * Fixes #23983 * Added test for unique IDEDiagnosticIDs * Fixed capitalization on local variable * Fix `is` and pattern-matching behavior in presence of implicit UD conversion (#24547) * Fix `is` and pattern-matching behavior in presence of implicit UD conversion and also an explicit reference conversion. User-defined conversions should never be considered for `is` and pattern-matching. Fixes #24522

heejaechang · 2018-02-09T03:40:15Z

@amcasey @minestarks VS side changes are checked into 15.7stg
https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/104684?_a=files

there are 2 things you guys need to do.

is moving to new API as I said here (moved waiter from diagnostics.dll to features.dll where all interfaces are defined. #24512 (review))
is moving away from Microsoft.VisualStudio.IntegrationTest.Setup.vsix and use environment variable. you guys don't need to install this (Microsoft.VisualStudio.IntegrationTest.Setup.vsix) anymore. and you need to set environment variable (RoslynWaiterEnabled=1) to enable listener/waiter.

if you are using VisualStudioHostTest (Apex) for integration testing, then you can do this to set the variable.

protected override VisualStudioHostConfiguration GetVisualStudioHostConfiguration()
{
var config = base.GetVisualStudioHostConfiguration();
config.Environment.Add("RoslynWaiterEnabled", "1");
return config;
}

let me know if you need more info.

heejaechang · 2018-02-09T04:05:41Z

[Obsolete] is added to Shim. after that is in, I will have another PR which will actually remove those legacy API.

heejaechang added 3 commits January 28, 2018 23:48

put listener change back in (dotnet#24120)

f92627f

leave old types in legacy folder until partner teams move to new inte…

5d7b8ba

…rface

added legacy waiter to support partner teams

a4b6cb5

heejaechang requested a review from a team as a code owner January 29, 2018 08:55

brettfo approved these changes Jan 29, 2018

View reviewed changes

jinujoseph approved these changes Jan 29, 2018

View reviewed changes

heejaechang commented Jan 29, 2018

View reviewed changes

Pilchie added the Approved to merge label Jan 31, 2018

heejaechang merged commit 6da6115 into dotnet:dev15.7.x Jan 31, 2018

heejaechang mentioned this pull request Feb 1, 2018

add [Obsolete] and checkin VS side change for this change - https://github.com/dotnet/roslyn/pull/24512 #24574

Closed

heejaechang mentioned this pull request Feb 9, 2018

added [Obsolete] to legacy waiter/listener API. #24724

Merged

heejaechang mentioned this pull request Feb 13, 2018

Enable TestPreviewDiagnosticTaggerInPreviewPane #24783

Merged

davkean mentioned this pull request Mar 9, 2018

VisualBasicSnippetInfoService is failing MEF cache for 15.7 #25212

Closed

heejaechang mentioned this pull request Mar 9, 2018

fixed MEF composition issue cased by waiter change #25361

Merged

moved waiter from diagnostics.dll to features.dll where all interfaces are defined. #24512

moved waiter from diagnostics.dll to features.dll where all interfaces are defined. #24512

Uh oh!

Conversation

heejaechang commented Jan 29, 2018 • edited by sharwell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

heejaechang commented Jan 29, 2018

Uh oh!

heejaechang commented Jan 29, 2018

Uh oh!

heejaechang commented Jan 29, 2018

Uh oh!

brettfo commented Jan 29, 2018

Uh oh!

heejaechang commented Jan 29, 2018

Uh oh!

minestarks commented Jan 29, 2018

Uh oh!

minestarks commented Jan 29, 2018

Uh oh!

heejaechang Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

heejaechang Jan 29, 2018

Choose a reason for hiding this comment

Uh oh!

brettfo commented Jan 29, 2018

Uh oh!

minestarks commented Jan 29, 2018

Uh oh!

Pilchie commented Jan 31, 2018

Uh oh!

heejaechang commented Jan 31, 2018

Uh oh!

jasonmalinowski commented Jan 31, 2018

Uh oh!

heejaechang commented Jan 31, 2018

Uh oh!

jasonmalinowski commented Feb 1, 2018

Uh oh!

heejaechang commented Feb 1, 2018

Uh oh!

heejaechang commented Feb 9, 2018

Uh oh!

heejaechang commented Feb 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

heejaechang commented Jan 29, 2018 •

edited by sharwell

Loading