Skip to content

Pass an FSharpProjectOptions down into the lint machinery when available#845

Open
Numpsy wants to merge 1 commit intofsprojects:masterfrom
Numpsy:pass_down_options
Open

Pass an FSharpProjectOptions down into the lint machinery when available#845
Numpsy wants to merge 1 commit intofsprojects:masterfrom
Numpsy:pass_down_options

Conversation

@Numpsy
Copy link
Copy Markdown
Contributor

@Numpsy Numpsy commented Feb 15, 2026

Rather than reading it from FSharpCheckProjectResults on every use, which appears to have a substantional performance cost.

refs #770 (specifically #770 (comment))

This is an attempt at making the FSharpProjectOptions directly available through the lint options and then constructing it once rather than on every call which seems to have a substantional performance impact.

This is more like how its done in the F# Analyzers SDK, where the project check results and the project options are both passed down from the top.

I'm also reminded when looking that they also have a ProjectFileName property on the top level options so that consumers don't need to worry about where it came from. That could potentially be done here given that all consumers as it stands only want the file name, though having the whole options available is more extensible for the future.

This change does seem to have a positive performance impact with the current code using FCS 43.9 as well as removing the large regression when using 43.10 -

Build of just this change - https://github.com/Numpsy/FSharpLint/actions/runs/22036364219 (this takes a couple of minutes off the runtime of the self-lint step in the CI runs as compared to the current master)
Build using this change and FCS 43.10.103 - https://github.com/Numpsy/FSharpLint/actions/runs/21925277621

GlobalConfig: Rules.GlobalRuleConfig
TypeCheckResults: FSharpCheckFileResults option
ProjectCheckResults: FSharpCheckProjectResults option
ProjectOptions: Lazy<FSharpProjectOptions option>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is Lazy beacuse if it's expensive to calculate and only used by a few rules then the work can be avoided in cases where those rules aren;t being run)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please these explanations have to go in the commit msg.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

commit message updated

@Numpsy Numpsy changed the title [RFC] Pass an FSharpProjectOptions down into the lint machinery when available Pass an FSharpProjectOptions down into the lint machinery when available Feb 26, 2026
@knocte
Copy link
Copy Markdown
Collaborator

knocte commented Feb 26, 2026

@webwarrior-ws please review

@webwarrior-ws
Copy link
Copy Markdown
Contributor

LGTM

@knocte
Copy link
Copy Markdown
Collaborator

knocte commented Feb 26, 2026

@Numpsy why draft? is it because of the red CI?

@Numpsy Numpsy marked this pull request as ready for review February 26, 2026 11:46
greptile-apps[bot]

This comment was marked as spam.

@Numpsy
Copy link
Copy Markdown
Contributor Author

Numpsy commented Feb 26, 2026

I'm not sure what's going on with the CI, it looks like the last nightly build failed with the same error?

@Numpsy
Copy link
Copy Markdown
Contributor Author

Numpsy commented Apr 3, 2026

I'm not sure what's going on with the CI, it looks like the last nightly build failed with the same error?

The nightly builds seem to have started working again now, but I don't seem to have the ability to rerun the failed build here without pushing more changes

@michaelglass
Copy link
Copy Markdown

As we briefly discussed in #846, this PR is great, makes things way faster, and I'd love to see it merged!

A lot of AI stuff so if you're fearful of slop feel free to ignore this or at least take it with a grain of salt, although this comment at least was written with my ten fleshy fingers.

The Pathological Case: Projects with lots of local dependent packages, e.g. FsHotWatch are particularly slow, and this PR makes them much faster.

I (claude actually) built a FSharpLint -> Analyzer shim to run FSharp linters via Ionide.Analyzer.

Then I benchmarked it vs the fsharplint CLI.

Small projects are faster with FsharpLint CLI but pathological cases with lots of local dependencies are way faster running the same FSharpLinters via my shim when launched through the Ionide.Analyzer CLI.

I tried to figure out what was different. One obvious difference was sharing a FSharpChecker & MSBuild per-solution instead of instantiating one for each sub project.

So I asked claude to implement those changes (on top of this branch) and benchmark them...

Branch Change Time vs 0.26.10 vs this branch
Published dotnet-fsharplint 0.26.10 515.1s ± 120.8s 0% +1118%
this branch (baseline) 42.3s ± 6.2s −91.8% 0%
perf/share-msbuild-workspaceloader One WorkspaceLoader + batched LoadProjects 39.9s −92.3% −5.7%
perf/share-fsharpchecker One FSharpChecker via OptionalLintParameters.Checker 35.1s −93.2% −17.0%
perf/share-solution-state Both, singleton-known-set mapping 19.9s −96.1% −52.9%

@Numpsy
Copy link
Copy Markdown
Contributor Author

Numpsy commented Apr 21, 2026

There may or may not be other perf changes if we get #850 done as Microsoft have been trying to speed it up, but with the odd regression as well. I'll try updating the branch to make the CI run again now that the nightly build seems to be fixed

Rather than reading it from FSharpCheckProjectResults on every use,
which appears to have a substantional performance cost.

The project options is stored as a Lazy because it's expensive to
calculate and only used by a few rules, so the work can be avoided
altogether if those rules aren't being run.

refs fsprojects#770.
@Numpsy Numpsy force-pushed the pass_down_options branch from fdb83e4 to c7ed077 Compare April 21, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants