Interface completion provider shows objects fix

[SeanFarrow]

Fixing issue #15988 to prevent methods from the object base class showing up when the completion list is requested from the ExplicitInterfaceCompletionProvider.

[CyrusNajmabadi]

Hi Sean,

I pulled down your branch and had no problem exercising your test. As it doesn't do much, it passes simply. If i add something like an Assert.True(false); call in it, then it fails as expected.

What are you seeing yourself?

[CyrusNajmabadi]

Example of the error i get when i add a failing assert to your test:

Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders.ExplicitInterfaceCompletionProviderTests.EnsureNoObjectMembersAreShown FAILED:
	Exception type: 'Xunit.Sdk.TrueException', number: '0', parent: '-1'
	Exception message:
Assert.True() Failure
Expected: True
Actual:   False
	Exception stacktrace
   at Xunit.Assert.True(Nullable`1 condition, String userMessage) in C:\BuildAgent\work\cb37e9acf085d108\src\xunit.assert\Asserts\BooleanAsserts.cs:line 95
   at Xunit.Assert.True(Boolean condition) in C:\BuildAgent\work\cb37e9acf085d108\src\xunit.assert\Asserts\BooleanAsserts.cs:line 62
   at Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders.ExplicitInterfaceCompletionProviderTests.<EnsureNoObjectMembersAreShown>d__5.MoveNext() in C:\GitHub\roslyn-internal\Open\src\EditorFeatures\CSharpTest\Completion\CompletionProviders\ExplicitInterfaceCompletionProviderTests.cs:line 116
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\BuildAgent\work\cb37e9acf085d108\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 260
...

[SeanFarrow]

Hi, Ok, given that the test run was so long, and I’m visually impaired, I might have missed the assembly running tests—I was just seeing it pass everything. I’ll alter the test so I can see what I need and it fails. Is there a way to stop the build if a test fails? This might make it easier to find a failing test. Cheers Sean. From: CyrusNajmabadi [mailto:notifications@github.com] Sent: Monday, April 10, 2017 09:57 To: dotnet/roslyn <roslyn@noreply.github.com> Cc: Sean Farrow <sean.farrow@seanfarrow.co.uk>; Author <author@noreply.github.com> Subject: Re: [dotnet/roslyn] Interface completion provider shows objects fix (#18574) Hi Sean, I pulled down your branch and had no problem exercising your test. As it doesn't do much, it passes simply. If i add something like an Assert.True(false); call in it, then it fails as expected. What are you seeing yourself? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#18574 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABY1fketnYjKzeKEgtJ_PHia7kET3u0Xks5rue7WgaJpZM4M4cdV>.

[CyrusNajmabadi]

Hi Sean! How are you currently executing tests? If you use xunit.runner.console you should be able to pass certain args to get it to test only a specific test, or a set of tests with a provided "trait". Let me see if i can hunt down some specific instructions on how to do this!

[SeanFarrow]

Hi, I’m currently just using: Restore Build Then test It would be nice if we could filter out what test/assemblies would run using this infrastructure, perhaps a new issue is required? From: CyrusNajmabadi [mailto:notifications@github.com] Sent: Monday, April 10, 2017 10:05 To: dotnet/roslyn <roslyn@noreply.github.com> Cc: Sean Farrow <sean.farrow@seanfarrow.co.uk>; Author <author@noreply.github.com> Subject: Re: [dotnet/roslyn] Interface completion provider shows objects fix (#18574) Hi Sean! How are you currently executing tests? If you use xunit.runner.console you should be able to pass certain args to get it to test only a specific test, or a set of tests with a provided "trait". Let me see if i can hunt down some specific instructions on how to do this! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#18574 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABY1fnrqTMTruj9JSPfEb3Of_UxumP-Fks5rufC8gaJpZM4M4cdV>.

[CyrusNajmabadi]

I see. Here's what you can do from teh command line to make things a lot easier:

xunit.console.x86.exe C:\GitHub\roslyn-internal\Open\Binaries\Debug\UnitTests\CSharpEditorServicesTest\Roslyn.Ser
vices.Editor.CSharp.UnitTests.dll  -method Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders
.ExplicitInterfaceCompletionProviderTests.EnsureNoObjectMembersAreShown
xUnit.net Console Runner (32-bit .NET 4.0.30319.42000)
  Discovering: Roslyn.Services.Editor.CSharp.UnitTests
  Discovered:  Roslyn.Services.Editor.CSharp.UnitTests
  Starting:    Roslyn.Services.Editor.CSharp.UnitTests
Period index is 41
    Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders.ExplicitInterfaceCompletionProviderTests.EnsureNoObjectMembersAreShown [FAIL]
      Assert.True() Failure
      Expected: True
      Actual:   False
      Stack Trace:
        src\EditorFeatures\CSharpTest\Completion\CompletionProviders\ExplicitInterfaceCompletionProviderTests.cs(116,0): at Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Completion.CompletionProviders.ExplicitInterfaceCompletionProviderTests.<EnsureNoObjectMembersAreShown>d__5.MoveNext()

Here i'm just asking xunit to run the tests for this particular assembly, and i'm only asking it to run that specific test in that assembly.

[CyrusNajmabadi]

It would be nice if we could filter out what test/assemblies would run using this infrastructure, perhaps a new issue is required?

That sounds like a totally reasonable suggestion. I'm unfamiliar with that part of our infrastructure, but maybe someone would be able to make that happen. In the meantime though, i hope that the specific xunit.runner instructions will be able to unblock you!

[SeanFarrow]

Hi, Are we restricted to using batch files, or could we use something like Cake for the build—I’m happy to help convert the build if you want as this is something due to a project I have quite a bit of experience in. I’ll raise another issue and if there’s somebody else I need to work with feel free to cc them! Cheers Sean. From: CyrusNajmabadi [mailto:notifications@github.com] Sent: Monday, April 10, 2017 10:14 To: dotnet/roslyn <roslyn@noreply.github.com> Cc: Sean Farrow <sean.farrow@seanfarrow.co.uk>; Author <author@noreply.github.com> Subject: Re: [dotnet/roslyn] Interface completion provider shows objects fix (#18574) It would be nice if we could filter out what test/assemblies would run using this infrastructure, perhaps a new issue is required? That sounds like a totally reasonable suggestion. I'm unfamiliar with that part of our infrastructure, but maybe someone would be able to make that happen. In the meantime though, i hope that the specific xunit.runner instructions will be able to unblock you! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#18574 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABY1fj8YnS1KLEtg0VEt943s1E-YJj6Fks5rufLTgaJpZM4M4cdV>.

[CyrusNajmabadi]

Hi Sean! I'm not really the best person to answer those types of questions. :-) I recommend raising the issue, and tagging dotnet/roslyn-infrastructure and jaredpar to discuss things with.

I will say that changing our infrastructure is non trivial. But if significant value can be demonstrated, it may be possible!

[SeanFarrow]

Hi Cyrus, Ok, I’ve got this test written as I’d expect, but it’s not failing as I would like. I suspect this is because I don’t understand the way tests/the VerifyItemIsAbsentAsync method works. Also, I’m unable to debug this as there is an OperationCancelledException in one of the diagnostic tests being thrown, so my test never gets to the stage where I can debug it. Any ideas how I can get my test to fail/debug the test without he other test causing an issue? Cheers Sean. From: CyrusNajmabadi [mailto:notifications@github.com] Sent: Monday, April 10, 2017 10:14 To: dotnet/roslyn <roslyn@noreply.github.com> Cc: Sean Farrow <sean.farrow@seanfarrow.co.uk>; Author <author@noreply.github.com> Subject: Re: [dotnet/roslyn] Interface completion provider shows objects fix (#18574) It would be nice if we could filter out what test/assemblies would run using this infrastructure, perhaps a new issue is required? That sounds like a totally reasonable suggestion. I'm unfamiliar with that part of our infrastructure, but maybe someone would be able to make that happen. In the meantime though, i hope that the specific xunit.runner instructions will be able to unblock you! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#18574 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABY1fj8YnS1KLEtg0VEt943s1E-YJj6Fks5rufLTgaJpZM4M4cdV>.

[CyrusNajmabadi]

Hey Sean,

The original suggestion that a test be written in ExplicitInterfaceCompletionProviderTests.cs was based on the supposition that the problem lies in the ExplicitInterfaceCompletionProvider itself. This supposition may be incorrect.

In order to figure out where the right test would go, you'll actually need to figure out where the problem is arising that's causing the errant members to be shown. My psychic debugging hat tells me that it's highly likely that the extra entries would be created by either the ExplicitInterfaceCompletionProvider or the SymbolCompletionProvider. As we've tentatively eliminated the former, the latter is probably the most likely cause of the issue.

I recommend actually trying to debug through the scenario to see what is producing this errant item. That will then help us determine the right fix, which will finally help direct where the tests should go.

Also, I’m unable to debug this as there is an OperationCancelledException in one of the diagnostic tests being thrown, so my test never gets to the stage where I can debug it.

I'm not quite understanding that. Cancellation-exceptions shouldn't prevent a test from being debuggable.

[SeanFarrow]

Hi, Having looked at this, I don’t seem to get all object members. I’ve debugged through this and get some object members. What makes you think it’s in the SymbolCompletionProvider? I don’t see any references to this at all in the ExplicitCompletionProvider code. Cheers Sean. From: CyrusNajmabadi [mailto:notifications@github.com] Sent: Monday, April 10, 2017 14:51 To: dotnet/roslyn <roslyn@noreply.github.com> Cc: Sean Farrow <sean.farrow@seanfarrow.co.uk>; Author <author@noreply.github.com> Subject: Re: [dotnet/roslyn] Interface completion provider shows objects fix (#18574) Hey Sean, The original suggestion that a test be written in ExplicitInterfaceCompletionProviderTests.cs was based on the supposition that the problem lies in the ExplicitInterfaceCompletionProvider itself. This supposition may be incorrect. In order to figure out where the right test would go, you'll actually need to figure out where the problem is arising that's causing the errant members to be shown. My psychic debugging hat tells me that it's highly likely that the extra entries would be created by either the ExplicitInterfaceCompletionProvider or the SymbolCompletionProvider. As we've tentatively eliminated the former, the latter is probably the most likely cause of the issue. I recommend actually trying to debug through the scenario to see what is producing this errant item. That will then help us determine the right fix, which will finally help direct where the tests should go. Also, I’m unable to debug this as there is an OperationCancelledException in one of the diagnostic tests being thrown, so my test never gets to the stage where I can debug it. I'm not quite understanding that. Cancellation-exceptions shouldn't prevent a test from being debuggable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#18574 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABY1fgkLma6rhd26uTakva8KDmZ96goVks5rujO5gaJpZM4M4cdV>.

[CyrusNajmabadi]

What makes you think it’s in the SymbolCompletionProvider?

Just a gut feeling, given that you said that the ExplicitInterfaceMemberCompletionProvider did not report any results.

However, when i debug through here, i see that it is the ExplicitInterfaceMemberCompletionProvider returning these:

>	Microsoft.CodeAnalysis.CSharp.Completion.Providers.ExplicitInterfaceMemberCompletionProvider.ProvideCompletionsAsync(context)	C#
 	Microsoft.CodeAnalysis.Completion.CompletionServiceWithProviders.GetContextAsync(provider, document, position, triggerInfo, options, defaultSpan, cancellationToken)	C#
 	Microsoft.CodeAnalysis.Completion.CompletionServiceWithProviders.ComputeNonEmptyCompletionContextsAsync(document, caretPosition, trigger, options, defaultItemSpan, providers, cancellationToken)	C#
 	Microsoft.CodeAnalysis.Completion.CompletionServiceWithProviders.GetCompletionsAsync(document, caretPosition, trigger, roles, options, cancellationToken)	C#
 	Microsoft.CodeAnalysis.Editor.Implementation.IntelliSense.Completion.Controller.Session.ModelComputer.GetCompletionListAsync(completionService, trigger, cancellationToken)	C#

When we call:

            var members = semanticModel.LookupSymbols(
                position: name.SpanStart,
                container: symbol)
                    .WhereAsArray(s => !s.IsStatic)
                    .FilterToVisibleAndBrowsableSymbols(options.GetOption(CompletionOptions.HideAdvancedMembers, semanticModel.Language), semanticModel.Compilation);

We get a list back with 5 members, including members of object.

IMO, it's wrong for us to even be calling LookupMembers here. We've got the interface symbol already in the "symbol" variable. What we can do at that point is simply call .GetMembers on that, filtering down to just events, properties and methods.

[CyrusNajmabadi]

@SeanFarrow Here is the problem line:

https://github.com/dotnet/roslyn/blob/master/src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.cs#L82

[jaredpar]

We apologize, but we are closing this PR due to code drift. We regret letting this PR go so long without attention, but at this point the code base has changed too much for us to revisit older PRs. Going forward, we will manage PRs better under an SLA currently under draft in issue #26266 – we encourage you to add comments to that draft as you see fit. Although that SLA is still in draft form, we nevertheless want to move forward with the identification of older PRs to give contributors as much early notice as possible to review and update them.

If you are interested in pursuing this PR, please reset it against the head of master and we will reopen it. Thank you!