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

Minor code clean-ups and performance improvements #906

Merged
merged 2 commits into from
Mar 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Common/Collections.fs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ module internal List =
f x
g ()
iterInterleaved f g (y :: tl)
| x :: [] -> f x
| [ x ] -> f x
| [] -> ()

/// Tests whether a list starts with the elements of another
Expand Down Expand Up @@ -90,7 +90,7 @@ module internal List =
let other, rest = partitionUntil (f >> not) other
yield last, other
yield! loop rest
| [] when other = [] -> ()
| [] when List.isEmpty other -> ()
| _ -> invalidArg "" "Should start with true"
}

Expand Down
40 changes: 25 additions & 15 deletions src/Common/StringParsing.fs
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ open FSharp.Formatting.Markdown

module String =
/// Matches when a string is a whitespace or null
[<return: Struct>]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have any idea under which conditions you would start using this?

  • What is the minimum language version?
  • Always for unit or can this be used for other return types as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Struct constructs came with FSharp.Core 6.0.1 so that's the minimum version.
Works with other return types too.

let (|WhiteSpace|_|) (s) =
if String.IsNullOrWhiteSpace(s) then Some() else None
if String.IsNullOrWhiteSpace(s) then
ValueSome()
else
ValueNone

/// Returns a string trimmed from both start and end
let (|TrimBoth|) (text: string) = text.Trim()
Expand Down Expand Up @@ -117,15 +121,20 @@ module String =

module StringPosition =
/// Matches when a string is a whitespace or null
[<return: Struct>]
let (|WhiteSpace|_|) (s, _n: MarkdownRange) =
if String.IsNullOrWhiteSpace(s) then Some() else None
if String.IsNullOrWhiteSpace(s) then
ValueSome()
else
ValueNone

/// Matches when a string does starts with non-whitespace
[<return: Struct>]
let (|Unindented|_|) (s: string, _n: MarkdownRange) =
if not (String.IsNullOrWhiteSpace(s)) && s.TrimStart() = s then
Some()
ValueSome()
else
None
ValueNone

/// Returns a string trimmed from both start and end
let (|TrimBoth|) (text: string, n: MarkdownRange) =
Expand Down Expand Up @@ -174,11 +183,12 @@ module StringPosition =
StartColumn = n.StartColumn + text.Length - trimmed.Length })

/// Matches when a string starts with any of the specified sub-strings
[<return: Struct>]
let (|StartsWithAny|_|) (starts: string seq) (text: string, _n: MarkdownRange) =
if starts |> Seq.exists (fun s -> text.StartsWith(s, StringComparison.Ordinal)) then
Some()
ValueSome()
else
None
ValueNone

/// Matches when a string starts with the specified sub-string
let (|StartsWith|_|) (start: string) (text: string, n: MarkdownRange) =
Expand Down Expand Up @@ -297,15 +307,16 @@ module StringPosition =

/// Matches when a string consists of some number of
/// complete repetitions of a specified sub-string.
[<return: Struct>]
let (|EqualsRepeated|_|) (repeated, _n: MarkdownRange) =
function
| StartsWithRepeated repeated (_n, (v, _)) when (String.IsNullOrWhiteSpace v) -> Some()
| _ -> None
| StartsWithRepeated repeated (_n, (v, _)) when (String.IsNullOrWhiteSpace v) -> ValueSome()
| _ -> ValueNone

module List =
/// Matches a list if it starts with a sub-list that is delimited
/// using the specified delimiters. Returns a wrapped list and the rest.
let inline (|DelimitedWith|_|) startl endl input =
let inline internal (|DelimitedWith|_|) startl endl input =
Copy link
Collaborator

Choose a reason for hiding this comment

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

So internal because this is used in other files in the project?
Otherwise private would also work?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just want to limit the scope to this solution: As you know inline makes compiler slower. So if you have hundreds of public inline functions in your solution and someone refers your project via Nuget package, they are going to pay the price with their compilation too, because those inline-functions come available for them too, even when they probably are not aware of them and probably will never need them.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my reasoning only, and I might be wrong:
If do code like this: myInt |> and now you ask your F# IDE to auto-complete, then when it knows the type of "myInt" then it can give proper function options: Those who take int in this case.

But in case of inline functions, they use statically resolved type parameters.

So if you have a referenced 10s other nuget packages and all have 10s of inline functions, then... all of those might be potentials for your auto-complete. You probably don't want call those, so it's just noise that could be avoided if they'd be internal functions.

if List.startsWith startl input then
match List.partitionUntilEquals endl (List.skip startl.Length input) with
| Some(pre, post) -> Some(pre, List.skip endl.Length post, startl.Length, endl.Length)
Expand All @@ -314,14 +325,14 @@ module List =
None

/// Matches a list if it starts with a sub-list. Returns the list.
let inline (|StartsWith|_|) startl input =
let inline internal (|StartsWith|_|) startl input =
if List.startsWith startl input then Some input else None

/// Matches a list if it starts with a sub-list that is delimited
/// using the specified delimiter. Returns a wrapped list and the rest.
let inline (|Delimited|_|) str = (|DelimitedWith|_|) str str
let inline internal (|Delimited|_|) str = (|DelimitedWith|_|) str str

let inline (|DelimitedNTimes|_|) str input =
let inline internal (|DelimitedNTimes|_|) str input =
let strs, _items = List.partitionWhile (fun i -> i = str) input

match strs with
Expand Down Expand Up @@ -403,9 +414,8 @@ module Lines =
let (|TrimParagraphLines|) lines =
lines
// first remove all whitespace on the beginning of the line
|> List.map (fun (StringPosition.TrimStart s) -> s)
// Now remove all additional spaces at the end, but keep two spaces if existent
|> List.map (fun (s, n) ->
// then remove all additional spaces at the end, but keep two spaces if existent
|> List.map (fun (StringPosition.TrimStart(s, n)) ->
let endsWithTwoSpaces = s.EndsWith(" ", StringComparison.Ordinal)

let trimmed = s.TrimEnd([| ' ' |]) + if endsWithTwoSpaces then " " else ""
Expand Down
4 changes: 2 additions & 2 deletions src/FSharp.Formatting.ApiDocs/Categorise.fs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ let entities (nsIndex: int, ns: ApiDocNamespace, suppress) =
//
// See https://github.com/fsharp/fsharp-core-docs/issues/57, we may rethink this
|> List.filter (fun e ->
not (e.Symbol.Namespace = Some "Microsoft.FSharp.Data.UnitSystems.SI.UnitSymbols"))
(e.Symbol.Namespace <> Some "Microsoft.FSharp.Data.UnitSystems.SI.UnitSymbols"))
|> List.filter (fun e ->
not (e.Symbol.Namespace = Some "Microsoft.FSharp.Data.UnitSystems.SI.UnitNames"))
(e.Symbol.Namespace <> Some "Microsoft.FSharp.Data.UnitSystems.SI.UnitNames"))
// Don't show 'AnonymousObject' in list-of-namespaces navigation
|> List.filter (fun e ->
not (
Expand Down
21 changes: 11 additions & 10 deletions src/FSharp.Formatting.ApiDocs/GenerateModel.fs
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,16 @@ module internal Utils =
else
None

[<return: Struct>]
let (|MeasureOne|_|) (typ: FSharpType) =
if
typ.HasTypeDefinition
&& typ.TypeDefinition.LogicalName = "1"
&& typ.GenericArguments.Count = 0
then
Some()
ValueSome()
else
None
ValueNone

let tryGetLocation (symbol: FSharpSymbol) =
match symbol.ImplementationLocation with
Expand Down Expand Up @@ -105,7 +106,7 @@ module internal Utils =
member x.TryAttr(attr: string) =
let a = x.Attribute(XName.Get attr)

if a = null then None
if isNull a then None
else if String.IsNullOrEmpty a.Value then None
else Some a.Value

Expand Down Expand Up @@ -1757,7 +1758,7 @@ module internal SymbolReader =
[ let mutable line = ""

while (line <- reader.ReadLine()
line <> null) do
not (isNull line)) do
yield line ]

String.removeSpaces lines
Expand Down Expand Up @@ -1905,14 +1906,14 @@ module internal SymbolReader =
let name = elem.Attribute(XName.Get "name")
let nameAsHtml = HttpUtility.HtmlEncode name.Value

if name <> null then
if not (isNull name) then
html.AppendFormat("<span class=\"fsdocs-param-name\">{0}</span>", nameAsHtml)
|> ignore
| "see"
| "seealso" ->
let cref = elem.Attribute(XName.Get "cref")

if cref <> null then
if not (isNull cref) then
if System.String.IsNullOrEmpty(cref.Value) || cref.Value.Length < 3 then
printfn "ignoring invalid cref specified in: %A" e

Expand Down Expand Up @@ -2018,7 +2019,7 @@ module internal SymbolReader =
let remarks =
let remarkNodes = doc.Elements(XName.Get "remarks") |> Seq.toList

if List.length remarkNodes > 0 then
if not (List.isEmpty remarkNodes) then
let html = new StringBuilder()

for (id, e) in List.indexed remarkNodes do
Expand Down Expand Up @@ -2053,7 +2054,7 @@ module internal SymbolReader =
[ for e in exceptionNodes do
let cref = e.Attribute(XName.Get "cref")

if cref <> null then
if not (isNull cref) then
if String.IsNullOrEmpty(cref.Value) || cref.Value.Length < 3 then
printfn "Warning: Invalid cref specified in: %A" doc

Expand Down Expand Up @@ -2581,7 +2582,7 @@ module internal SymbolReader =
|> Seq.choose (fun p ->
let nameAttr = p.Attribute(XName.Get "name")

if nameAttr = null then
if isNull nameAttr then
None
else
Some(nameAttr.Value, p.Value))
Expand Down Expand Up @@ -2821,7 +2822,7 @@ module internal SymbolReader =
[ for e in doc.Descendants(XName.Get "member") do
let attr = e.Attribute(XName.Get "name")

if attr <> null && not (String.IsNullOrEmpty(attr.Value)) then
if (not (isNull attr)) && not (String.IsNullOrEmpty(attr.Value)) then
yield attr.Value, e ] do
// NOTE: We completely ignore duplicate keys and I don't see
// an easy way to detect where "value" is coming from, because the entries
Expand Down
2 changes: 1 addition & 1 deletion src/FSharp.Formatting.CodeFormat/CodeFormatAgent.fs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ module CodeFormatter =
[| let line = ref ""

while (line := reader.ReadLine()
line.Value <> null) do
not (isNull line.Value)) do
yield line.Value |]
// Get options for a standalone script file (this adds some
// default references and doesn't require full project information)
Expand Down
4 changes: 2 additions & 2 deletions src/FSharp.Formatting.CodeFormat/SourceCode.fs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ type ToolTipSpan =
| HardLineBreak

/// Classifies tokens reported by the FCS
[<RequireQualifiedAccess>]
[<RequireQualifiedAccess; Struct>]
type TokenKind =
| Keyword
| String
Expand Down Expand Up @@ -49,7 +49,7 @@ type TokenKind =


/// Represents a kind of error reported from the F# compiler (warning or error)
[<RequireQualifiedAccess>]
[<RequireQualifiedAccess; Struct>]
type ErrorKind =
| Error
| Warning
Expand Down
3 changes: 1 addition & 2 deletions src/FSharp.Formatting.Common/PynbModel.fs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ type Cell =
| Some(x) -> string<int> x)
(this.outputs |> Array.map string<Output> |> String.concat ",\n")))
(this.source
|> Array.map addLineEnd
|> Array.map escapeAndQuote
|> Array.map (addLineEnd >> escapeAndQuote)
|> String.concat ",\n ")

type Kernelspec =
Expand Down
14 changes: 6 additions & 8 deletions src/FSharp.Formatting.Common/Templating.fs
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,12 @@ module internal SimpleTemplating =
// If there is no template or the template is an empty file, return just document + tooltips (tooltips empty if not HTML)
let lookup = readOnlyDict substitutions

(if lookup.ContainsKey ParamKeys.``fsdocs-content`` then
lookup.[ParamKeys.``fsdocs-content``]
else
"")
+ (if lookup.ContainsKey ParamKeys.``fsdocs-tooltips`` then
"\n\n" + lookup.[ParamKeys.``fsdocs-tooltips``]
else
"")
(match lookup.TryGetValue ParamKeys.``fsdocs-content`` with
| true, lookupContent -> lookupContent
| false, _ -> "")
+ (match lookup.TryGetValue ParamKeys.``fsdocs-tooltips`` with
| true, lookupTips -> "\n\n" + lookupTips
| false, _ -> "")
| Some templateText -> ApplySubstitutionsInText substitutions templateText

let UseFileAsSimpleTemplate (substitutions, templateOpt, outputFile) =
Expand Down
23 changes: 12 additions & 11 deletions src/FSharp.Formatting.Common/YaafFSharpScripting.fs
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,9 @@ module internal CompilerServiceExtensions =
dllFiles
|> List.map (fun file ->
file,
(if referenceDict.ContainsKey file then
Some referenceDict.[file]
else
None))
(match referenceDict.TryGetValue file with
| true, refFile -> Some refFile
| false, _ -> None))

let getProjectReferencesSimple frameworkVersion (dllFiles: string list) =
getProjectReferences frameworkVersion None None dllFiles |> resolve dllFiles
Expand Down Expand Up @@ -464,26 +463,29 @@ module internal ArgParser =
else
None

[<return: Struct>]
Copy link
Member Author

@Thorium Thorium Mar 15, 2024

Choose a reason for hiding this comment

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

@nojaf here is an example where there is a Struct boolean. As boolean is a tiny field, there is no point of wrapping it to inside a separate class, like the original code did. Wrapping just makes the processing of comparisons etc slower.

let (|FsiBoolArg|_|) argName s =
match s with
| StartsWith argName rest ->
if String.IsNullOrWhiteSpace rest then
Some true
ValueSome true
else
match rest with
| "+" -> Some true
| "-" -> Some false
| _ -> None
| _ -> None
| "+" -> ValueSome true
| "-" -> ValueSome false
| _ -> ValueNone
| _ -> ValueNone

open ArgParser

[<Struct>]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The heuristic would be here to add [<Struct>] when the DU only has cases without any fields?

Copy link
Member Author

@Thorium Thorium Mar 15, 2024

Choose a reason for hiding this comment

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

Yes, Enums and similar are struct per default, so if you do this:

type MyE =
| A = 1
| B = 2

it's fine as is. But if you do this:

type MyE =
| A 
| B 

...it's not a struct. So basically it'll generate a class of MyE containing a class of A and class of B and an instance of either.

type internal DebugMode =
| Full
| PdbOnly
| Portable
| NoDebug

[<Struct>]
type internal OptimizationType =
| NoJitOptimize
| NoJitTracking
Expand Down Expand Up @@ -761,7 +763,7 @@ type internal FsiOptions =
| [] -> Seq.empty
| opts ->
opts
|> Seq.map (fun (enable, types) ->
|> Seq.collect (fun (enable, types) ->
seq {
yield sprintf "--optimize%s" (getMinusPlus enable)

Expand All @@ -778,7 +780,6 @@ type internal FsiOptions =
| NoTailCalls -> "notailcalls")
|> String.concat ","
})
|> Seq.concat

yield! getSimpleBoolArg "--quiet" x.Quiet
yield! getSimpleBoolArg "--quotations-debug" x.QuotationsDebug
Expand Down
2 changes: 1 addition & 1 deletion src/FSharp.Formatting.Literate/Contexts.fs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type internal CompilerContext =
}

/// Defines the possible output types from literate script (HTML, Latex, Pynb)
[<RequireQualifiedAccess>]
[<RequireQualifiedAccess; Struct>]
type OutputKind =
/// Requests HTML output
| Html
Expand Down
2 changes: 1 addition & 1 deletion src/FSharp.Formatting.Literate/Evaluator.fs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ open FSharp.Formatting.Internal
/// <namespacedoc>
/// <summary>Functionality to support literate evaluation for F# scripts</summary>
/// </namespacedoc>
[<RequireQualifiedAccessAttribute>]
[<RequireQualifiedAccessAttribute; Struct>]
type FsiEmbedKind =
/// The FSI output
| FsiOutput
Expand Down
6 changes: 4 additions & 2 deletions src/FSharp.Formatting.Literate/ParseScript.fs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,9 @@ type internal ParseScript(parseOptions, ctx: CompilerContext) =
| Command "condition" name when not (String.IsNullOrWhiteSpace name) -> { Condition = Some name }
| _ -> { Condition = None }

let (|EmptyString|_|) (v: string) = if v.Length = 0 then Some() else None
[<return: Struct>]
let (|EmptyString|_|) (v: string) =
if v.Length = 0 then ValueSome() else ValueNone

/// Transform list of code blocks (snippet/comment/command)
/// into a formatted Markdown document, with link definitions
Expand Down Expand Up @@ -386,7 +388,7 @@ type internal ParseScript(parseOptions, ctx: CompilerContext) =

let parsedBlocks =
[ for Snippet(name, lines) in sourceSnippets do
if name <> null then
if not (isNull name) then
yield BlockComment("## " + name)

yield! parseScriptFile (lines) ]
Expand Down
Loading