Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add editor formatting service to auto-deindent closing brackets #3313

Merged
merged 10 commits into from
Sep 6, 2017
1 change: 1 addition & 0 deletions vsintegration/src/FSharp.Editor/FSharp.Editor.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
<Compile Include="Classification\ColorizationService.fs" />
<Compile Include="Formatting\BraceMatchingService.fs" />
<Compile Include="Formatting\IndentationService.fs" />
<Compile Include="Formatting\EditorFormattingService.fs" />
<Compile Include="Debugging\BreakpointResolutionService.fs" />
<Compile Include="Debugging\LanguageDebugInfoService.fs" />
<Compile Include="Diagnostics\DocumentDiagnosticAnalyzer.fs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,16 @@ type internal FSharpBraceMatchingService
projectInfoManager: ProjectInfoManager
) =

static let userOpName = "BraceMatching"
static member GetBraceMatchingResult(checker: FSharpChecker, sourceText, fileName, options, position: int) =

static let defaultUserOpName = "BraceMatching"

static member GetBraceMatchingResult(checker: FSharpChecker, sourceText, fileName, options, position: int, userOpName: string) =
async {
let! matchedBraces = checker.MatchBraces(fileName, sourceText.ToString(), options, userOpName = userOpName)
let! matchedBraces = checker.MatchBraces(fileName, sourceText.ToString(), options, userOpName)
let isPositionInRange range =
match RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, range) with
| None -> false
| Some range -> range.Contains(position)
| Some range -> position - range.Start <= range.Length
return matchedBraces |> Array.tryFind(fun (left, right) -> isPositionInRange left || isPositionInRange right)
}

Expand All @@ -31,7 +33,7 @@ type internal FSharpBraceMatchingService
asyncMaybe {
let! options = projectInfoManager.TryGetOptionsForEditingDocumentOrProject(document)
let! sourceText = document.GetTextAsync(cancellationToken)
let! (left, right) = FSharpBraceMatchingService.GetBraceMatchingResult(checkerProvider.Checker, sourceText, document.Name, options, position)
let! (left, right) = FSharpBraceMatchingService.GetBraceMatchingResult(checkerProvider.Checker, sourceText, document.Name, options, position, defaultUserOpName)
let! leftSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, left)
let! rightSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, right)
return BraceMatchingResult(leftSpan, rightSpan)
Expand Down
101 changes: 101 additions & 0 deletions vsintegration/src/FSharp.Editor/Formatting/EditorFormattingService.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Copyright (c) Microsoft Corporation. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.VisualStudio.FSharp.Editor

open System.Composition
open System.Collections.Generic

open Microsoft.CodeAnalysis
open Microsoft.CodeAnalysis.Editor
open Microsoft.CodeAnalysis.Host.Mef
open Microsoft.CodeAnalysis.Text

open Microsoft.FSharp.Compiler.SourceCodeServices
open System.Threading

[<Shared>]
[<ExportLanguageService(typeof<IEditorFormattingService>, FSharpConstants.FSharpLanguageName)>]
type internal FSharpEditorFormattingService
[<ImportingConstructor>]
(
checkerProvider: FSharpCheckerProvider,
projectInfoManager: ProjectInfoManager
) =

static member GetFormattingChanges(document: Document, sourceText: SourceText, checker: FSharpChecker, optionsOpt: FSharpProjectOptions option, position: int) =
// Logic should be:
// If first token on the current line is a closing brace,
// match the indent with the indent on the line that opened it

asyncMaybe {
let! options = optionsOpt

let line = sourceText.Lines.[sourceText.Lines.IndexOf position]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is realistic or would ever get hit, but max 0 (sourceText.Lines.IndexOf position) would be useful.

Perhaps this is just me being paranoid after #3180.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IndexOf throws if the position isn't within the text, but as Roslyn has already supplied us with the position I think it's impossible that we can't find the line in the source text (also supplied by Roslyn).


let defines = CompilerEnvironment.GetCompilationDefinesForEditing(document.FilePath, options.OtherOptions |> List.ofArray)

let tokens = Tokenizer.tokenizeLine(document.Id, sourceText, line.Start, document.FilePath, defines)

let! firstMeaningfulToken =
tokens
|> List.tryFind (fun x ->
x.Tag <> FSharpTokenTag.WHITESPACE &&
x.Tag <> FSharpTokenTag.COMMENT &&
x.Tag <> FSharpTokenTag.LINE_COMMENT)

let! (left, right) =
FSharpBraceMatchingService.GetBraceMatchingResult(checker, sourceText, document.FilePath, options, position, "FormattingService")

if right.StartColumn = firstMeaningfulToken.LeftColumn then
// Replace the indentation on this line with the indentation of the left bracket
let! leftSpan = RoslynHelpers.TryFSharpRangeToTextSpan(sourceText, left)

let indentChars (line : TextLine) =
line.ToString()
|> Seq.takeWhile ((=) ' ')
|> Seq.length

let startIndent = indentChars sourceText.Lines.[sourceText.Lines.IndexOf leftSpan.Start]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, or perhaps check this before hand if a startIndent of 0 would cause trouble?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with startIndent 0 and everything's fine :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

let currentIndent = indentChars line

return TextChange(TextSpan(line.Start, currentIndent), String.replicate startIndent " ")
else
return! None
}

member __.GetFormattingChangesAsync (document: Document, position: int, cancellationToken: CancellationToken) =
async {
let! sourceText = document.GetTextAsync(cancellationToken) |> Async.AwaitTask
let optionsOpt = projectInfoManager.TryGetOptionsForEditingDocumentOrProject document
let! textChange = FSharpEditorFormattingService.GetFormattingChanges(document, sourceText, checkerProvider.Checker, optionsOpt, position)

return
match textChange with
| Some change ->
List([change]) :> IList<_>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally use the name ResizeArray in F# code, but it's no big deal (not block merge on this)

| None ->
List() :> IList<_>
}

interface IEditorFormattingService with
member val SupportsFormatDocument = false
member val SupportsFormatSelection = false
member val SupportsFormatOnPaste = false
member val SupportsFormatOnReturn = true

override __.SupportsFormattingOnTypedCharacter (_document, ch) =
match ch with
| ')' | ']' | '}' -> true
| _ -> false

override __.GetFormattingChangesAsync (_document, _span, _cancellationToken) = null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of null in interface implementations always makes me a little nervous, could you add a comment indicating why this is valid, perhaps with a reference to a line in the Roslyn source that checks the null return value and does something sensible with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roslyn checks whether SupportsFormatSelection is true or SupportsFormatDocument is true before calling this. You're right though - I can just make it return an empty enumerable.


override __.GetFormattingChangesOnPasteAsync (_document, _span, _cancellationToken) = null

override this.GetFormattingChangesAsync (document, _typedChar, position, cancellationToken) =
this.GetFormattingChangesAsync (document, position, cancellationToken)
|> RoslynHelpers.StartAsyncAsTask cancellationToken

override this.GetFormattingChangesOnReturnAsync (document, position, cancellationToken) =
this.GetFormattingChangesAsync (document, position, cancellationToken)
|> RoslynHelpers.StartAsyncAsTask cancellationToken