Intellisense not showing methods from the Base class in Signature Help by akshita31 · Pull Request #1030 · OmniSharp/omnisharp-roslyn

akshita31 · 2017-11-23T02:32:58Z

During the search for the signature help, only the immediate Type that contains the given method was being searched for possible method overloads hence the overloads present in the Base Classes were not appearing. Added the code to search for members using the GetMemberGroup. Added a test for the same.

Please review @DustinCampbell @TheRealPiotrP @rchande

filipw · 2017-11-23T10:38:54Z

            }

-            return symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();
+            var MethodOverloads = symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();


local variables should start with lowercase

filipw · 2017-11-23T10:41:18Z

-            return symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();
+            var MethodOverloads = symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();
+            var BaseType = symbol.ContainingType.BaseType;
+            while(BaseType!=null && BaseType.ContainingNamespace.Name!= "System")


spacing is off here.
Also, perhaps it would be better to check for baseTypeSymbol.SpecialType != SpecialType.System_Object and terminate there instead rather than on "System" namespace

The above mentioned change is failing for the case of attribute constructor as the System.Attribute class doesn't have the SpecialType as System_Object, hence for that case we will also get an Attribute.Attribute method which is not required.This is the reason the System Namespace has been used.

rchande · 2017-11-27T16:57:30Z

-            return symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();
+            var methodOverloads = symbol.ContainingType.GetMembers(symbol.Name).OfType<IMethodSymbol>();
+            var baseTypeSymbol = symbol.ContainingType.BaseType;
+            while(baseTypeSymbol != null && baseTypeSymbol.ContainingNamespace.Name != "System")


It looks like Roslyn has a public "GetMemberGroup" extension method you can call that might do all of this for you. Should we just call that? (http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/SignatureHelp/InvocationExpressionSignatureHelpProvider.cs,71)

rchande · 2017-11-27T17:12:09Z

+            var baseTypeSymbol = symbol.ContainingType.BaseType;
+            while(baseTypeSymbol != null && baseTypeSymbol.ContainingNamespace.Name != "System")
+            {
+                methodOverloads = methodOverloads.Concat(baseTypeSymbol.GetMembers(symbol.Name).OfType<IMethodSymbol>());


I think this will add inaccessible members with the same name.

Yes it is returning all the methods regardless of their accessibility.

rchande · 2017-11-27T17:12:26Z

        }

+        [Fact]
+        public async Task SignatureHelpForOverloadedMethodsInheritance()


Can you add a case for methods with the same name that shouldn't be accessible? Eg:

class Program { static void Main(string[] args) { } private void Foo() { } } class G : Program { protected void Foo() { } }

rchande · 2017-11-27T19:02:00Z

+        }
+
+        [Fact]
+        public async Task SignatureHelpForOverloadedInaccesibleMethods()


Can you also add a test to verify behavior if there's an extension method with the same name?

DustinCampbell · 2017-11-27T22:43:50Z

+        }
+
+        [Fact]
+        public async Task SignatureHelpForInheritedInaccesibleMethods()


I'd like to see a few more tests for variants of this scenario:

An inherited protected method appears in signature help within the a sub type.

class A { protected void M1() { } } class B : A { void M2() { $$ // Both M1() and M2() } }

An inherited protected method appears in signature help after this..

class A { protected void M1() { } } class B : A { void M2() { this.$$ // Both M1() and M2() } }

An inherited protected method appears in signature help after base..

class A { protected void M1() { } } class B : A { void M2() { base.$$ // Only M1() } }

Only static methods appear in a static context.

class A { protected static void M1() { } public void M2() { } } class B : A { static void M3() { A.$$ // Only M1() } public void M4() { } }

class A { protected static void M1() { } public void M2() { } } class B : A { static void M3() { B.$$ // Both M1() and M3() } public void M4() { } }

Static and instance methods appear in an instance context.

class A { protected static void M1() { } public void M2() { } } class B : A { static void M3() { $$ // M1(), M2(), M3() and M4() appear } public void M4() { } }

DustinCampbell · 2017-11-29T14:04:33Z

+
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
+    <LangVersion>7.1</LangVersion>
+  </PropertyGroup>


Instead of adding this to a particular project, could you remove this properties here and just add <LangVersion>7.1</LangVersion> to https://github.com/OmniSharp/omnisharp-roslyn/blob/master/build/Settings.props so that all projects are using C# 7.1?

DustinCampbell · 2017-11-29T14:05:05Z

+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using System;
+using System.Collections.Generic;
+using System.Linq;


Sort usings. System namespaces go first. (Note that this is a setting in Visual Studio)

DustinCampbell · 2017-11-29T14:05:22Z

+using System;
+using System.Collections.Generic;
+using System.Linq;
+namespace OmniSharp.Roslyn.CSharp.Services.Signatures


Missing a line between the using directives and the namespace.

DustinCampbell · 2017-11-29T14:05:43Z

+using System.Linq;
+namespace OmniSharp.Roslyn.CSharp.Services.Signatures
+{
+    static internal class CheckForStatic


Sort modifiers. "internal" before "static"

By convention, classes containing extension methods are typically named with an Extensions suffix?

DustinCampbell · 2017-11-29T14:06:02Z

+            // any other location is considered static
+            return true;
+        }
+        public static SyntaxTokenList GetModifiers(SyntaxNode member)


Missing a blank line between methods.

DustinCampbell · 2017-11-29T14:11:45Z

+                {
+                    var throughExpression = ((MemberAccessExpressionSyntax)invocation.Receiver).Expression;
+                    throughSymbol = invocation.SemanticModel.GetSpeculativeSymbolInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsExpression).Symbol;
+                    throughType = invocation.SemanticModel.GetSpeculativeTypeInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsTypeOrNamespace).Type;


Why do we need to retrieve speculative symbol and type info here?

Our heuristic is to show instance members if the thing left of the dot can bind as a non-type and to show static members if the thing left of the dot can bind as a type (and both, in the case of Color Color).

Is this just copied and pasted from Roslyn?

Roslyn uses a different API for doing the same thing.

Roslyn does awkward type lookup to achieve the same effect: http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp.Features/SignatureHelp/InvocationExpressionSignatureHelpProvider_MethodGroup.cs,44. @DustinCampbell does what they are doing seem materially different to you?

Yup. It's similar. This looks fine.

DustinCampbell · 2017-11-29T14:12:48Z

+                    throughSymbol = invocation.SemanticModel.GetSpeculativeSymbolInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsExpression).Symbol;
+                    throughType = invocation.SemanticModel.GetSpeculativeTypeInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsTypeOrNamespace).Type;
+                    var includeInstance = throughSymbol != null && !(throughSymbol is ITypeSymbol);
+                    var includeStatic = (throughSymbol is INamedTypeSymbol) || throughType != null;


Why are the two clauses in the statements above reversed? Was that intentional? Why are we checking for null? Doesn't the is check already do that for you?

In the first case if we remove the null check,then if throughSymbol is null,the second statement will evaluate to true (and we want it to be false on a null).
In the second case one statement is checking throughSymbol and the other throughType, so we need to have both of them.

I see it now -- thanks!

DustinCampbell · 2017-11-29T14:13:04Z

+                    methodGroup = methodGroup.Where(m => (m.IsStatic && includeStatic) || (!m.IsStatic && includeInstance));
+                }
+
+                else if (invocation.Receiver is SimpleNameSyntax && invocation.IsStatic)


Unnecessary blank line above this.

DustinCampbell · 2017-11-29T14:14:56Z

+
+ }";
+            var actual = await GetSignatureHelp(source);
+            Assert.Single(actual.Signatures);


Should we verify which signature we got?

Yes we should.Added that.

DustinCampbell · 2017-11-29T14:16:05Z

+            var actual = await GetSignatureHelp(source);
+            Assert.Equal(4, actual.Signatures.Count());
+        }
+        [Fact]


Missing a blank line before this method

rchande · 2017-11-29T16:06:42Z

        public SyntaxNode Receiver { get; }
        public IEnumerable<TypeInfo> ArgumentTypes { get; }
        public IEnumerable<SyntaxToken> Separators { get; }
+        public bool IsStatic { get; } 


IsInStaticContext

rchande · 2017-11-29T16:10:02Z

+                {
+                    var throughExpression = ((MemberAccessExpressionSyntax)invocation.Receiver).Expression;
+                    throughSymbol = invocation.SemanticModel.GetSpeculativeSymbolInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsExpression).Symbol;
+                    throughType = invocation.SemanticModel.GetSpeculativeTypeInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsTypeOrNamespace).Type;


Our heuristic is to show instance members if the thing left of the dot can bind as a non-type and to show static members if the thing left of the dot can bind as a type (and both, in the case of Color Color).

rchande · 2017-11-29T16:13:09Z

                {
                    var semanticModel = await document.GetSemanticModelAsync();
-                    return new InvocationContext(semanticModel, position, invocation.Expression, invocation.ArgumentList);
+                    return new InvocationContext(semanticModel, position, invocation.Expression, invocation.ArgumentList, invocation.IsInStaticContext());


Can InvocationContext take care of calling IsInStaticContext()? You've making the same call in all the initializations.

In that case we will have to pass the invocation object to InvocationContext and since there are three different types of such objects we will have to add another constructor in InvocationContext. Which is better of the two ?

Ok, I'll leave it up to you.

rchande · 2017-11-29T16:13:29Z

                {
                    var semanticModel = await document.GetSemanticModelAsync();
-                    return new InvocationContext(semanticModel, position, objectCreation, objectCreation.ArgumentList);
+                    return new InvocationContext(semanticModel, position, objectCreation, objectCreation.ArgumentList,objectCreation.IsInStaticContext());


Missing comma between objectCreation.ArgumentList and objectCreation

DustinCampbell · 2017-11-29T21:40:55Z

@@ -1,4 +1,6 @@
-using System;
+// Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.


You might add a comment for where this was copied from.

rchande · 2017-12-07T00:04:23Z

+
+namespace OmniSharp.Roslyn.CSharp.Services.Signatures
+{
+    //TO DO: Reomove this class once a public API for Signature Help from Roslyn is available


Reomove -> Remove

DustinCampbell · 2017-12-07T14:31:43Z

+                {
+                    var throughExpression = ((MemberAccessExpressionSyntax)invocation.Receiver).Expression;
+                    throughSymbol = invocation.SemanticModel.GetSpeculativeSymbolInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsExpression).Symbol;
+                    throughType = invocation.SemanticModel.GetSpeculativeTypeInfo(invocation.Position, throughExpression, SpeculativeBindingOption.BindAsTypeOrNamespace).Type;


Yup. It's similar. This looks fine.

akshita31 added 2 commits November 22, 2017 17:07

Added code to check for the methods in the base classes

30c5092

Made changes to avoid the classes in System Namespace and added test

de3de6a

filipw reviewed Nov 23, 2017

View reviewed changes

Changed local variables to lowerCase and added spacing

ef0352a

rchande suggested changes Nov 27, 2017

View reviewed changes

akshita31 added 3 commits November 27, 2017 10:42

Test for Inaccesible Methods

eff068d

Using GetMemberGroup

cf8f790

Unnecessary using removed

6362fda

rchande reviewed Nov 27, 2017

View reviewed changes

Test for Overloaded Extension Methods

997d104

DustinCampbell reviewed Nov 27, 2017

View reviewed changes

akshita31 added 2 commits November 27, 2017 17:27

Changes for handling Extension Methods

41d4d2c

Added test for Access Modifiers

2c0bc6f

akshita31 force-pushed the base_class_func branch from cbfe81a to 132b636 Compare November 29, 2017 00:34

Changes for static invocations

9cb58b4

akshita31 force-pushed the base_class_func branch from 132b636 to 9cb58b4 Compare November 29, 2017 00:37

DustinCampbell requested changes Nov 29, 2017

View reviewed changes

rchande suggested changes Nov 29, 2017

View reviewed changes

akshita31 added 2 commits November 29, 2017 13:12

Removed unnecessary Blank Lines and Renamed IsInStaticContext

07cee28

Added copyright and todo

29b3803

DustinCampbell reviewed Nov 29, 2017

View reviewed changes

akshita31 added 3 commits November 29, 2017 14:04

Merge branch 'master' into base_class_func

2e1e0a0

Changed Language Version

cda2c28

Added Attribution

a13a6b0

akshita31 changed the title ~~Intellisense not showing methods from the Base class while parameters are suggested~~ Intellisense not showing methods from the Base class in Signature Help Nov 30, 2017

rchande reviewed Dec 7, 2017

View reviewed changes

Corrected spelling

17f52cf

rchande approved these changes Dec 7, 2017

View reviewed changes

DustinCampbell approved these changes Dec 7, 2017

View reviewed changes

Merge branch 'master' into base_class_func

fbc4c5f

DustinCampbell merged commit eae75dc into OmniSharp:master Dec 8, 2017

This was referenced Dec 8, 2017

Signature Help for Local Functions #1052

Merged

Internal Constructor from another project appears in autocomplete/intellisense. dotnet/vscode-csharp#1664

Closed

akshita31 deleted the base_class_func branch December 9, 2017 01:34

		@@ -1,4 +1,6 @@
		using System;
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

Conversation

akshita31 commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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!

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!

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!

rchande Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akshita31 commented Nov 23, 2017 •

edited

Loading

rchande Dec 7, 2017 •

edited

Loading