Cleaning up the code - your input please

[dsyme]

I've been doing PRs for cleaning up the code, splitting out files, formatting the signature files and so on.

I'd like to hear from everyone what other systematic code improvements we could make. Here's a working list (updated to reflect various suggestions below):

• File formatting:
  • Auto-format the FSharp.Core implementation files
  • Auto-format other implementation files
• Code conventions:
  • Eliminate or reduce the use of stupidly short identifiers like x or e or t, most critically when used for multiple types especially in the same file, e.g. e for exception and e for `expression.
  • Reduce the use of shadowing, e.g. let f x = ... with local function also taking x.
  • Root out all the abbrevthing identifiers that run together abbreviations and so on. I removed a lot via exprty --> exprTy and so on
  • /// comments on everything
  • Write a tool that auto-aligns /// comments on signature and implementation
  • Ensure all the values of a particular type follow a scheme, e.g. all identifiers of static type TType follow regexp scheme ty|[a-z]+Ty .
  • Adopt FSharpLint
  • Add a clear coding conventions file
• Understanding the code structure
  • READMEs in every directory
• Hard-to-understand coding idioms
  • Link to design decisions (RFCs, PR discussions) from code
  • Reduce use of single-case discriminated unions with many fields, matched by using a large tuple
  • Reduce uses of large tuples
    (Cleaning up the code - your input please #13116 (comment))
• Abstraction boundaries
  • Properly document/educate on [the subtleties of working with TType] * [ ] Properly document and/or tighten the pseudo-abstractions TType, MethInfo, PropInfo and so on. See this comment
• File structure:
  • Move BuildGraph.fs/fsi into Facilities
  • Rename CompilerLocationUtils.fs/fsi --> CompilerLocation.fs/fsi
  • Add FxResolver.fsi
  • Add TypedTree.fsi
  • Split CheckPatterns.fs(i) out of CheckExpressions.fs
  • Rename Facilities or rename Utilities
  • Remove Logger.fs/fsi
  • Move FxResolver.fs --> Facilities
  • split Service into CodeAnalysis and EditorServices
• Larger investments
  • Cleanup the auto-completion implementation end to end, see this comment
  • This: Plan: Modernizing F# Analysis #11976

Problems reported:

• Not all test projects seem to be runnable (on @charlesroddie's system).

  Note: this may be documented in TESTGUIDE.md, but it's too unclear what is and isn't runnable, e.g. in debug mode.

Existing coding conventions to document/enforce:

• Standard ordering for open - we order System.* then Internal.* then FSharp.Compiler.* each alphabetically.
• We essentially never use ignore except with identifiers, and almost never use let _ =. We may occasionally use |> ignore

[vzarytovskii]

@baronfel's thoughts from #13118 (comment):

Another useful convention I've seen is README.md documents in the root of each folder as a guide to the purpose/intent of the contents of that folder. Perhaps overkill to have one in every folder, but localized READMEs also lend themselves to nicer browsing experiences in github, since a README in a directory is rendered automatically when viewing the code.

[dsyme]

A comment from Josh
https://twitter.com/JCMRVA/status/1524937070766071834?s=20&t=i49Ds9FmNgEANkH88TSEsg

[JaggerJo]

Completely eliminate or reduce the use of stupidly short identifiers like x or e or t

[dsyme]

Note what I'm particularly concerned about here is cases where the same short identifiers are used for multiple types especially in the same file of code, e.g. e for exception and e for `expression.

[pbiggar]

Assuming the purpose is 1) making the fsharp codebase more approachable to contributors and 2) lower maintenance. Over at @darklang we've had success with:

• auto-lint everything (including files which are not our main languages, such as yaml, bash, python, js), enforce in CI
• auto-format everything, enforce in CI
• A /// comment on every file, function and type
• READMEs in every directory
• ignore should always be typed
  • let! (_ : SomeType) = ... also
• poisoning functions that aren't allowed (ignore with no types, failwith, printf)
• defining a standard ordering for include and opens
• where there are multiple ways of doing something (eg, you can print things differently for the user in context 1, content 2 or context 3, for internal debugging, for tracing), naming those functions explicitly including not having any of them be the "default" name
• remove support for building outside the devcontainer (means we don't need support for different versions of tools, OSes, etc)
• link to design decisions (RFCs, PR discussions) from code
• one single command builds the entire codebase

[vzarytovskii]

• READMEs in every directory

This is something we're definitely want to do, as it was suggested by @baronfel. One technical problem would be is to make sure they're always up to date.

[vzarytovskii]

• remove support for building outside the devcontainer (means we don't need support for different versions of tools, OSes, etc)

I will duplicate my answer here:
I would personally be against constraining building this repo to devcontainer only, since it's rather a limiting factor:

1. Building VS parts in devcontainer will be challenging, to say the least.
2. Making it building inside devcontainer as well as with arcade, and internal CI, including insertions, will be a headache.

Besides, we already have devcontainer where FSharp.sln can be built and debugged. We may just need to add a CI leg making sure it's buildable.

[charlesroddie]

Initial thoughts on the repo

I am new to the repo (started first PR a month ago). It did seem to need serious improvement. F# should result in easy to understand code (with ordered definitions, static types, high s/n ratio, types matching domain). But this is not apparent when looking at the F# repo.

I made some notes then. There may have been improvements since then.

• No xml docs
• Terrible formatting - obviously existing lines of code are treated as sacrosanct and no one is allowed to fix formatting
• Code forms used deprioritize readability. No type annotations (except via signature files which are inconvenient for reading). Partially fixed by IDEs except this takes seconds to show type signatures for a file and doesn't analyze git diffs. Gratuitous currying results in less helpful intellisense and doesn't convey intent. Studious avoidance of classes, favouring huge matches of tuples over using a property.
• Lack of folder structure - not clear which files are in which projects!
• Low perf types everywhere - ref DUs instead of struct DUs/enums, use of linked lists everywhere. Doesn't fall under the category of intelligibility but goes along with lack of hygiene.
• Not all test projects seem to be runnable (on my system)

So @dsyme suggestions are good

/// comments on everything

Most important

Auto-format the implementation files

Yes will be a large improvement. Doesn't matter whether auto-formatted or manually formatted: the formatting is so bad that anything will be a large improvement.

Write a tool that auto-aligns /// comments on signature and implementation

Yes useful, and if it could also-align the type arguments between signature and implementation that would be even better. If both things were done a reader would not need to read the signature file to understand the code.

Completely eliminate or reduce the use of stupidly short identifiers like x or e or t

In local scopes these are fine. Suppose the code says let e = Entropy(...) and then it's used a couple of lines later and falls out of scope. Perfectly good. So this may need to be made more specific to identify where these are reducing comprehensibility.
One case is DU matches where you have type DU = | Case1 of totalEntropy:Entropy * ... and then match du with | Case1(t, ...): this is not good as the shorter name removes information intended to be useful for understanding the case, so by default the matching name should be the given name in the DU definition.

Other things to consider

• An md file making it clear how the projects are organized. I only found an explainer for the compilation process as a whole, but not how this related to projects.
• In our codebase we use number prefexes on projects so they show in IDEs/git in an order compatible with compilation order.
• Allow use of classes/structs in the repo where they are the natural structure and allow PRs that tidy the code in this way.
• [Speculative - only an initial feeling] I have the feeling that low level stuff is being exposed all over the place because there isn't a data structure for what a dotnet type is etc.. And that this makes the compiler code hard and type providers hard. If this feeling persists I will try to make it more precise in future.

[dsyme]

An md file making it clear how the projects are organized. I only found an explainer for the compilation process as a whole, but not how this related to projects.

I think this is done now, see https://github.com/dotnet/fsharp/blob/main/docs/index.md and https://github.com/dotnet/fsharp/blob/main/docs/overview.md. Hopefully people land their pretty quickly starting from top-level README.md I think I'll merge index.md and overview.md

[dsyme]

Allow use of classes/structs in the repo where they are the natural structure and allow PRs that tidy the code in this way.

These are definitely allowed - we use objects quite a lot. I agree there are many places they could be used to good effect to improve code readability and maintainability

[vzarytovskii]

@charlesroddie regarding the following

Not all test projects seem to be runnable (on my system)

Could you please share which ones you're having problems running? And which OS are you using?

[dsyme]

Thank you @charlesroddie and @pbiggar - all good suggestions!

One comment - existing code is not sacrosanct - we can certainly clean-up everything at the moment. Once upon a time there were multiple forks (e.g. FCS a separate fork) and multiple major active features= branches making clean-up painful, but right now things are at a good point where we can sanitise rapidly. The availability of fantomas is a key factor in this.

I'll fold the suggestions into the overall working list above. Folder structure has been dealt with, at least to first approximation.

[dsyme]

ignore should always be typed, also let! (_ : SomeType) = ...

I think we essentially never use ignore except with identifiers, and almost never use let _ = . We may occasionally use |> ignore. I'll check

[dsyme]

defining a standard ordering for include and opens

These are pretty standard now - we order System.* then Internal.* then FSharp.Compiler.* each alphabetically. I'll add it to the coding conventions

[dsyme]

remove support for building outside the devcontainer (means we don't need support for different versions of tools, OSes, etc)

In principle this would be great, but the practicality of this is unclear to me, since we need to test/debug/deliver on multiple OS, and especially Windows (since this repo includes Visual Studio integration and testing).

Splitting the VS Integration out of this repo has been discussed from time to time, though it would massively reduce efficiency of creating IDE features, since people frequently contribute across the FCS/FSharp.Editor divide, and it's one of the most important areas we need to make efficient for the benefit of the whole F# ecosystem. A standard problem of monorepo v. multi-repo.

[vzarytovskii]

I would personally be against constraining building this repo to devcontainer only, since it's rather a limiting factor:

1. Building VS parts in devcontainer will be challenging, to say the least.
2. Making it building inside devcontainer as well as with arcade, and internal CI, including insertions, will be a headache.
   Besides, we already have devcontainer where FSharp.sln can be built and debugged. We may just need to add a CI leg making sure it's buildable.

[auduchinok]

In addition to being seemingly hard itself, it may also make things more difficult for projects maintaining FCS forks (Fable, Fantomas, ReSharper.FSharp, FSAC, and probably others) as they may be building things differently. The current build process allows everyone to customize the needed bits in each fork without single devcontainer restrictions.

[pbiggar]

Completely agree, not appropriate for your situation.

[dsyme]

one single command builds the entire codebase

We have this today - .\build or ./build.sh

[voronoipotato]

oh and don't forget to call with the -noVisualStudio turned on. I missed this and was very confused until I read through the readme :)

[vzarytovskii]

oh and don't forget to call with the -noVisualStudio turned on. I missed this and was very confused until I read through the readme :)

Out of curiosity, why confused? Did you get the error when not providing this flag? Or just to shave off some build time?

[voronoipotato]

I did get an error and I was confused. It was late so perhaps the error was sufficiently descriptive, I don't know. I can say it's not immediately obvious to me that the Visual Studio path should require the build script, so when I ran the build script I assumed that I didn't need Visual Studio.

[dsyme]

Studious avoidance of classes, favouring huge matches of tuples over using a property

I'll record this as

• "overuse of single-case discriminated unions with many fields, matched by using a large tuple", and
• "overuse of large tuples"

[dsyme]

[Speculative - only an initial feeling] I have the feeling that low level stuff is being exposed all over the place because there isn't a data structure for what a dotnet type is etc.. And that this makes the compiler code hard and type providers hard. If this feeling persists I will try to make it more precise in future.

This is a good topic for discussion.

There are various "soft" boundaries in the compiler, meaning data structures which almost act as an abstraction boundary, but where the representation is still exposed in the signature file and can be accessed or matched on.

One pervasive example is TType. The representation of this is available and for most purposes the data structure is not a canonical view - particularly because TType can include includes type inference variables and equations. (After type inference is completed, all inference variables have been either solved or generalised. However we do not do a rewrite pass to eliminate the presence of TType_var with Solution = Some ....) This is a problem because people tend to like to write code that matches directly on TType, which is nearly always wrong.

However even if we eliminated type inference equations, TType also includes type abbreviations type X = int - (we leave those in for purposes of creating more readable output that respects any known abbreviations.) For this reason there are about four ways of viewing TType

• The raw representation including type inference equations
• The view with type inference equations removed stripTyParEqns
• The view with both type inference equations and type abbreviations removed stripTyEqns. This is usually what you want.
• The view with type inference equations , type abbreviations and unit-of-measure erasure removed stripTyEqnsAndMeasureEqns.
• The view with type inference equations , type abbreviations and all erasures stripTyEqnsWrtErasure.

Some active patterns exist to make this simpler but they are not used methodically (and may also have performance implications if they were)

People working with TType must know about these multiple ways of viewing TType and it's very important to code review any operations on TType carefully - I've noticed a lot of code creep in that violates the implicit rules here - partly because those rules aren't adequately documented. I will add some notes about this and address the topic in the online compiler session tomorrow evening.

Another problem is that we have multiple representations of types/metadata

• ILTypes (coming from Abstract IL and .NET metadata, also used in emit)
• Provided types and their associated infos (coming from type providers)
• The unifying TType
• Item that acts as an overall symbol for reporting to the IDE.

The unification of property/method/event data happens in infos.fs and is a messy unification that is sometimes broken - the abstraction boundary of that unification is not always respected, and people writing new code tend to like to match down through the pseudo-abstraction boundary and access each case (e.g. ILMeth, FSMeth) respectively, rather than use the unifying set of members. It's tricky to stop this without sealing the abstraction completely, which is something we should probably aim for.

I'll add these topics to the list above

[nojaf]

Hello,

I've been working on the compiler for some time now, here are some remarks in no particular order:

• Everything is fine if your changes stay within the scope of FSharp.Compiler.Service.sln.
  Once you need to change code outside that in the VisualFSharp.sln, it really is painful to work with.
  I never got things to work outside Visual Studio, and even there it is not that great.
• I picked up a lot of concepts by talking to people. The high-level documentation only goes so far.
  So, having readme files in each folder would be beneficial.
• When to add what kind of unit tests is also a bit unclear. For the syntax tree, I know what to do there, but
  for other topics, it is less clear to me.
• Logic in pars.fsy should be factored out of it.
• It could be beneficial to start a "developer tips and tricks" document. For example, having two instances of the compiler open, one with the main branch and another with your PR, can be enlightening when simultaneously debugging the same code.
  I also frequently use a local dummy unit test that calls checker.ParseAndCheckFileInProject.
  This enables you to walk through a lot of the code. There are most certainly other tricks that other people use, collecting this knowledge would be a win.
• There could also be some more documentation on what service API editors typically use. Every time I see an improvement PR, I always wonder if this is a Visual Studio only thing or if other editors like VSCode or Rider will benefit from this as well.
• Having a sample of SyntaxVisitorBase would also be nice.
• I would also like to know more about parser recovery. Are there areas left where the community can help?

I would also like to express my interest in the tool that syncs the XML documentation comments.
Would that be a repository on fsprojects? Or more a project in here?

[safesparrow]

Hello,

I haven't contributed to the codebase yet, but have spent some time looking into it, trying to focus on improving performance.
Performance of the FCS when used by IDEs (in our case Rider) is the main bottleneck in using F# at my company.

I would find it useful if there were:

• more docs on performance analysis, any benchmarks available, and any decisions made. For example a decision record, with detailed results&benchmarks attached, saying "FSharpLists are only marginally slower (or even faster) in most areas of the codebase than Arrays thus we are happy to use them due to their readability and immutability. Here are the things we tried and the results: (...)."
• benchmark script(s) ready to run by users (there are some example benchmark scripts/notebooks, but I found it hard to use them/they don't work)
• benchmark scripts showing real-world IDE usage (eg. check performed by FCS when Rider asks it to parse the top file in the Fantomas codebase)
• infrastructure to run those scripts on main (and optionally on PRs), aiding any performance-related discussions around code changes and preventing accidental performance degradation.

I would be happy to spend some time working on improving performance and/or making the above reality, but would find it difficult without guidance from the existing contributors.

There is a separate discussion on performance and making it easier to analyse & optimize it, but it's not very chatty, so I thought I'd post here too.

[smoothdeveloper]

Continuing on ramblings around code formatter and style guide, and how it applies to this repository, from #13312 (comment):

there are constructs, that I feel make code reading, and also some operations when editing/writing a better experience:

• taking all field names for a record out of the definition with a single rectangular selection
• checking the types when they are lined up with white space indentation
• continuation character in front of the next line rather than end of previous one

ValFlags relies a bit on similar things (and would benefit from manual prettifying).

Turning all the screws super tight, without considering that the formatter and classic style of choice should mature to allow those variations will make it harder to enable formatter on all code (makes it harder to commit to implementing it on codebases at large too).

It has been a pain point for colleagues, for adopting F#, where they felt it takes more effort to type in code that is formatted and consistent in layout, compared to vanilla Visual Studio, or Visual Studio + Resharper, both for C# and VB.NET code.

Resharper has support for some of those refined formatting styles for C# and VB.net (beyond what VS offers), and I've played with stylish-haskell (https://github.com/haskell/stylish-haskell) which made it super easy to do the kind of alignment I'm referring to.

What is the good place to have discussion pertaining to formatter and style guide symbiotic relationship?

Putting the onus on fantomas repository isn't fair; especially if there is no actionable / endorsed plan for extending it, and having the community hashing on those concerns would be good, to evolve the style guide, even if it isn't tooled.

[edgarfgp]

Hello everyone .
I recently started contributing the the compiler and notice that, There are a lot of //TODO comments about future code improvements, limitations , suggestions etc. So I was wondering if you would consider having ///TODO comments link to an issues so it does not get lost in time and it is tracked somewhere and offers a lille bit more of context about the problem .

[vzarytovskii]

Yeah, it would be helpful to link those to either issues or language suggestions/RFCs. Also need to make sure it's still relevant and not already implemented.

[edgarfgp]

I think only linking Approved language suggestions will make more sense as that implies that someone can write or find the RFC and start implementing the suggestion