Omnisharp VS Code Issue 1814 Fix #1007

akshita31 · 2017-11-07T19:19:13Z

The issue was arising because there was no code to handle the AttributeSyntax type node(which is the case for the Attribute Constructors). But there was a difference between the type of arguments being used by the InvocationExpressionSyntax class and the AttributeSyntax class.Hence instead of keeping the entire Arguments object, we pulled out the relevant properties at the initial stage itself, and a change was made to the InvocationContext class accordingly. A test case has also been added to verify the behavior.
Please review @rchande @TheRealPiotrP @DustinCampbell .

david-driscoll

Minor changes from me, will let @DustinCampbell review too.

david-driscoll · 2017-11-07T19:22:41Z

package-lock.json

@@ -0,0 +1,3 @@
+{


should probably drop this file. 😄

david-driscoll · 2017-11-07T19:23:22Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -96,27 +97,43 @@ public SignatureHelpService(OmniSharpWorkspace workspace)
                var invocation = node as InvocationExpressionSyntax;
                if (invocation != null && invocation.ArgumentList.Span.Contains(position))
                {
+                    var i = await document.GetSemanticModelAsync();


probably give i a slightly better name.

Call this semanticaModel and replace the call to await document.GetSemanticModelAsync() below with the variable as well. There's no need to call it twice.

david-driscoll · 2017-11-07T19:23:45Z

tests/OmniSharp.Roslyn.CSharp.Tests/FindImplementationFacts.cs

@@ -153,6 +153,7 @@ public class MyClass
            }
        }

+


Extra whitespace

DustinCampbell

Nice work fixing the bug! There's just some code clean up left to do.

DustinCampbell · 2017-11-07T19:31:01Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

@@ -1,13 +1,21 @@
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
+using System.Collections.Generic;


nit: System namespaces should be sorted on top.

DustinCampbell · 2017-11-07T19:33:11Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -96,27 +97,43 @@ public SignatureHelpService(OmniSharpWorkspace workspace)
                var invocation = node as InvocationExpressionSyntax;
                if (invocation != null && invocation.ArgumentList.Span.Contains(position))
                {
+                    var i = await document.GetSemanticModelAsync();


Call this semanticaModel and replace the call to await document.GetSemanticModelAsync() below with the variable as well. There's no need to call it twice.

DustinCampbell · 2017-11-07T19:33:54Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                    return new InvocationContext()
                    {
                        SemanticModel = await document.GetSemanticModelAsync(),
                        Position = position,
                        Receiver = invocation.Expression,
-                        ArgumentList = invocation.ArgumentList
+                        ArgumentTypes = invocation.ArgumentList.Arguments.Select(argument => i.GetTypeInfo(argument.Expression)),


Is the expression here getting complex enough to extract into a local?

DustinCampbell · 2017-11-07T19:34:25Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                var objectCreation = node as ObjectCreationExpressionSyntax;
                if (objectCreation != null && objectCreation.ArgumentList.Span.Contains(position))
                {
+                    var i = await document.GetSemanticModelAsync();


semanticModel rather than i

DustinCampbell · 2017-11-07T19:34:33Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                    return new InvocationContext()
                    {
                        SemanticModel = await document.GetSemanticModelAsync(),
                        Position = position,
                        Receiver = invocation.Expression,
-                        ArgumentList = invocation.ArgumentList
+                        ArgumentTypes = invocation.ArgumentList.Arguments.Select(argument => i.GetTypeInfo(argument.Expression)),
+                        Separators = invocation.ArgumentList.Arguments.GetSeparators()


same comment as above

DustinCampbell · 2017-11-07T19:34:40Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                        Position = position,
                        Receiver = objectCreation,
-                        ArgumentList = objectCreation.ArgumentList
+                        ArgumentTypes = objectCreation.ArgumentList.Arguments.Select(argument => i.GetTypeInfo(argument.Expression)),


Same as above

@akshita31 we can extract this logic into an extension method or a private function.

I'd consider adding two constructor overloads to InvocationContext which will deal with this initialization. If we want to keep InvocationContext a POCO [Plain Old C# Object] then we can put this logic into a factory method instead. I don't have a strong preference.

DustinCampbell · 2017-11-07T19:35:08Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

+                        ArgumentTypes = attributeSyntax.ArgumentList.Arguments.Select(argument => i.GetTypeInfo(argument.Expression)),
+                        Separators = attributeSyntax.ArgumentList.Arguments.GetSeparators()
+                    };
+                }


Same comments as above.

Could you avoid copying and pasting the same code multiple times?

DustinCampbell · 2017-11-07T19:35:14Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                        Position = position,
                        Receiver = objectCreation,
-                        ArgumentList = objectCreation.ArgumentList
+                        ArgumentTypes = objectCreation.ArgumentList.Arguments.Select(argument => i.GetTypeInfo(argument.Expression)),
+                        Separators = objectCreation.ArgumentList.Arguments.GetSeparators()


DustinCampbell · 2017-11-07T19:36:15Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            Assert.Single(signature.Parameters);
+            Assert.Equal("value", signature.Parameters.ElementAt(0).Name);
+            Assert.Equal("int value", signature.Parameters.ElementAt(0).Label);
+


unnecessary blank line

TheRealPiotrP · 2017-11-07T20:45:02Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs


 namespace OmniSharp.Roslyn.CSharp.Services.Signatures
 {
+


lots of extra whitespace here.

TheRealPiotrP · 2017-11-07T20:45:49Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -115,7 +115,7 @@ public SignatureHelpFacts(ITestOutputHelper output)
    public static void Main(){
        Foo($$);
    }
-    pubic static Foo(bool b, int n = 1234)
+    public static Foo(bool b, int n = 1234)


have we verified that this code was not intentionally invalid previously?

No, didn't verify that. Should I revert it back to the previous state ?

I wouldn't think so. This doesn't look intentional to me based on the test name.

You should look at the test and decide whether the typo was intentional :)

My guess, based on what's being tested, is that it was unintentional.

TheRealPiotrP · 2017-11-07T20:46:53Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            int value;
+            public MyTestAttribute(int value)
+            {
+                this.value =value;


please add a space between = and value

TheRealPiotrP · 2017-11-07T20:47:02Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            void TestMethod(int value){
+                this.value = value;
+            }
+            public class MyTestAttribute : Attribute {


newline here

C# code typically puts opening { on a new line. Consider using VS to format the document as this will get your style in-line with C#.

TheRealPiotrP · 2017-11-07T20:49:16Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -153,6 +153,42 @@ private int Foo(int one, int two, int three)
        }

        [Fact]
+        public async Task AttributeConstructorSignatureHelp()


@rchande the tests for signatures in Constructor/Method invocations cover a bunch of scenarios. Is this single test sufficient to cover attribute signatures?

@akshita31 the test name here seems misaligned with the names of other tests in this file. Since we're replicating a test behavior in a new syntax node, can you align the test names between this and constructor/method signatures?

I was confused by the "1st position" comment, but looks like there is plenty of other coverage.

TheRealPiotrP · 2017-11-07T20:51:00Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

-                var types = invocation.ArgumentList.Arguments
-                    .Select(argument => invocation.SemanticModel.GetTypeInfo(argument.Expression));
+                var types = invocation.ArgumentTypes;
+


Unnecessary empty lines

TheRealPiotrP

Thanks @akshita31! The change looks good overall. Like others, I have some style feedback to keep the code in-line with the rest of the repo. I'd also like to be sure that you and @rchande are comfortable with the level of test coverage here. Thanks for the contribution!

rchande

Looks pretty good--minor nits.

rchande · 2017-11-07T19:22:58Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs


 namespace OmniSharp.Roslyn.CSharp.Services.Signatures
 {
+


Blank lines

rchande · 2017-11-07T19:23:03Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

    }
+
+


Blank lines

rchande · 2017-11-07T19:23:11Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

}
+

rchande · 2017-11-07T19:27:59Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -96,27 +97,43 @@ public SignatureHelpService(OmniSharpWorkspace workspace)
                var invocation = node as InvocationExpressionSyntax;
                if (invocation != null && invocation.ArgumentList.Span.Contains(position))
                {
+                    var i = await document.GetSemanticModelAsync();


rchande · 2017-11-07T19:28:36Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                    return new InvocationContext()
                    {
-                        SemanticModel = await document.GetSemanticModelAsync(),
+                        SemanticModel = i,


Oh, I see--can you make this consistent with above, either with "i" or just calling GetSemanticModel?

Also "i" is probably not the best name :)

rchande · 2017-11-07T19:29:08Z

tests/OmniSharp.Roslyn.CSharp.Tests/FindImplementationFacts.cs

@@ -153,6 +153,7 @@ public class MyClass
            }
        }

+


rchande · 2017-11-07T19:29:17Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -115,7 +115,7 @@ public SignatureHelpFacts(ITestOutputHelper output)
    public static void Main(){
        Foo($$);
    }
-    pubic static Foo(bool b, int n = 1234)


rchande · 2017-11-07T19:51:31Z

package-lock.json

@@ -0,0 +1,3 @@
+{
+  "lockfileVersion": 1


Intenentional?

rchande · 2017-11-07T19:52:29Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            const string source =
+                @"using System;
+
+        namespace New_folder


Can you align the quoted string to the left margin? Makes it easier to tell what the test contents are

rchande · 2017-11-07T19:53:00Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -153,6 +153,42 @@ private int Foo(int one, int two, int three)
        }

        [Fact]
+        public async Task AttributeConstructorSignatureHelp()


What about the second parameter, or if there aren't any?

DustinCampbell · 2017-11-07T22:26:00Z

@akshita31 : Your last commit didn't compile (you didn't replace references to i with semanticModel).

…arp-roslyn into vscode_iss_1814

rchande · 2017-11-07T23:42:03Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -192,7 +192,7 @@ public MyTestAttribute(int value)
            Assert.Equal("int value", signature.Parameters.ElementAt(0).Label);
        }
        [Fact]
-        public async Task SignatureHelpforAttCtorMultipleArg()
+        public async Task SignatureHelpforAttCtorMultipleParam()
        {
            const string source =
 @"using System;


Does this pass? It doesn't look like there's a "$$" anywhere in the markup?

My bad, I see it. Anyway, it seems like the test is called "MultipleParam", but you're only testing in the first parameter position.

rchande · 2017-11-07T23:43:52Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+    double value2;
+    public MyTestAttribute()
+    {
+        this.value1 = 1;


You can delete all the "value1" and "value2" related stuff since this test doesn't use them.

rchande · 2017-11-07T23:46:29Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

@@ -153,7 +153,7 @@ private int Foo(int one, int two, int three)
        }

        [Fact]
-        public async Task SignatureHelpforAttCtorSingleArg()
+        public async Task SignatureHelpforAttCtorSingleParam()


I would prefer that we not abbreviate "attribute".

rchande · 2017-11-08T00:53:56Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

                }
                node = node.Parent;
            }
-


Don't remove this line (the rule is to leave a blank line between a close brace and a statement).

rchande · 2017-11-08T00:55:26Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

-        public IEnumerable<SyntaxToken> Separators { get; set; } 
+        public IEnumerable<SyntaxToken> Separators { get; set; }
+
+        public InvocationContext(SemanticModel SemModel,int Pos,SyntaxNode Rec, ArgumentListSyntax ArgList)


lowerCase parameter names.
space after comma
what does "rec" mean?

rchande · 2017-11-08T00:56:20Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

@@ -11,7 +12,24 @@ internal class InvocationContext
        public int Position { get; set; }


Not your fault, but do these properties actually need to be mutable? Can we have them be get-only?

Yes , it works with get-only.

rchande · 2017-11-08T00:59:06Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+}
+public class MyTestAttribute : Attribute 
+{
+    int value1;


Up to you, but you can remove the fields and their assignment, if you want to make the test code even simpler.

…essary details in tests

rchande · 2017-11-08T01:31:13Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

+        public SemanticModel SemanticModel { get; }
+        public int Position { get; }
+        public SyntaxNode Receiver { get; }
+        public IEnumerable<TypeInfo> ArgumentTypes { get;  }


Extra space

rchande · 2017-11-08T01:32:46Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+public class MyTestAttribute : Attribute 
+{
+    int value1;
+    double value2;


You can remove these.

rchande · 2017-11-08T01:33:09Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+public class MyTestAttribute : Attribute 
+{
+    int value1;
+    double value2;


This can still be removed?

DustinCampbell · 2017-11-08T09:28:43Z

This is looking much better! Just a couple more tiny nits from me and that package-lock.json file looks like it was included unintentionally.

rchande · 2017-11-08T16:44:40Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

+                    return new InvocationContext(semanticModel, position, objectCreation,objectCreation.ArgumentList);
+                }
+                var attributeSyntax = node as AttributeSyntax;
+                if (attributeSyntax != null && attributeSyntax.ArgumentList.Span.Contains(position))


You can make this a little cleaner by using a pattern match expression:
if (node is AttributeSyntax attributeSyntax && attributeSyntax ... )

rchande · 2017-11-08T16:46:13Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -74,7 +72,6 @@ public SignatureHelpService(OmniSharpWorkspace workspace)
                    }
                }
            }
-


Leave this blank line here (the brace is followed by a statement).

rchande · 2017-11-08T18:38:58Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/SignatureHelpService.cs

@@ -90,24 +91,25 @@ public SignatureHelpService(OmniSharpWorkspace workspace)
            // Walk up until we find a node that we're interested in.
            while (node != null)
            {
-                var invocation = node as InvocationExpressionSyntax;
-                if (invocation != null && invocation.ArgumentList.Span.Contains(position))
+


My bad, should have been more specific. Blank line between close brace and statement :).

DustinCampbell

Just a couple more nits and I'm OK with this change.

DustinCampbell · 2017-11-08T09:25:32Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            Assert.Equal("value", signature.Parameters.ElementAt(0).Name);
+            Assert.Equal("int value", signature.Parameters.ElementAt(0).Label);
+        }
+        [Fact]


Missing blank line before method. Should be between close brace and [Fact]

DustinCampbell · 2017-11-08T09:25:54Z

tests/OmniSharp.Roslyn.CSharp.Tests/SignatureHelpFacts.cs

+            Assert.Equal("value2", signature.Parameters.ElementAt(1).Name);
+            Assert.Equal("double value2", signature.Parameters.ElementAt(1).Label);
+        }
+        [Fact]


Missing blank line before method. Should be between close brace and [Fact]

rchande

LGTM. Thanks!

DustinCampbell · 2017-11-08T19:40:07Z

src/OmniSharp.Roslyn.CSharp/Services/Signatures/InvocationContext.cs

+            ArgumentTypes = argList.Arguments.Select(argument => semModel.GetTypeInfo(argument.Expression));
+            Separators = argList.Arguments.GetSeparators();
+        }
+        public InvocationContext(SemanticModel semModel, int position, SyntaxNode receiver, AttributeArgumentListSyntax argList)


Blank line after before this constructor please!

DustinCampbell · 2017-11-08T19:54:40Z

@TheRealPiotrP : Have your requested changes been addressed?

akshita31 added 2 commits November 7, 2017 10:53

Vscode Issue 1814 Fix

868c002

Vscode 1814 fix

debcfec

david-driscoll reviewed Nov 7, 2017

View reviewed changes

DustinCampbell requested changes Nov 7, 2017

View reviewed changes

TheRealPiotrP reviewed Nov 7, 2017

View reviewed changes

TheRealPiotrP suggested changes Nov 7, 2017

View reviewed changes

rchande approved these changes Nov 7, 2017

View reviewed changes

Iss 1814 - Changes Made

71a318b

akshita31 added 4 commits November 7, 2017 14:41

Changed Test Names and added a new test for multiple arguments

e74060a

Merge branch 'master' into vscode_iss_1814

f8e6896

Added more Test Cases

94459ae

Merge branch 'vscode_iss_1814' of https://github.com/akshita31/omnish…

c33df39

…arp-roslyn into vscode_iss_1814

rchande reviewed Nov 7, 2017

View reviewed changes

akshita31 added 2 commits November 7, 2017 16:16

Added Overloaded Constructors for InvocationContext

cd8954f

Renamed Tests and added new test for active parameter

978efe6

rchande reviewed Nov 8, 2017

View reviewed changes

Modified Properties in InvocatioContext as Read-Only and removed unnc…

f7ac87b

…essary details in tests

rchande reviewed Nov 8, 2017

View reviewed changes

Removed unncessary variable declaration

ace2701

rchande reviewed Nov 8, 2017

View reviewed changes

Added Pattern Matching

9a1e814

rchande reviewed Nov 8, 2017

View reviewed changes

DustinCampbell approved these changes Nov 8, 2017

View reviewed changes

akshita31 added 2 commits November 8, 2017 11:09

Deleted package-lock.json

9445b4b

Added missing newline in tests

c91c3f0

rchande approved these changes Nov 8, 2017

View reviewed changes

DustinCampbell reviewed Nov 8, 2017

View reviewed changes

Added Blank Line

fff464b

david-driscoll approved these changes Nov 9, 2017

View reviewed changes

TheRealPiotrP approved these changes Nov 9, 2017

View reviewed changes

DustinCampbell merged commit 9d26eb4 into OmniSharp:master Nov 9, 2017

akshita31 deleted the vscode_iss_1814 branch November 9, 2017 21:16

		@@ -11,7 +12,24 @@ internal class InvocationContext
		public int Position { get; set; }

@@ @@ -74,7 +72,6 @@ public SignatureHelpService(OmniSharpWorkspace workspace) @@
                                   }
                               }
                           }

Omnisharp VS Code Issue 1814 Fix #1007

Omnisharp VS Code Issue 1814 Fix #1007

Conversation

akshita31 commented Nov 7, 2017

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealPiotrP left a comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Nov 8, 2017