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.

Sign up for GitHub

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

Separate fsi extra parameters #1299

Merged
merged 5 commits into from
May 23, 2024
Merged

Separate fsi extra parameters #1299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e35615c
FIX: #1210; split `FSIExtraParameters` in two
greggyb May 23, 2024
f9cd5a6
fix typo in README: update `FSIExtraParameters` to `FSIExtraSharedPar…
greggyb May 23, 2024
2e1ee31
fix casing on FSI parameter options
greggyb May 23, 2024
841b449
fix formatting for build checks
greggyb May 23, 2024
27343a1
Preserve old `FSIExtraParameters`; pick reasonable options
greggyb May 23, 2024
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
8 changes: 6 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,14 @@ Options that should be send as `initializationOptions` as part of `initialize` r
* `FSharp.ResolveNamespaces` - enables resolve namespace quick fix (add `open` if symbol is from not yet opened module/namespace), recommended default value: `true`
* `FSharp.EnableReferenceCodeLens` - enables reference count code lenses, recommended default value: `true` if `--background-service-enabled` is used by default, `false` otherwise
* `FSharp.dotNetRoot` - sets the root path for finding dotnet SDK references. Primarily used for FSX Scripts. Default value: operating-system dependent. On windows, `C:\Program Files\dotnet`; on Unix, `/usr/local/share/dotnet`
* `FSharp.fsiExtraParameters` - an array of additional runtime arguments that are passed to FSI. These are used when typechecking scripts to ensure that typechecking has the same context as your FSI instances. An example would be to set the following parameters to enable Preview features (like opening static classes) for typechecking.
* Extra parameters for FSI: use only `FSharp.FSIExtraParameters` on its own *or* a combination of `FSharp.FSIExtraInteractiveParameters` and `FSharp.FSIExtraSharedParameters`. The former is expected to be deprecated in favor of the second two. See #1210 for more detail. FSAC will send a warning if you mix usage improperly.
* `FSharp.FSIExtraParameters` - an array of additional runtime arguments that are passed to FSI. These are used when typechecking scripts to ensure that typechecking has the same context as your FSI instances. An example would be to set the following parameters to enable Preview features (like opening static classes) for typechecking.
* `FSharp.FSIExtraInteractiveParameters` - currently unused by FSAC, but available to editor plugins for interactive `dotnet fsi` parameters that are not shared by the compiler. Future intentions are to manage the interpreter from FSAC, at which point FSAC will utilize this parameter. [Check this reference for parameters that are interactive-only or shared with the compiler](https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/fsharp-interactive-options).
* `FSharp.FSIExtraSharedParameters` - an array of additional runtime arguments that are passed to FSI; specifically parameters that are shared with the compiler. These are used when typechecking scripts to ensure that typechecking has the same context as your FSI instances. An example would be to set the following parameters to enable Preview features (like opening static classes) for typechecking. [Check this reference for parameters that are interactive-only or shared with the compiler](https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/fsharp-interactive-options).

```json
"FSharp.fsiExtraParameters": ["--langversion:preview"]
"FSharp.fsiExtraSharedParameters": ["--langversion:preview"]
"FSharp.fsiExtraInteractiveParameters": ["--readline-"]
```

#### Debug Settings
Expand Down
16 changes: 16 additions & 0 deletions src/FsAutoComplete/LspHelpers.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,8 @@ type FSharpConfigDto =
UseSdkScripts: bool option
DotNetRoot: string option
FSIExtraParameters: string array option
FSIExtraInteractiveParameters: string array option
FSIExtraSharedParameters: string array option
Comment on lines +677 to +678
Copy link
Contributor

Choose a reason for hiding this comment

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

When we take this update in Ionide we'll need to use the deprecation features of the config system there to message to users that this setting has changed, but that's a reasonable thing to change IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we do similar here in FSAC?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a search on github for usages of this and I did see a few notable ones in the Farmer project's docs and samples. This is a fairly large F# OSS tool and I'd prefer not to outright break them, so I think we should introduce the older field for compatibility. In the FSharpChecker's setFsiArgs we should check both this older field and the newer, shared arguments field. Then in the Ionide layer we can communicate the deprecation of the old setting, and in a release or two (maybe in sync with the .NET SDK 8.0.400 release in August?) we can deprecate the older fields entirely, removing them from FSAC and any prior editor handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the old field back again. I will leave the changes to where the options are consumed, so that all consuming code is referencing the new FSIExtraSharedParameters. And then I think the logic below should set us up well for success now and in the future:

  • FSIExtraParameters is set explicitly and the new params are not set -> set FSIExtraSharedParameters to the configured value of FSIExtraParameters.
    • Reasoning: they are using the old field, so expecting unchanged behavior. The new code consumes only FSIExtraSharedParameters. By setting the new option in this way, everything the client has passed to us is used, mimicking the existing behavior.
  • FSIExtraParameters not set explicitly -> use whatever is in the new parameters.
    • Reasoning: if they are not setting the old parameter, then it doesn't matter. We can unconditionally take the new ones (or defaults if they are blank)
  • FSIExtraParameters is set explicitly and the new params are set -> unsupported. We take the new params only and ignore FSIExtraParameters. Maybe log a warning or raise in this case?
    • Reasoning: if someone is using the new parameters, they should be opting in to new behavior. If they're using all three, there is no reasonable thing to do with the old parameter. Given that this depends on them opting in to new parameters, I'd argue to raise and tell them to pick one.

Following this logic, when we eventually remove FSIExtraParameters, all we need to do is remove the bit that conditionally copies its value to FSIExtraSharedParameters.

Thoughts? Specifically:

  1. Do you like the conditional logic I laid out above?
  2. What to do if the client is sending config options for the combination of both the old and the new parameters? I like raiseing on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally like this with the exception that we shouldn't raise because we only consume these changes in two RPC calls:

  • initialize
  • workspace/didChangeConfiguration

raise in either of these contexts will either leave the LSP in an unstable state or prevent the configuration change from 'taking'. Instead, we should send a message back to the client via the lspClient.WindowShowMessage API, like we do for other warnings/notifications. You can see an example of this here. This should communicate the situation to users while not impacting the stability of the LSP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I think sticking to that requirement actually simplifies the logic a bit:

  • FSIExtraParameters is sent by client and no new params -> silently use this as FSIExtraSharedParameters, which mimics current behavior. when we want, in the future, we can also start messaging about deprecation for this.
  • FSIExtraParameters is sent by client and either/both new params -> use this as FSIExtraSharedParameters and send a message to that effect, warning not to mix old and new
  • no FSIExtraParameters -> silent, use whatever is passed (or not) by client for new parameters

FSICompilerToolLocations: string array option
TooltipMode: string option
GenerateBinlog: bool option
Expand Down Expand Up @@ -815,7 +817,13 @@ type FSharpConfig =
LineLens: LineLensConfig
UseSdkScripts: bool
DotNetRoot: string
// old, combines both shared and interactive. To be deprecated. Either use
// only this one, or some combination of the new ones.
FSIExtraParameters: string array
// for parameters only used in interactive FSI sessions; currently unused
FSIExtraInteractiveParameters: string array
// for parameters used both in the compiler and interactive FSI
FSIExtraSharedParameters: string array
FSICompilerToolLocations: string array
TooltipMode: string
GenerateBinlog: bool
Expand Down Expand Up @@ -865,6 +873,8 @@ type FSharpConfig =
UseSdkScripts = true
DotNetRoot = Environment.dotnetSDKRoot.Value.FullName
FSIExtraParameters = [||]
FSIExtraInteractiveParameters = [||]
FSIExtraSharedParameters = [||]
FSICompilerToolLocations = [||]
TooltipMode = "full"
GenerateBinlog = false
Expand Down Expand Up @@ -921,6 +931,9 @@ type FSharpConfig =
|> Option.bind (fun s -> if String.IsNullOrEmpty s then None else Some s)
|> Option.defaultValue Environment.dotnetSDKRoot.Value.FullName
FSIExtraParameters = defaultArg dto.FSIExtraParameters FSharpConfig.Default.FSIExtraParameters
FSIExtraInteractiveParameters =
defaultArg dto.FSIExtraInteractiveParameters FSharpConfig.Default.FSIExtraInteractiveParameters
FSIExtraSharedParameters = defaultArg dto.FSIExtraSharedParameters FSharpConfig.Default.FSIExtraSharedParameters
FSICompilerToolLocations = defaultArg dto.FSICompilerToolLocations FSharpConfig.Default.FSICompilerToolLocations
TooltipMode = defaultArg dto.TooltipMode "full"
GenerateBinlog = defaultArg dto.GenerateBinlog false
Expand Down Expand Up @@ -1030,6 +1043,9 @@ type FSharpConfig =
|> Option.bind (fun s -> if String.IsNullOrEmpty s then None else Some s)
|> Option.defaultValue FSharpConfig.Default.DotNetRoot
FSIExtraParameters = defaultArg dto.FSIExtraParameters FSharpConfig.Default.FSIExtraParameters
FSIExtraInteractiveParameters =
defaultArg dto.FSIExtraInteractiveParameters FSharpConfig.Default.FSIExtraInteractiveParameters
FSIExtraSharedParameters = defaultArg dto.FSIExtraSharedParameters FSharpConfig.Default.FSIExtraSharedParameters
FSICompilerToolLocations = defaultArg dto.FSICompilerToolLocations FSharpConfig.Default.FSICompilerToolLocations
TooltipMode = defaultArg dto.TooltipMode x.TooltipMode
GenerateBinlog = defaultArg dto.GenerateBinlog x.GenerateBinlog
Expand Down
4 changes: 4 additions & 0 deletions src/FsAutoComplete/LspHelpers.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ type FSharpConfigDto =
UseSdkScripts: bool option
DotNetRoot: string option
FSIExtraParameters: string array option
FSIExtraInteractiveParameters: string array option
FSIExtraSharedParameters: string array option
FSICompilerToolLocations: string array option
TooltipMode: string option
GenerateBinlog: bool option
Expand Down Expand Up @@ -403,6 +405,8 @@ type FSharpConfig =
UseSdkScripts: bool
DotNetRoot: string
FSIExtraParameters: string array
FSIExtraInteractiveParameters: string array
FSIExtraSharedParameters: string array
FSICompilerToolLocations: string array
TooltipMode: string
GenerateBinlog: bool
Expand Down
59 changes: 56 additions & 3 deletions src/FsAutoComplete/LspServers/AdaptiveFSharpLspServer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,41 @@ type AdaptiveFSharpLspServer

Helpers.ignoreNotification

///<summary>
/// Transform a config DTO to use (old FSIExtraParameters) xor (new FSIExtraInteractiveParameters and FSIExtraSharedParameters).
///</summary>
///<remarks>
/// We expect to eventually deprecate FSIExtraParameters in favor of the two
/// new, specialized versions. For now, mimic old behavior either silently
/// or loudly, depending on the client's combination of config options. This
/// method and the consumption of it can be removed after deprecation.
///</remarks>
static member private oldOrNewExtraParams(dto: FSharpConfigDto) =
match dto.FSIExtraParameters, dto.FSIExtraInteractiveParameters, dto.FSIExtraSharedParameters with
// old-style, silent success; start warning when we plan to
// deprecate
| Some p, None, None ->
let c =
{ dto with
FSIExtraSharedParameters = Some p }

None, c
// mix old and new, warn and mimic old behavior
| Some p, Some _, _
| Some p, _, Some _ ->
let m =
{ Type = MessageType.Warning
Message =
"Do not mix usage of FSIExtraParameters and (FSIExtraInteractiveParameters or FSIExtraSharedParameters)." }

let c =
{ dto with
FSIExtraSharedParameters = Some p }

Some m, c
// no old parameter, proceed happily
| None, _, _ -> None, dto

interface IFSharpLspServer with
override x.Shutdown() = (x :> System.IDisposable).Dispose() |> async.Return

Expand All @@ -295,10 +330,19 @@ type AdaptiveFSharpLspServer
try
logger.info (Log.setMessage "Initialize Request {p}" >> Log.addContextDestructured "p" p)

let c =
let configMessage =
p.InitializationOptions
|> Option.bind (fun options -> if options.HasValues then Some options else None)
|> Option.map Server.deserialize<FSharpConfigDto>
|> Option.map AdaptiveFSharpLspServer.oldOrNewExtraParams

match Option.bind fst configMessage with
| Some message -> do! lspClient.WindowShowMessage message
| None -> ()

let c =
configMessage
|> Option.map snd
|> Option.map FSharpConfig.FromDto
|> Option.defaultValue FSharpConfig.Default

Expand Down Expand Up @@ -1684,9 +1728,18 @@ type AdaptiveFSharpLspServer
>> Log.addContextDestructured "params" p
)

let dto = p.Settings |> Server.deserialize<FSharpConfigRequest>
let dto =
p.Settings
|> Server.deserialize<FSharpConfigRequest>
|> (fun f -> f.FSharp)
|> Option.map AdaptiveFSharpLspServer.oldOrNewExtraParams

match Option.bind fst dto with
| Some message -> do! lspClient.WindowShowMessage message
| None -> ()

dto.FSharp
dto
|> Option.map snd
|> Option.iter (fun fsharpConfig ->
let c = state.Config
let c = c.AddDto fsharpConfig
Expand Down
2 changes: 1 addition & 1 deletion src/FsAutoComplete/LspServers/AdaptiveServerState.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ type AdaptiveState
|> Observable.subscribe (fun (config, checker, rootPath) ->
toggleTraceNotification config.Notifications.Trace config.Notifications.TraceNamespaces

setFSIArgs checker config.FSICompilerToolLocations config.FSIExtraParameters
setFSIArgs checker config.FSICompilerToolLocations config.FSIExtraSharedParameters

loadAnalyzers config rootPath

Expand Down
2 changes: 1 addition & 1 deletion test/FsAutoComplete.Tests.Lsp/CodeFixTests/Tests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2333,7 +2333,7 @@ let private removeUnnecessaryReturnOrYieldTests state =
let private removeUnusedBindingTests state =
let config =
{ defaultConfigDto with
FSIExtraParameters = Some [| "--warnon:1182" |] }
FSIExtraSharedParameters = Some [| "--warnon:1182" |] }

serverTestList (nameof RemoveUnusedBinding) state config None (fun server ->
[ let selectRemoveUnusedBinding = CodeFix.withTitle RemoveUnusedBinding.titleBinding
Expand Down
6 changes: 3 additions & 3 deletions test/FsAutoComplete.Tests.Lsp/CodeFixTests/ToInterpolatedStringTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module private FsAutoComplete.Tests.CodeFixTests.ToInterpolatedStringTests
module private FsAutoComplete.Tests.CodeFixTests.ToInterpolatedStringTests

open Expecto
open Helpers
Expand All @@ -9,7 +9,7 @@ open FsAutoComplete.FCSPatches

let langVersion60Config =
{ defaultConfigDto with
FSIExtraParameters = Some [| "--langversion:6.0" |] }
FSIExtraSharedParameters = Some [| "--langversion:6.0" |] }

let tests state =
serverTestList (nameof ToInterpolatedString) state langVersion60Config None (fun server ->
Expand Down Expand Up @@ -356,7 +356,7 @@ let tests state =

let langVersion47Config =
{ defaultConfigDto with
FSIExtraParameters = Some [| "--langversion:4.7" |] }
FSIExtraSharedParameters = Some [| "--langversion:4.7" |] }

let unavailableTests state =
serverTestList $"unavailable {(nameof ToInterpolatedString)}" state langVersion47Config None (fun server ->
Expand Down
2 changes: 2 additions & 0 deletions test/FsAutoComplete.Tests.Lsp/Helpers.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,8 @@ let defaultConfigDto: FSharpConfigDto =
UseSdkScripts = Some true
DotNetRoot = None
FSIExtraParameters = None
FSIExtraInteractiveParameters = None
FSIExtraSharedParameters = None
FSICompilerToolLocations = None
TooltipMode = None
GenerateBinlog = Some true
Expand Down
8 changes: 4 additions & 4 deletions test/FsAutoComplete.Tests.Lsp/ScriptTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ let scriptPreviewTests state =
serverInitialize
path
{ defaultConfigDto with
FSIExtraParameters = Some [| "--langversion:preview" |] }
FSIExtraSharedParameters = Some [| "--langversion:preview" |] }
state

do! waitForWorkspaceFinishedParsing events
Expand Down Expand Up @@ -81,7 +81,7 @@ let scriptEvictionTests state =
{ FSharp =
Some
{ defaultConfigDto with
FSIExtraParameters = Some [| "--nowarn:760" |] } }
FSIExtraSharedParameters = Some [| "--nowarn:760" |] } }

{ Settings = Server.serialize config }

Expand Down Expand Up @@ -115,7 +115,7 @@ let dependencyManagerTests state =

let dependencyManagerEnabledConfig =
{ defaultConfigDto with
FSIExtraParameters = Some [| "--langversion:preview" |] }
FSIExtraSharedParameters = Some [| "--langversion:preview" |] }

let! (server, events) = serverInitialize workingDir dependencyManagerEnabledConfig state
do! waitForWorkspaceFinishedParsing events
Expand Down Expand Up @@ -165,7 +165,7 @@ let scriptProjectOptionsCacheTests state =

let previewEnabledConfig =
{ defaultConfigDto with
FSIExtraParameters = Some [| "--langversion:preview" |] }
FSIExtraSharedParameters = Some [| "--langversion:preview" |] }

let! (server, events) = serverInitialize workingDir previewEnabledConfig state
let options = ResizeArray()
Expand Down
Loading