-
Notifications
You must be signed in to change notification settings - Fork 796
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
Remove FSharpProjectOptions from Transparent Compiler check results #18205
Conversation
❗ Release notes required
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change only affects internal APIs, right?
No. With this change, if you use Transparent Compiler to get But if you use the default I think this is ok, since TC is marked as experimental. But eventually we should probably change the public API. Not sure what the point of options being accessible there is anyway. |
Another thing is that currently it might already be possible to get an exception requesting fsharp/src/Compiler/Service/FSharpCheckerResults.fs Lines 3453 to 3456 in 73b8cbb
|
I added a better message in the exception in case some one does run into it. I think we can merge it. It should provide a small perf improvement for Transparent Compiler users since we don't have to construct the options at all. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Description
Proper fix for #16979
FSharpProjectOptions
is part of the public API, however it might be ok to not have it if you use the new experimental API which relies on snapshots instead. We could also addFSharpProjectSnapshot
to theProjectContext
. But the user has to have it in order to request the result, so maybe we can just leave it up to them to hold it if they need it later...Checklist
Test cases addedPerformance benchmarks added in case of performance changes