Merge pull request microsoft#168 from AlexanderSher/master

Removed 'goto' from 'LineFormatter' and other code refactorings

Author: AlexanderSher

src/Analysis/Engine/Impl/Definitions/Position.cs

@@ -40,6 +40,6 @@ public struct Position {
         public static bool operator >(Position p1, Position p2) => p1.line > p2.line || p1.line == p2.line && p1.character > p2.character;
         public static bool operator <(Position p1, Position p2) => p1.line < p2.line || p1.line == p2.line && p1.character < p2.character;
 
-        public override string ToString() => ((SourceLocation)this).ToString();
+        public override string ToString() => $"({line}, {character})";
     }
 }

src/Analysis/Engine/Impl/Definitions/Range.cs

@@ -24,6 +24,6 @@ public struct Range {
         public static implicit operator SourceSpan(Range r) => new SourceSpan(r.start, r.end);
         public static implicit operator Range(SourceSpan span) => new Range { start = span.Start, end = span.End };
 
-        public override string ToString() => ((SourceSpan)this).ToString();
+        public override string ToString() => $"{start} - {end}";
     }
 }

src/Analysis/Engine/Impl/Infrastructure/Extensions/StringBuilderExtensions.cs

@@ -25,19 +25,19 @@ public static StringBuilder TrimEnd(this StringBuilder sb) {
             return sb;
         }
 
-        public static StringBuilder EnsureEndsWithSpace(this StringBuilder sb, int count = 1, bool allowLeading = false) {
-            if (sb.Length == 0 && !allowLeading) {
+        public static StringBuilder EnsureEndsWithWhiteSpace(this StringBuilder sb, int whiteSpaceCount = 1) {
+            if (sb.Length == 0) {
                 return sb;
             }
 
-            for (var i = sb.Length - 1; i >= 0 && char.IsWhiteSpace(sb[i]); i--) {
-                count--;
+            for (var i = sb.Length - 1; i >= 0 && whiteSpaceCount > 0 && char.IsWhiteSpace(sb[i]); i--) {
+                whiteSpaceCount--;
             }
 
-            if (count > 0) {
-                sb.Append(new string(' ', count));
+            if (whiteSpaceCount > 0) {
+                sb.Append(' ', whiteSpaceCount);
             }
-
+            
             return sb;
         }
     }

src/Analysis/Engine/Impl/Parsing/Tokenizer.cs

@@ -130,17 +130,9 @@ public List<TokenInfo> ReadTokens(int characterCount) {
             return tokens;
         }
 
-        public object CurrentState {
-            get {
-                return _state;
-            }
-        }
-
-        public SourceLocation CurrentPosition {
-            get {
-                return IndexToLocation(CurrentIndex);
-            }
-        }
+        public object CurrentState => _state;
+        public int CurrentLine => _newLineLocations.Count;
+        public SourceLocation CurrentPosition => IndexToLocation(CurrentIndex);
 
         public SourceLocation IndexToLocation(int index) {
             int match = _newLineLocations.BinarySearch(new NewLineLocation(index, NewLineKind.None));
@@ -2235,6 +2227,7 @@ private static void DumpToken(Token token) {
             Console.WriteLine("{0} `{1}`", token.Kind, token.Image.Replace("\r", "\\r").Replace("\n", "\\n").Replace("\t", "\\t"));
         }
 
+        internal NewLineLocation GetNewLineLocation(int line) => _newLineLocations.Count == line ? new NewLineLocation(CurrentIndex, NewLineKind.None) : _newLineLocations[line];
         internal NewLineLocation[] GetLineLocations() => _newLineLocations.ToArray();
         internal SourceLocation[] GetCommentLocations() => _commentLocations.ToArray();
 
@@ -2564,11 +2557,7 @@ private bool AtBeginning {
             }
         }
 
-        private int CurrentIndex {
-            get {
-                return _tokenStartIndex + Math.Min(_position, _end) - _start;
-            }
-        }
+        private int CurrentIndex => _tokenStartIndex + Math.Min(_position, _end) - _start;
 
         private void DiscardToken() {
             CheckInvariants();

src/Analysis/Engine/Test/FluentAssertions/AssertionsFactory.cs

@@ -102,5 +102,8 @@ public static SignatureHelpAssertions Should(this SignatureHelp signatureHelp)
 
         public static SignatureInformationAssertions Should(this SignatureInformation signatureInformation)
             => new SignatureInformationAssertions(signatureInformation);
+
+        public static TextEditCollectionAssertions Should(this IEnumerable<TextEdit> textEdits)
+            => new TextEditCollectionAssertions(textEdits);
     }
 }

src/Analysis/Engine/Test/FluentAssertions/AssertionsUtilities.cs

@@ -206,6 +206,9 @@ public static IEnumerable<IAnalysisValue> FlattenAnalysisValues(IEnumerable<IAna
             }
         }
 
+        public static bool RangeEquals(Range r1, Range r2) => PositionEquals(r1.start, r2.start) && PositionEquals(r1.end, r2.end);
+        public static bool PositionEquals(Position p1, Position p2) => p1.line == p2.line && p1.character == p2.character;
+
         public static string DoubleEscape(string input) 
             => input.Replace("\r", "\\\u200Br").Replace("\n", "\\\u200Bn").Replace("\t", @"\t");
 

src/Analysis/Engine/Test/FluentAssertions/ReferenceCollectionAssertions.cs

@@ -75,10 +75,10 @@ public AndConstraint<ReferenceCollectionAssertions> OnlyHaveReferences(IEnumerab
             if (excess.Length > 0) {
                 var excessString = string.Join(", ", excess.Select(Format));
                 var errorMessage = expected.Length > 1
-                    ? $"Expected {GetName()} to have only {expected.Length} references{{reason}}, but it also has references: {excessString}."
+                    ? $"Expected {GetSubjectName()} to have only {expected.Length} references{{reason}}, but it also has references: {excessString}."
                     : expected.Length > 0
-                        ? $"Expected {GetName()} to have only one reference{{reason}}, but it also has references: {excessString}."
-                        : $"Expected {GetName()} to have no references{{reason}}, but it has references: {excessString}.";
+                        ? $"Expected {GetSubjectName()} to have only one reference{{reason}}, but it also has references: {excessString}."
+                        : $"Expected {GetSubjectName()} to have no references{{reason}}, but it has references: {excessString}.";
 
                 Execute.Assertion.BecauseOf(because, reasonArgs).FailWith(errorMessage);
             }
@@ -87,10 +87,7 @@ public AndConstraint<ReferenceCollectionAssertions> OnlyHaveReferences(IEnumerab
         }
 
         private static string Format((Uri uri, (int, int, int, int) range, ReferenceKind? kind) reference) 
-            => $"({TestData.GetTestRelativePath(reference.uri)}, {Format(reference.range)}, {reference.kind})";
-
-        private static string Format((int startLine, int startCharacter, int endLine, int endCharacter) range) 
-            => $"({range.startLine}, {range.startCharacter}) - ({range.endLine}, {range.endCharacter})";
+            => $"({TestData.GetTestRelativePath(reference.uri)}, {reference.range.ToString()}, {reference.kind})";
 
         [CustomAssertion]
         public AndConstraint<ReferenceCollectionAssertions> HaveReferenceAt(Uri documentUri, int startLine, int startCharacter, int endLine, int endCharacter, ReferenceKind? referenceKind = null, string because = "", params object[] reasonArgs) {
@@ -117,38 +114,29 @@ public AndConstraint<ReferenceCollectionAssertions> HaveReferenceAt(Uri document
         private string FindReference(Uri documentUri, string moduleName, Range range, ReferenceKind? referenceKind = null) {
             var candidates = Subject.Where(av => Equals(av.uri, documentUri)).ToArray();
             if (candidates.Length == 0) {
-                return $"Expected {GetName()} to have reference in the module '{moduleName}'{{reason}}, but no references has been found.";
+                return $"Expected {GetSubjectName()} to have reference in the module '{moduleName}'{{reason}}, but no references has been found.";
             }
 
             foreach (var candidate in candidates.Where(c => RangeEquals(c.range, range))) {
                 return referenceKind.HasValue && candidate._kind != referenceKind
-                    ? $"Expected {GetName()} to have reference of type '{referenceKind}'{{reason}}, but reference in module '{moduleName}' at {RangeToString(range)} has type '{candidate._kind}'"
+                    ? $"Expected {GetSubjectName()} to have reference of type '{referenceKind}'{{reason}}, but reference in module '{moduleName}' at {range.ToString()} has type '{candidate._kind}'"
                     : string.Empty;
             }
 
-            var errorMessage = $"Expected {GetName()} to have reference at {RangeToString(range)}{{reason}}, but module '{moduleName}' has no references at that range.";
+            var errorMessage = $"Expected {GetSubjectName()} to have reference at {range.ToString()}{{reason}}, but module '{moduleName}' has no references at that range.";
             if (!referenceKind.HasValue) {
                 return errorMessage;
             }
 
             var matchingTypes = candidates.Where(av => av._kind == referenceKind).ToArray();
             var matchingTypesString = matchingTypes.Length > 0
-                ? $"References that match type '{referenceKind}' have spans {string.Join(" ,", matchingTypes.Select(av => RangeToString(av.range)))}"
+                ? $"References that match type '{referenceKind}' have spans {string.Join(" ,", matchingTypes.Select(av => av.range.ToString()))}"
                 : $"There are no references with type '{referenceKind}' either";
 
             return $"{errorMessage} {matchingTypesString}";
         }
-
-        private static string RangeToString(Range range)
-            => $"({range.start.line}, {range.start.character}) - ({range.end.line}, {range.end.character})";
-
-        private static bool RangeEquals(Range r1, Range r2)
-            => r1.start.line == r2.start.line
-               && r1.start.character == r2.start.character
-               && r1.end.line == r2.end.line
-               && r1.end.character == r2.end.character;
-
+        
         [CustomAssertion]
-        private static string GetName() => CallerIdentifier.DetermineCallerIdentity() ?? "collection";
+        private static string GetSubjectName() => CallerIdentifier.DetermineCallerIdentity() ?? "collection";
     }
 }

src/Analysis/Engine/Test/FluentAssertions/TextEditCollectionAssertions.cs

@@ -0,0 +1,113 @@
+// Python Tools for Visual Studio
+// Copyright(c) Microsoft Corporation
+// All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the License); you may not use
+// this file except in compliance with the License. You may obtain a copy of the
+// License at http://www.apache.org/licenses/LICENSE-2.0
+//
+// THIS CODE IS PROVIDED ON AN  *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS
+// OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY
+// IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE,
+// MERCHANTABLITY OR NON-INFRINGEMENT.
+//
+// See the Apache Version 2.0 License for specific language governing
+// permissions and limitations under the License.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using FluentAssertions;
+using FluentAssertions.Collections;
+using FluentAssertions.Execution;
+using Microsoft.Python.LanguageServer;
+using static Microsoft.PythonTools.Analysis.FluentAssertions.AssertionsUtilities;
+
+namespace Microsoft.PythonTools.Analysis.FluentAssertions {
+    [ExcludeFromCodeCoverage]
+    internal sealed class TextEditCollectionAssertions : SelfReferencingCollectionAssertions<TextEdit, TextEditCollectionAssertions> {
+        public TextEditCollectionAssertions(IEnumerable<TextEdit> references) : base(references) { }
+
+        protected override string Identifier => nameof(TextEdit) + "Collection";
+
+
+        [CustomAssertion]
+        public AndConstraint<TextEditCollectionAssertions> OnlyHaveTextEdit(string expectedText, (int startLine, int startCharacter, int endLine, int endCharacter) expectedRange, string because = "", params object[] reasonArgs)
+            => OnlyHaveTextEdits(new[] {(expectedText, expectedRange)}, because, reasonArgs);
+
+        [CustomAssertion]
+        public AndConstraint<TextEditCollectionAssertions> OnlyHaveTextEdits(params (string expectedText, (int startLine, int startCharacter, int endLine, int endCharacter) expectedRange)[] textEdits)
+            => OnlyHaveTextEdits(textEdits, string.Empty);
+
+        [CustomAssertion]
+        public AndConstraint<TextEditCollectionAssertions> OnlyHaveTextEdits(IEnumerable<(string expectedText, (int startLine, int startCharacter, int endLine, int endCharacter) expectedRange)> textEdits, string because = "", params object[] reasonArgs) {
+            var expected = textEdits.ToArray();
+            foreach (var (expectedText, (startLine, startCharacter, endLine, endCharacter)) in expected) {
+                HaveTextEditAt(expectedText, (startLine, startCharacter, endLine, endCharacter), because, reasonArgs);
+            }
+
+            var excess = Subject.Select(r => (r.newText, (r.range.start.line, r.range.start.character, r.range.end.line, r.range.end.character)))
+                .Except(expected)
+                .ToArray();
+
+            if (excess.Length > 0) {
+                var excessString = string.Join(", ", excess.Select(((string text, (int, int, int, int) range) te) => $"({te.text}, {te.range.ToString()})"));
+                var errorMessage = expected.Length > 1
+                    ? $"Expected {GetSubjectName()} to have only {expected.Length} textEdits{{reason}}, but it also has textEdits: {excessString}."
+                    : expected.Length > 0
+                        ? $"Expected {GetSubjectName()} to have only one reference{{reason}}, but it also has textEdits: {excessString}."
+                        : $"Expected {GetSubjectName()} to have no textEdits{{reason}}, but it has textEdits: {excessString}.";
+
+                Execute.Assertion.BecauseOf(because, reasonArgs).FailWith(errorMessage);
+            }
+
+            return new AndConstraint<TextEditCollectionAssertions>(this);
+        }
+
+        [CustomAssertion]
+        public AndConstraint<TextEditCollectionAssertions> HaveTextEditAt(string expectedText, (int startLine, int startCharacter, int endLine, int endCharacter) expectedRange, string because = "", params object[] reasonArgs) {
+            var range = new Range {
+                start = new Position { line = expectedRange.startLine, character = expectedRange.startCharacter },
+                end = new Position { line = expectedRange.endLine, character = expectedRange.endCharacter }
+            };
+
+            var errorMessage = GetHaveTextEditErrorMessage(expectedText, range);
+            if (errorMessage != string.Empty) {
+                var assertion = Execute.Assertion.BecauseOf(because, reasonArgs);
+                assertion.AddNonReportable("expectedText", expectedText);
+                assertion.AddNonReportable("expectedRange", range);
+                assertion.AddNonReportable("currentTexts", GetQuotedNames(Subject.Select(te => te.newText)));
+                assertion.FailWith(errorMessage);
+            }
+
+            return new AndConstraint<TextEditCollectionAssertions>(this);
+        }
+
+        [CustomAssertion]
+        private string GetHaveTextEditErrorMessage(string expectedText, Range expectedRange) {
+            var candidates = Subject.Where(av => string.Equals(av.newText, expectedText, StringComparison.Ordinal)).ToArray();
+            if (candidates.Length == 0) {
+                return "Expected {context:subject} to have text edit with newText \'{expectedText}\'{reason}, but "
+                    + (Subject.Any() ? "it has {currentTexts}" : "it is empty");
+            }
+
+            var candidatesWithRange = candidates.Where(c => RangeEquals(c.range, expectedRange)).ToArray();
+            if (candidatesWithRange.Length > 1) {
+                return $"Expected {{context:subject}} to have only one text edit with newText '{{expectedText}}' and range {{expectedRange}}{{reason}}, but there are {candidatesWithRange.Length}";
+            }
+
+            if (candidatesWithRange.Length == 0) {
+                return "Expected {context:subject} to have text edit with newText \'{expectedText}\' in range {expectedRange}{reason}, but "
+                    + (candidates.Length == 1
+                        ? $"it has range {candidates[0].range.ToString()}"
+                        : $"they are in ranges {string.Join(", ", candidates.Select(te => te.range.ToString()))}");
+            }
+
+            return string.Empty;
+        }
+
+        [CustomAssertion]
+        private static string GetSubjectName() => CallerIdentifier.DetermineCallerIdentity() ?? "collection";
+    }
+}

src/Analysis/Engine/Test/LanguageServerTests.cs

@@ -1010,31 +1010,13 @@ public async Task OnTypeFormatting() {
                 // Extended tests for line formatting are in LineFormatterTests.
                 // These just verify that the language server formats and returns something correct.
                 var edits = await s.SendDocumentOnTypeFormatting(uri, new SourceLocation(2, 1), "\n");
-                edits.Should().OnlyContain(new TextEdit {
-                    newText = "def foo():",
-                    range = new Range {
-                        start = new SourceLocation(1, 1),
-                        end = new SourceLocation(1, 15)
-                    }
-                });
+                edits.Should().OnlyHaveTextEdit("def foo():", (0, 0, 0, 14));
 
                 edits = await s.SendDocumentOnTypeFormatting(uri, new SourceLocation(3, 1), "\n");
-                edits.Should().OnlyContain(new TextEdit {
-                    newText = "x = a + b",
-                    range = new Range {
-                        start = new SourceLocation(2, 5),
-                        end = new SourceLocation(2, 14)
-                    }
-                });
+                edits.Should().OnlyHaveTextEdit("x = a + b", (1, 4, 1, 13));
 
                 edits = await s.SendDocumentOnTypeFormatting(uri, new SourceLocation(4, 1), "\n");
-                edits.Should().OnlyContain(new TextEdit {
-                    newText = "x += 1",
-                    range = new Range {
-                        start = new SourceLocation(3, 5),
-                        end = new SourceLocation(3, 10)
-                    }
-                });
+                edits.Should().OnlyHaveTextEdit("x += 1", (2, 4, 2, 9));
             }
         }