Don't crash when invoking signature help on an inaccessible item. by brettfo · Pull Request #4157 · dotnet/roslyn

brettfo · 2015-07-28T00:06:52Z

As reported by a customer, when attempting to show signature help on an inaccessible item can cause a crash. The fix is: when checking if we're looking for instance members, also consider the containing type of the invocation expression, not just if it's name is visible.

Question for reviewers:
Consider the following code (taken from the unit test):

class A
{
    List<int> args;
}

class B : A
{
    void M()
    {
        args.Add$$
    }
}

There is no crash in VB, but the args symbol also doesn't bind to begin with, like it does in C#. Given this scenario it can either be argued that when the user typed args they knew exactly what they were doing and they intend to update the definition of args to be public or protected, or, they were unaware of the private field and they might be surprised by the completion/sig-help sessions that follow the dot. Thoughts?

As it stands now, Add(int item) appears in the completion list after typing the dot in C#, but nothing appears in VB.

Edit
After digging in a bit, VB does show the appropriate completion items, but I wasn't originally seeing that due to some unnoticed changes on my box.

brettfo · 2015-07-28T00:08:23Z

Tagging for review: @Pilchie @DustinCampbell @dpoeschl @davkean @jasonmalinowski @balajikris @basoundr @jmarolf @rchande

DustinCampbell · 2015-07-28T00:14:28Z

WRT your question, I think the right approach is to show the inaccessible signature. The user will get an error after using it anyway. Maybe we'll add a code fix someday to change the accessibility of the member. That would make this a smooth experience.

For VB, I think it's OK if the compiler has already removed the inaccessible members and doesn't consider them. However, if the IDE is removing the members, I think we should relax that. How do others feel? @Pilchie?

jasonmalinowski · 2015-07-28T00:16:17Z

I agree with @DustinCampbell -- let's show it.

DustinCampbell · 2015-07-28T00:21:05Z

So, what was the crash in the repro? It's not clear to me how this fixes a crash. Also, can throughSymbol.ContainingType be null? How does that impact this situation?

The crash occurred on the next line, Contract.ThrowIfFalse() that was unhandled, and this makes the boolean true where it should be. As for ContainingType, the default to the method is null anyways and that simply means 'from this context', similar to the line above that doesn't specify the container, so it defaults to the class containing the method, B in this case.

Got it. I didn't realize any of the methods on Contract compiled in release. Good to know.

I'm still thinking about this - I understand that this fixes the issue, but doesn't feel quite right for some reason.

From my perspective, we really shouldn't be failing fast and crashing the user's machine within signature help providers. In my observation, signature help can get all sorts of mangled trees and the providers should be somewhat resilient and just return null when things bind in ways we don't expect. I'd prefer a debug-only assert over crashing VS and causing the user to lose their unsaved changes.

Just my 2 cents.

I agree with the not crashing bit. One of my concerns is the existence of this IncludeStatic and IncludeInstance code - we have a bunch of it in Completion that @rchande was just touching due to late regressions in VB. Should we share that code?

The code here is basically asking the same two questions: "could this name have bound to a type?" and "could this name have bound to a non-type?". I could see us adding the extension methods ISymbol.CouldHaveBoundAsType(position, name) and ISymbol.CouldHaveBoundAsSomethingOtherThanAType(position, name) and using them in at least the 3 places that do this check.

brettfo · 2015-07-29T17:22:00Z

retest this please

brettfo · 2015-07-31T18:23:45Z

Pinging for additional feedback/signoff given that we had a hallway discussion yesterday that this fix should just be targeted at fixing the crash and not about making signature help crash-proof.

jasonmalinowski · 2015-07-31T18:26:56Z

Nitpick: name the test for what it's testing about inaccessible items, since without looking I could imagine this would either assert they are there or they aren't there.

Updated in 8a0e1f8.

Pilchie · 2015-08-03T14:07:21Z

👍

…ider the left expression explicitly, not just by name

Don't crash when invoking signature help on an inaccessible item.

dnfclas added the cla-already-signed label Jul 28, 2015

brettfo added the Area-IDE label Jul 28, 2015

DustinCampbell reviewed Jul 28, 2015
View reviewed changes

brettfo force-pushed the sig-help-inaccessible branch from 4bb86d6 to da94be6 Compare July 31, 2015 17:50

jasonmalinowski reviewed Jul 31, 2015
View reviewed changes

When gathering symbols for invocation expression signature help, cons…

19a1358

…ider the left expression explicitly, not just by name

brettfo force-pushed the sig-help-inaccessible branch from 8a0e1f8 to 19a1358 Compare August 3, 2015 17:30

brettfo added a commit that referenced this pull request Aug 3, 2015

Merge pull request #4157 from brettfo/sig-help-inaccessible

849b664

Don't crash when invoking signature help on an inaccessible item.

brettfo merged commit 849b664 into dotnet:master Aug 3, 2015

brettfo deleted the sig-help-inaccessible branch August 3, 2015 18:29

brettfo mentioned this pull request Sep 4, 2015

Unexpected False Crash #5011

Closed

Conversation

brettfo commented Jul 28, 2015

Uh oh!

brettfo commented Jul 28, 2015

Uh oh!

DustinCampbell commented Jul 28, 2015

Uh oh!

jasonmalinowski commented Jul 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettfo commented Jul 29, 2015

Uh oh!

brettfo commented Jul 31, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pilchie commented Aug 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants