Pass an FSharpProjectOptions down into the lint machinery when available#845
Open
Numpsy wants to merge 1 commit intofsprojects:masterfrom
Open
Pass an FSharpProjectOptions down into the lint machinery when available#845Numpsy wants to merge 1 commit intofsprojects:masterfrom
Numpsy wants to merge 1 commit intofsprojects:masterfrom
Conversation
Numpsy
commented
Feb 15, 2026
| GlobalConfig: Rules.GlobalRuleConfig | ||
| TypeCheckResults: FSharpCheckFileResults option | ||
| ProjectCheckResults: FSharpCheckProjectResults option | ||
| ProjectOptions: Lazy<FSharpProjectOptions option> |
Contributor
Author
There was a problem hiding this comment.
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)
Collaborator
There was a problem hiding this comment.
Please these explanations have to go in the commit msg.
Contributor
Author
There was a problem hiding this comment.
commit message updated
3 tasks
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
force-pushed
the
pass_down_options
branch
from
46314f3 to
fdb83e4
Compare
Numpsy
changed the title
Collaborator
|
@webwarrior-ws please review |
Contributor
|
LGTM |
Collaborator
|
@Numpsy why draft? is it because of the red CI? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Contributor
Author
|
I'm not sure what's going on with the CI, it looks like the last nightly build failed with the same error? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
ProjectFileNameproperty 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