From 3fb590e17fd36bca12c42047cea4c54edef8f308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blak?= Date: Mon, 20 Jan 2020 15:41:43 +0100 Subject: [PATCH 1/5] Expose simplify name analyzer --- src/fsharp/service/ServiceAnalysis.fs | 63 +++++++++++++++++ src/fsharp/service/ServiceAnalysis.fsi | 6 +- .../SimplifyNameDiagnosticAnalyzer.fs | 68 +++---------------- 3 files changed, 79 insertions(+), 58 deletions(-) diff --git a/src/fsharp/service/ServiceAnalysis.fs b/src/fsharp/service/ServiceAnalysis.fs index e568767a1e8..0e6b30f24e7 100644 --- a/src/fsharp/service/ServiceAnalysis.fs +++ b/src/fsharp/service/ServiceAnalysis.fs @@ -226,4 +226,67 @@ module UnusedOpens = let symbolUses = splitSymbolUses symbolUses let openStatements = getOpenStatements checkFileResults.OpenDeclarations return! filterOpenStatements symbolUses openStatements + } + +module SimplifyNames = + let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length + + /// Get all ranges that can be simplified in a file + let getUnnecessaryRanges (checkFileResults: FSharpCheckFileResults, getSourceLineStr: int -> string, sleep: int option) : Async<(range*string) list> = + async { + let result = ResizeArray() + let! symbolUses = checkFileResults.GetAllUsesOfAllSymbolsInFile() + let symbolUses = + symbolUses + |> Array.filter (fun symbolUse -> not symbolUse.IsFromOpenStatement) + |> Array.Parallel.map (fun symbolUse -> + let lineStr = getSourceLineStr symbolUse.RangeAlternate.StartLine + // for `System.DateTime.Now` it returns ([|"System"; "DateTime"|], "Now") + let partialName = QuickParse.GetPartialLongNameEx(lineStr, symbolUse.RangeAlternate.EndColumn - 1) + // `symbolUse.RangeAlternate.Start` does not point to the start of plid, it points to start of `name`, + // so we have to calculate plid's start ourselves. + let plidStartCol = symbolUse.RangeAlternate.EndColumn - partialName.PartialIdent.Length - (getPlidLength partialName.QualifyingIdents) + symbolUse, partialName.QualifyingIdents, plidStartCol, partialName.PartialIdent) + |> Array.filter (fun (_, plid, _, name) -> name <> "" && not (List.isEmpty plid)) + |> Array.groupBy (fun (symbolUse, _, plidStartCol, _) -> symbolUse.RangeAlternate.StartLine, plidStartCol) + |> Array.map (fun (_, xs) -> xs |> Array.maxBy (fun (symbolUse, _, _, _) -> symbolUse.RangeAlternate.EndColumn)) + + for symbolUse, plid, plidStartCol, name in symbolUses do + if not symbolUse.IsFromDefinition then + let posAtStartOfName = + let r = symbolUse.RangeAlternate + if r.StartLine = r.EndLine then Range.mkPos r.StartLine (r.EndColumn - name.Length) + else r.Start + + let getNecessaryPlid (plid: string list) : Async = + let rec loop (rest: string list) (current: string list) = + async { + match rest with + | [] -> return current + | headIdent :: restPlid -> + let! res = checkFileResults.IsRelativeNameResolvableFromSymbol(posAtStartOfName, current, symbolUse.Symbol) + if res then return current + else return! loop restPlid (headIdent :: current) + } + loop (List.rev plid) [] + + if sleep.IsNone then + do! Async.Sleep sleep.Value // be less intrusive, give other work priority most of the time + + let! necessaryPlid = getNecessaryPlid plid + + match necessaryPlid with + | necessaryPlid when necessaryPlid = plid -> () + | necessaryPlid -> + let r = symbolUse.RangeAlternate + let necessaryPlidStartCol = r.EndColumn - name.Length - (getPlidLength necessaryPlid) + + let unnecessaryRange = + Range.mkRange r.FileName (Range.mkPos r.StartLine plidStartCol) (Range.mkPos r.EndLine necessaryPlidStartCol) + + let relativeName = (String.concat "." plid) + "." + name + result.Add(unnecessaryRange, relativeName) + + + return List.ofSeq result } \ No newline at end of file diff --git a/src/fsharp/service/ServiceAnalysis.fsi b/src/fsharp/service/ServiceAnalysis.fsi index 8d976443890..656050acd0f 100644 --- a/src/fsharp/service/ServiceAnalysis.fsi +++ b/src/fsharp/service/ServiceAnalysis.fsi @@ -8,4 +8,8 @@ open FSharp.Compiler.Range module public UnusedOpens = /// Get all unused open declarations in a file - val getUnusedOpens : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async \ No newline at end of file + val getUnusedOpens : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async + +module public SimplifyNames = + /// Get all ranges that can be simplified in a file + val getUnnecessaryRanges : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) * sleep: int option -> Async<(range*string) list> \ No newline at end of file diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs index f90717522a5..ad307dccbf0 100644 --- a/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs +++ b/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs @@ -16,6 +16,7 @@ open FSharp.Compiler.Range open System.Runtime.Caching open Microsoft.CodeAnalysis.Host.Mef open Microsoft.CodeAnalysis.ExternalAccess.FSharp.Diagnostics +open FSharp.Compiler.SourceCodeServices type private TextVersionHash = int type private PerDocumentSavedData = { Hash: int; Diagnostics: ImmutableArray } @@ -26,7 +27,7 @@ type internal SimplifyNameDiagnosticAnalyzer [] () = static let userOpName = "SimplifyNameDiagnosticAnalyzer" let getProjectInfoManager (document: Document) = document.Project.Solution.Workspace.Services.GetService().FSharpProjectOptionsManager let getChecker (document: Document) = document.Project.Solution.Workspace.Services.GetService().Checker - let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length + static let cache = new MemoryCache("FSharp.Editor." + userOpName) // Make sure only one document is being analyzed at a time, to be nice static let guard = new SemaphoreSlim(1) @@ -52,62 +53,15 @@ type internal SimplifyNameDiagnosticAnalyzer [] () = let! sourceText = document.GetTextAsync() let checker = getChecker document let! _, _, checkResults = checker.ParseAndCheckDocument(document, projectOptions, sourceText = sourceText, userOpName=userOpName) - let! symbolUses = checkResults.GetAllUsesOfAllSymbolsInFile() |> liftAsync - let mutable result = ResizeArray() - let symbolUses = - symbolUses - |> Array.filter (fun symbolUse -> not symbolUse.IsFromOpenStatement) - |> Array.Parallel.map (fun symbolUse -> - let lineStr = sourceText.Lines.[Line.toZ symbolUse.RangeAlternate.StartLine].ToString() - // for `System.DateTime.Now` it returns ([|"System"; "DateTime"|], "Now") - let partialName = QuickParse.GetPartialLongNameEx(lineStr, symbolUse.RangeAlternate.EndColumn - 1) - // `symbolUse.RangeAlternate.Start` does not point to the start of plid, it points to start of `name`, - // so we have to calculate plid's start ourselves. - let plidStartCol = symbolUse.RangeAlternate.EndColumn - partialName.PartialIdent.Length - (getPlidLength partialName.QualifyingIdents) - symbolUse, partialName.QualifyingIdents, plidStartCol, partialName.PartialIdent) - |> Array.filter (fun (_, plid, _, name) -> name <> "" && not (List.isEmpty plid)) - |> Array.groupBy (fun (symbolUse, _, plidStartCol, _) -> symbolUse.RangeAlternate.StartLine, plidStartCol) - |> Array.map (fun (_, xs) -> xs |> Array.maxBy (fun (symbolUse, _, _, _) -> symbolUse.RangeAlternate.EndColumn)) - - for symbolUse, plid, plidStartCol, name in symbolUses do - if not symbolUse.IsFromDefinition then - let posAtStartOfName = - let r = symbolUse.RangeAlternate - if r.StartLine = r.EndLine then Range.mkPos r.StartLine (r.EndColumn - name.Length) - else r.Start - - let getNecessaryPlid (plid: string list) : Async = - let rec loop (rest: string list) (current: string list) = - async { - match rest with - | [] -> return current - | headIdent :: restPlid -> - let! res = checkResults.IsRelativeNameResolvableFromSymbol(posAtStartOfName, current, symbolUse.Symbol, userOpName=userOpName) - if res then return current - else return! loop restPlid (headIdent :: current) - } - loop (List.rev plid) [] - - do! Async.Sleep DefaultTuning.SimplifyNameEachItemDelay |> liftAsync // be less intrusive, give other work priority most of the time - let! necessaryPlid = getNecessaryPlid plid |> liftAsync - - match necessaryPlid with - | necessaryPlid when necessaryPlid = plid -> () - | necessaryPlid -> - let r = symbolUse.RangeAlternate - let necessaryPlidStartCol = r.EndColumn - name.Length - (getPlidLength necessaryPlid) - - let unnecessaryRange = - Range.mkRange r.FileName (Range.mkPos r.StartLine plidStartCol) (Range.mkPos r.EndLine necessaryPlidStartCol) - - let relativeName = (String.concat "." plid) + "." + name - result.Add( - Diagnostic.Create( - descriptor, - RoslynHelpers.RangeToLocation(unnecessaryRange, sourceText, document.FilePath), - properties = (dict [SimplifyNameDiagnosticAnalyzer.LongIdentPropertyKey, relativeName]).ToImmutableDictionary())) - - let diagnostics = result.ToImmutableArray() + let! result = SimplifyNames.getUnnecessaryRanges(checkResults, (fun lineNumber -> sourceText.Lines.[Line.toZ lineNumber].ToString()), Some (DefaultTuning.SimplifyNameEachItemDelay) ) |> liftAsync + let mutable diag = ResizeArray() + for (unnecessaryRange,relativeName) in result do + diag.Add( + Diagnostic.Create( + descriptor, + RoslynHelpers.RangeToLocation(unnecessaryRange, sourceText, document.FilePath), + properties = (dict [SimplifyNameDiagnosticAnalyzer.LongIdentPropertyKey, relativeName]).ToImmutableDictionary())) + let diagnostics = diag.ToImmutableArray() cache.Remove(key) |> ignore let data = { Hash = textVersionHash; Diagnostics=diagnostics } let cacheItem = CacheItem(key, data) From 194b39bfd6fe02032eb9b40ac8da4018d298268f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blak?= Date: Mon, 20 Jan 2020 15:56:58 +0100 Subject: [PATCH 2/5] Move IsPrivateToFile to the FSharpSymbolUse from extensions --- src/fsharp/symbols/Symbols.fs | 25 ++++++++++++++++++ src/fsharp/symbols/Symbols.fsi | 3 +++ .../FSharp.Editor/LanguageService/Symbols.fs | 26 ------------------- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/src/fsharp/symbols/Symbols.fs b/src/fsharp/symbols/Symbols.fs index 295e42b02cb..f4b2f6babe6 100644 --- a/src/fsharp/symbols/Symbols.fs +++ b/src/fsharp/symbols/Symbols.fs @@ -2518,5 +2518,30 @@ type FSharpSymbolUse(g:TcGlobals, denv: DisplayEnv, symbol:FSharpSymbol, itemOcc member __.Range = Range.toZ range member __.RangeAlternate = range + + member this.IsPrivateToFile = + let isPrivate = + match this.Symbol with + | :? FSharpMemberOrFunctionOrValue as m -> not m.IsModuleValueOrMember || m.Accessibility.IsPrivate + | :? FSharpEntity as m -> m.Accessibility.IsPrivate + | :? FSharpGenericParameter -> true + | :? FSharpUnionCase as m -> m.Accessibility.IsPrivate + | :? FSharpField as m -> m.Accessibility.IsPrivate + | _ -> false + + let declarationLocation = + match this.Symbol.SignatureLocation with + | Some x -> Some x + | _ -> + match this.Symbol.DeclarationLocation with + | Some x -> Some x + | _ -> this.Symbol.ImplementationLocation + + let declaredInTheFile = + match declarationLocation with + | Some declRange -> declRange.FileName = this.RangeAlternate.FileName + | _ -> false + + isPrivate && declaredInTheFile override __.ToString() = sprintf "%O, %O, %O" symbol itemOcc range diff --git a/src/fsharp/symbols/Symbols.fsi b/src/fsharp/symbols/Symbols.fsi index 6893097ed94..33d80108789 100644 --- a/src/fsharp/symbols/Symbols.fsi +++ b/src/fsharp/symbols/Symbols.fsi @@ -1101,3 +1101,6 @@ type public FSharpSymbolUse = /// The range of text representing the reference to the symbol member RangeAlternate: range + ///Indicates if the reference is private to the file + member IsPrivateToFile : bool + diff --git a/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs b/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs index a3d9b784526..dd7359d08fd 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs +++ b/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs @@ -77,32 +77,6 @@ type FSharpSymbolUse with | projects -> Some (SymbolDeclarationLocation.Projects (projects, isSymbolLocalForProject)) | None -> None - member this.IsPrivateToFile = - let isPrivate = - match this.Symbol with - | :? FSharpMemberOrFunctionOrValue as m -> not m.IsModuleValueOrMember || m.Accessibility.IsPrivate - | :? FSharpEntity as m -> m.Accessibility.IsPrivate - | :? FSharpGenericParameter -> true - | :? FSharpUnionCase as m -> m.Accessibility.IsPrivate - | :? FSharpField as m -> m.Accessibility.IsPrivate - | _ -> false - - let declarationLocation = - match this.Symbol.SignatureLocation with - | Some x -> Some x - | _ -> - match this.Symbol.DeclarationLocation with - | Some x -> Some x - | _ -> this.Symbol.ImplementationLocation - - let declaredInTheFile = - match declarationLocation with - | Some declRange -> declRange.FileName = this.RangeAlternate.FileName - | _ -> false - - isPrivate && declaredInTheFile - - type FSharpMemberOrFunctionOrValue with member x.IsConstructor = x.CompiledName = ".ctor" From 6c350955f75335599982173c05a85a7be71de287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blak?= Date: Mon, 20 Jan 2020 16:01:02 +0100 Subject: [PATCH 3/5] Expose Unused Declarations analyzer --- src/fsharp/service/ServiceAnalysis.fs | 71 +++++++++++++++++++ src/fsharp/service/ServiceAnalysis.fsi | 6 +- .../Diagnostics/UnusedDeclarationsAnalyzer.fs | 65 +---------------- 3 files changed, 77 insertions(+), 65 deletions(-) diff --git a/src/fsharp/service/ServiceAnalysis.fs b/src/fsharp/service/ServiceAnalysis.fs index 0e6b30f24e7..5e559632508 100644 --- a/src/fsharp/service/ServiceAnalysis.fs +++ b/src/fsharp/service/ServiceAnalysis.fs @@ -289,4 +289,75 @@ module SimplifyNames = return List.ofSeq result + } + +module UnusedDeclarations = + let isPotentiallyUnusedDeclaration (symbol: FSharpSymbol) : bool = + match symbol with + // Determining that a record, DU or module is used anywhere requires inspecting all their enclosed entities (fields, cases and func / vals) + // for usages, which is too expensive to do. Hence we never gray them out. + | :? FSharpEntity as e when e.IsFSharpRecord || e.IsFSharpUnion || e.IsInterface || e.IsFSharpModule || e.IsClass || e.IsNamespace -> false + // FCS returns inconsistent results for override members; we're skipping these symbols. + | :? FSharpMemberOrFunctionOrValue as f when + f.IsOverrideOrExplicitInterfaceImplementation || + f.IsBaseValue || + f.IsConstructor -> false + // Usage of DU case parameters does not give any meaningful feedback; we never gray them out. + | :? FSharpParameter -> false + | _ -> true + + let getUnusedDeclarationRanges (symbolsUses: FSharpSymbolUse[]) (isScript: bool) = + let definitions = + symbolsUses + |> Array.filter (fun su -> + su.IsFromDefinition && + su.Symbol.DeclarationLocation.IsSome && + (isScript || su.IsPrivateToFile) && + not (su.Symbol.DisplayName.StartsWith "_") && + isPotentiallyUnusedDeclaration su.Symbol) + + let usages = + let usages = + symbolsUses + |> Array.filter (fun su -> not su.IsFromDefinition) + |> Array.choose (fun su -> su.Symbol.DeclarationLocation) + HashSet(usages) + + let unusedRanges = + definitions + |> Array.map (fun defSu -> defSu, usages.Contains defSu.Symbol.DeclarationLocation.Value) + |> Array.groupBy (fun (defSu, _) -> defSu.RangeAlternate) + |> Array.filter (fun (_, defSus) -> defSus |> Array.forall (fun (_, isUsed) -> not isUsed)) + |> Array.map (fun (m, _) -> m) + + //#if DEBUG + //let formatRange (x: FSharp.Compiler.Range.range) = sprintf "(%d, %d) - (%d, %d)" x.StartLine x.StartColumn x.EndLine x.EndColumn + + //symbolsUses + //|> Array.map (fun su -> sprintf "%s, %s, is definition = %b, Symbol (def range = %A)" + // (formatRange su.RangeAlternate) su.Symbol.DisplayName su.IsFromDefinition + // (su.Symbol.DeclarationLocation |> Option.map formatRange)) + //|> Logging.Logging.logInfof "SymbolUses:\n%+A" + // + //definitions + //|> Seq.map (fun su -> sprintf "su range = %s, symbol range = %A, symbol name = %s" + // (formatRange su.RangeAlternate) (su.Symbol.DeclarationLocation |> Option.map formatRange) su.Symbol.DisplayName) + //|> Logging.Logging.logInfof "Definitions:\n%A" + // + //usages + //|> Seq.map formatRange + //|> Seq.toArray + //|> Logging.Logging.logInfof "Used ranges:\n%A" + // + //unusedRanges + //|> Array.map formatRange + //|> Logging.Logging.logInfof "Unused ranges: %A" + //#endif + Array.toList unusedRanges + + let getUnusedDeclarations(checkFileResults: FSharpCheckFileResults, isScriptFile: bool) : Async = + async { + let! allSymbolUsesInFile = checkFileResults.GetAllUsesOfAllSymbolsInFile() + let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile isScriptFile + return unusedRanges } \ No newline at end of file diff --git a/src/fsharp/service/ServiceAnalysis.fsi b/src/fsharp/service/ServiceAnalysis.fsi index 656050acd0f..d2b7b70ee43 100644 --- a/src/fsharp/service/ServiceAnalysis.fsi +++ b/src/fsharp/service/ServiceAnalysis.fsi @@ -12,4 +12,8 @@ module public UnusedOpens = module public SimplifyNames = /// Get all ranges that can be simplified in a file - val getUnnecessaryRanges : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) * sleep: int option -> Async<(range*string) list> \ No newline at end of file + val getUnnecessaryRanges : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) * sleep: int option -> Async<(range*string) list> + +module public UnusedDeclarations = + /// Get all unused declarations in a file + val getUnusedDeclarations : checkFileResults: FSharpCheckFileResults * isScriptFile: bool -> Async \ No newline at end of file diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs index ff7c43839d0..28f1b1f9c4f 100644 --- a/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs +++ b/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs @@ -22,68 +22,6 @@ type internal UnusedDeclarationsAnalyzer [] () = let getProjectInfoManager (document: Document) = document.Project.Solution.Workspace.Services.GetService().FSharpProjectOptionsManager let getChecker (document: Document) = document.Project.Solution.Workspace.Services.GetService().Checker - let isPotentiallyUnusedDeclaration (symbol: FSharpSymbol) : bool = - match symbol with - // Determining that a record, DU or module is used anywhere requires inspecting all their enclosed entities (fields, cases and func / vals) - // for usages, which is too expensive to do. Hence we never gray them out. - | :? FSharpEntity as e when e.IsFSharpRecord || e.IsFSharpUnion || e.IsInterface || e.IsFSharpModule || e.IsClass || e.IsNamespace -> false - // FCS returns inconsistent results for override members; we're skipping these symbols. - | :? FSharpMemberOrFunctionOrValue as f when - f.IsOverrideOrExplicitInterfaceImplementation || - f.IsBaseValue || - f.IsConstructor -> false - // Usage of DU case parameters does not give any meaningful feedback; we never gray them out. - | :? FSharpParameter -> false - | _ -> true - - let getUnusedDeclarationRanges (symbolsUses: FSharpSymbolUse[]) (isScript: bool) = - let definitions = - symbolsUses - |> Array.filter (fun su -> - su.IsFromDefinition && - su.Symbol.DeclarationLocation.IsSome && - (isScript || su.IsPrivateToFile) && - not (su.Symbol.DisplayName.StartsWith "_") && - isPotentiallyUnusedDeclaration su.Symbol) - - let usages = - let usages = - symbolsUses - |> Array.filter (fun su -> not su.IsFromDefinition) - |> Array.choose (fun su -> su.Symbol.DeclarationLocation) - HashSet(usages) - - let unusedRanges = - definitions - |> Array.map (fun defSu -> defSu, usages.Contains defSu.Symbol.DeclarationLocation.Value) - |> Array.groupBy (fun (defSu, _) -> defSu.RangeAlternate) - |> Array.filter (fun (_, defSus) -> defSus |> Array.forall (fun (_, isUsed) -> not isUsed)) - |> Array.map (fun (m, _) -> m) - - //#if DEBUG - //let formatRange (x: FSharp.Compiler.Range.range) = sprintf "(%d, %d) - (%d, %d)" x.StartLine x.StartColumn x.EndLine x.EndColumn - - //symbolsUses - //|> Array.map (fun su -> sprintf "%s, %s, is definition = %b, Symbol (def range = %A)" - // (formatRange su.RangeAlternate) su.Symbol.DisplayName su.IsFromDefinition - // (su.Symbol.DeclarationLocation |> Option.map formatRange)) - //|> Logging.Logging.logInfof "SymbolUses:\n%+A" - // - //definitions - //|> Seq.map (fun su -> sprintf "su range = %s, symbol range = %A, symbol name = %s" - // (formatRange su.RangeAlternate) (su.Symbol.DeclarationLocation |> Option.map formatRange) su.Symbol.DisplayName) - //|> Logging.Logging.logInfof "Definitions:\n%A" - // - //usages - //|> Seq.map formatRange - //|> Seq.toArray - //|> Logging.Logging.logInfof "Used ranges:\n%A" - // - //unusedRanges - //|> Array.map formatRange - //|> Logging.Logging.logInfof "Unused ranges: %A" - //#endif - unusedRanges interface IFSharpUnusedDeclarationsDiagnosticAnalyzer with @@ -98,8 +36,7 @@ type internal UnusedDeclarationsAnalyzer [] () = let! sourceText = document.GetTextAsync() let checker = getChecker document let! _, _, checkResults = checker.ParseAndCheckDocument(document, projectOptions, sourceText = sourceText, userOpName = userOpName) - let! allSymbolUsesInFile = checkResults.GetAllUsesOfAllSymbolsInFile() |> liftAsync - let unusedRanges = getUnusedDeclarationRanges allSymbolUsesInFile (isScriptFile document.FilePath) + let! unusedRanges = UnusedDeclarations.getUnusedDeclarations( checkResults, (isScriptFile document.FilePath)) |> liftAsync return unusedRanges |> Seq.map (fun m -> Diagnostic.Create(descriptor, RoslynHelpers.RangeToLocation(m, sourceText, document.FilePath))) From 8329021ba01b6d1738fe09d37546522073568cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blak?= Date: Mon, 20 Jan 2020 19:13:07 +0100 Subject: [PATCH 4/5] Update src/fsharp/symbols/Symbols.fsi Co-Authored-By: Phillip Carter --- src/fsharp/symbols/Symbols.fsi | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/fsharp/symbols/Symbols.fsi b/src/fsharp/symbols/Symbols.fsi index 33d80108789..ad402418c6c 100644 --- a/src/fsharp/symbols/Symbols.fsi +++ b/src/fsharp/symbols/Symbols.fsi @@ -1101,6 +1101,5 @@ type public FSharpSymbolUse = /// The range of text representing the reference to the symbol member RangeAlternate: range - ///Indicates if the reference is private to the file + /// Indicates if the FSharpSymbolUse is declared as private member IsPrivateToFile : bool - From 7813eef1f44d9686018da2f7556c78d3d9186c8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blak?= Date: Mon, 20 Jan 2020 19:33:54 +0100 Subject: [PATCH 5/5] Apply feedback --- src/fsharp/service/ServiceAnalysis.fs | 37 ++++--------------- src/fsharp/service/ServiceAnalysis.fsi | 10 ++++- .../SimplifyNameDiagnosticAnalyzer.fs | 8 ++-- 3 files changed, 20 insertions(+), 35 deletions(-) diff --git a/src/fsharp/service/ServiceAnalysis.fs b/src/fsharp/service/ServiceAnalysis.fs index 5e559632508..33c5a457759 100644 --- a/src/fsharp/service/ServiceAnalysis.fs +++ b/src/fsharp/service/ServiceAnalysis.fs @@ -229,10 +229,14 @@ module UnusedOpens = } module SimplifyNames = + type SimplifiableRange = { + Range: range + RelativeName: string + } + let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length - /// Get all ranges that can be simplified in a file - let getUnnecessaryRanges (checkFileResults: FSharpCheckFileResults, getSourceLineStr: int -> string, sleep: int option) : Async<(range*string) list> = + let getSimplifiableNames (checkFileResults: FSharpCheckFileResults, getSourceLineStr: int -> string) : Async = async { let result = ResizeArray() let! symbolUses = checkFileResults.GetAllUsesOfAllSymbolsInFile() @@ -270,9 +274,6 @@ module SimplifyNames = } loop (List.rev plid) [] - if sleep.IsNone then - do! Async.Sleep sleep.Value // be less intrusive, give other work priority most of the time - let! necessaryPlid = getNecessaryPlid plid match necessaryPlid with @@ -285,9 +286,8 @@ module SimplifyNames = Range.mkRange r.FileName (Range.mkPos r.StartLine plidStartCol) (Range.mkPos r.EndLine necessaryPlidStartCol) let relativeName = (String.concat "." plid) + "." + name - result.Add(unnecessaryRange, relativeName) + result.Add({Range = unnecessaryRange; RelativeName = relativeName}) - return List.ofSeq result } @@ -330,29 +330,6 @@ module UnusedDeclarations = |> Array.filter (fun (_, defSus) -> defSus |> Array.forall (fun (_, isUsed) -> not isUsed)) |> Array.map (fun (m, _) -> m) - //#if DEBUG - //let formatRange (x: FSharp.Compiler.Range.range) = sprintf "(%d, %d) - (%d, %d)" x.StartLine x.StartColumn x.EndLine x.EndColumn - - //symbolsUses - //|> Array.map (fun su -> sprintf "%s, %s, is definition = %b, Symbol (def range = %A)" - // (formatRange su.RangeAlternate) su.Symbol.DisplayName su.IsFromDefinition - // (su.Symbol.DeclarationLocation |> Option.map formatRange)) - //|> Logging.Logging.logInfof "SymbolUses:\n%+A" - // - //definitions - //|> Seq.map (fun su -> sprintf "su range = %s, symbol range = %A, symbol name = %s" - // (formatRange su.RangeAlternate) (su.Symbol.DeclarationLocation |> Option.map formatRange) su.Symbol.DisplayName) - //|> Logging.Logging.logInfof "Definitions:\n%A" - // - //usages - //|> Seq.map formatRange - //|> Seq.toArray - //|> Logging.Logging.logInfof "Used ranges:\n%A" - // - //unusedRanges - //|> Array.map formatRange - //|> Logging.Logging.logInfof "Unused ranges: %A" - //#endif Array.toList unusedRanges let getUnusedDeclarations(checkFileResults: FSharpCheckFileResults, isScriptFile: bool) : Async = diff --git a/src/fsharp/service/ServiceAnalysis.fsi b/src/fsharp/service/ServiceAnalysis.fsi index d2b7b70ee43..eb277269abb 100644 --- a/src/fsharp/service/ServiceAnalysis.fsi +++ b/src/fsharp/service/ServiceAnalysis.fsi @@ -11,8 +11,16 @@ module public UnusedOpens = val getUnusedOpens : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async module public SimplifyNames = + /// Data for use in finding unnecessarily-qualified names and generating diagnostics to simplify them + type SimplifiableRange = { + /// The range of a name that can be simplified + Range: range + /// The relative name that can be applied to a simplifiable name + RelativeName: string + } + /// Get all ranges that can be simplified in a file - val getUnnecessaryRanges : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) * sleep: int option -> Async<(range*string) list> + val getSimplifiableNames : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async module public UnusedDeclarations = /// Get all unused declarations in a file diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs index ad307dccbf0..09ee9cc2390 100644 --- a/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs +++ b/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs @@ -53,14 +53,14 @@ type internal SimplifyNameDiagnosticAnalyzer [] () = let! sourceText = document.GetTextAsync() let checker = getChecker document let! _, _, checkResults = checker.ParseAndCheckDocument(document, projectOptions, sourceText = sourceText, userOpName=userOpName) - let! result = SimplifyNames.getUnnecessaryRanges(checkResults, (fun lineNumber -> sourceText.Lines.[Line.toZ lineNumber].ToString()), Some (DefaultTuning.SimplifyNameEachItemDelay) ) |> liftAsync + let! result = SimplifyNames.getSimplifiableNames(checkResults, fun lineNumber -> sourceText.Lines.[Line.toZ lineNumber].ToString()) |> liftAsync let mutable diag = ResizeArray() - for (unnecessaryRange,relativeName) in result do + for r in result do diag.Add( Diagnostic.Create( descriptor, - RoslynHelpers.RangeToLocation(unnecessaryRange, sourceText, document.FilePath), - properties = (dict [SimplifyNameDiagnosticAnalyzer.LongIdentPropertyKey, relativeName]).ToImmutableDictionary())) + RoslynHelpers.RangeToLocation(r.Range, sourceText, document.FilePath), + properties = (dict [SimplifyNameDiagnosticAnalyzer.LongIdentPropertyKey, r.RelativeName]).ToImmutableDictionary())) let diagnostics = diag.ToImmutableArray() cache.Remove(key) |> ignore let data = { Hash = textVersionHash; Diagnostics=diagnostics }