diff --git a/src/fsharp/service/ServiceAnalysis.fs b/src/fsharp/service/ServiceAnalysis.fs index e568767a1e..33c5a45775 100644 --- a/src/fsharp/service/ServiceAnalysis.fs +++ b/src/fsharp/service/ServiceAnalysis.fs @@ -226,4 +226,115 @@ module UnusedOpens = let symbolUses = splitSymbolUses symbolUses let openStatements = getOpenStatements checkFileResults.OpenDeclarations return! filterOpenStatements symbolUses openStatements + } + +module SimplifyNames = + type SimplifiableRange = { + Range: range + RelativeName: string + } + + let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length + + let getSimplifiableNames (checkFileResults: FSharpCheckFileResults, getSourceLineStr: int -> string) : Async = + 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) [] + + 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({Range = unnecessaryRange; RelativeName = relativeName}) + + 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) + + 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 8d97644389..eb277269ab 100644 --- a/src/fsharp/service/ServiceAnalysis.fsi +++ b/src/fsharp/service/ServiceAnalysis.fsi @@ -8,4 +8,20 @@ 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 = + /// 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 getSimplifiableNames : checkFileResults: FSharpCheckFileResults * getSourceLineStr: (int -> string) -> Async + +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/src/fsharp/symbols/Symbols.fs b/src/fsharp/symbols/Symbols.fs index 295e42b02c..f4b2f6babe 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 6893097ed9..ad402418c6 100644 --- a/src/fsharp/symbols/Symbols.fsi +++ b/src/fsharp/symbols/Symbols.fsi @@ -1101,3 +1101,5 @@ type public FSharpSymbolUse = /// The range of text representing the reference to the symbol member RangeAlternate: range + /// Indicates if the FSharpSymbolUse is declared as private + member IsPrivateToFile : bool diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs index f90717522a..09ee9cc239 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.getSimplifiableNames(checkResults, fun lineNumber -> sourceText.Lines.[Line.toZ lineNumber].ToString()) |> liftAsync + let mutable diag = ResizeArray() + for r in result do + diag.Add( + Diagnostic.Create( + descriptor, + 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 } let cacheItem = CacheItem(key, data) diff --git a/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs b/vsintegration/src/FSharp.Editor/Diagnostics/UnusedDeclarationsAnalyzer.fs index ff7c43839d..28f1b1f9c4 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))) diff --git a/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs b/vsintegration/src/FSharp.Editor/LanguageService/Symbols.fs index a3d9b78452..dd7359d08f 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"