Separate fsi extra parameters #1299
Conversation
`FSIExtraParameters` (old) becomes `FSIExtraInteractiveParameters` and `FSIExtraSharedParameters`. Previously, `FSIExtraParameters` was passed directly to the compiler for code analysis. This is to ensure that the static analysis of a .fsx script file is congruent with the code being evaluated in the interpreter. The FSI interpreter has different parameters than the compiler, [documented here](https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/fsharp-interactive-options). When an interactive-only parameter was used, this led to compiler errors. It is intended that FSAC will learn to manage the interactive interpreter in the future, but it does not yet do so. Editor plugins manage the interpreter right now. Some plugins (at least Ionide-vim) use the config option `FSIExtraParameters` to configure that interpreter, and this makes sense. To support the current state and the future state, we split the single `FSIExtraParameters` into `FSIExtraInteractiveParameters` and `FSIExtraSharedParameters` so that the interactive interpreter can be launched with the union of these two and the compiler can receive only the shared parameters that it knows how to deal with. This allows editors to use both config options today to launch the interpreter. Today, FSAC only uses the shared parameters. In the future, when FSAC learns to manage the interactive interpreter, it can union the parameters for the interactive session and continue to pass only the shared parameters to the compiler. When this change occurs, if an editor plugin still manages the `dotnet fsi` interpreter, that will continue to work. Editor plugins can opt in to FSAC-managed interpreters at their will, and with the same configuration options the users already have set up. Additional discussion: - ionide#1210: proximate bug: ionide#1210 - `dotnet fsi` readline bug, making this option very salient for editor plugins: dotnet/fsharp#17215
|
Looks like formatting is off - you can run Thanks for this comprehensive PR! |
| FSIExtraInteractiveParameters: string array option | ||
| FSIExtraSharedParameters: string array option |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should we do similar here in FSAC?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
FSIExtraParametersis set explicitly and the new params are not set -> setFSIExtraSharedParametersto the configured value ofFSIExtraParameters.- 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.
- Reasoning: they are using the old field, so expecting unchanged behavior. The new code consumes only
FSIExtraParametersnot 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)
FSIExtraParametersis set explicitly and the new params are set -> unsupported. We take the new params only and ignoreFSIExtraParameters. Maybe log a warning orraisein 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
raiseand tell them to pick one.
- 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
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:
- Do you like the conditional logic I laid out above?
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good. I think sticking to that requirement actually simplifies the logic a bit:
FSIExtraParametersis sent by client and no new params -> silently use this asFSIExtraSharedParameters, which mimics current behavior. when we want, in the future, we can also start messaging about deprecation for this.FSIExtraParametersis sent by client and either/both new params -> use this asFSIExtraSharedParametersand 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
Bring the old `FSIExtraParameters` option back to not break existing clients. Implement logic to mimic old behavior if the old option is used, warning if the old option is mixed with either of the new. Introduce a new private helper method `AdaptiveFSharpLspServer.oldOrNewExtraParams` for use in RPC calls that parse config options: `Initialize` and `DidChangeConfiguration`. This helper ensures that any clients using the old parameter get the expected behavior from before this PR. Both RPC methods will send a warning to the client if they mix parameters. In the future, we can update warnings to support deprecation in that helper function. The signature of the helper is not the best, and to work with the CEs where the RPC happens, we need to have our message sent at the top level of the CE, so we can use `do!`. This means that we read the Dto, then break our pipeline so we can consume the message, then have a second pipeline that consumes the fixed up Dto. Given that this is in support of a to-be-deprecated option, and that upon deprecation we can remove the helper, it seems okay to have this ugliness.
|
If the CI is green I'll go ahead and merge this and create a new patch-release of FSAC to unblock ionide-vim. Thanks again for the analysis and attention to detail in this Issue and PR @greggyb! |
FIX: #1210; split
FSIExtraParametersin twoFSIExtraParameters(old) becomesFSIExtraInteractiveParametersandFSIExtraSharedParameters.Previously,
FSIExtraParameterswas passed directly to the compiler for code analysis. This is to ensure that the static analysis of a .fsx script file is congruent with the code being evaluated in the interpreter. The FSI interpreter hasdifferent parameters than the compiler, documented here.
When an interactive-only parameter was used, this led to compiler errors.
It is intended that FSAC will learn to manage the interactive interpreter in the future, but it does not yet do so. Editor plugins manage the interpreter right now. Some plugins (at least Ionide-vim) use the config option
FSIExtraParametersto configure that interpreter, and this makes sense.To support the current state and the future state, we split the single
FSIExtraParametersintoFSIExtraInteractiveParametersandFSIExtraSharedParametersso that the interactive interpreter can be launched with the union of these two and the compiler can receive only the shared parameters that it knows how to deal with. This allows editors to use both config options today to launch the interpreter. Today, FSAC only uses the sharedparameters.
In the future, when FSAC learns to manage the interactive interpreter, it can union the parameters for the interactive session and continue to pass only the shared parameters to the compiler. When this change occurs, if an editor plugin still manages the
dotnet fsiinterpreter, that will continue to work. Editor plugins can opt in to FSAC-managed interpreters at their will, and with the same configuration options the users already have set up.Additional discussion:
dotnet fsireadline bug, making this option very salient for editor plugins: dotnet fsi crashes with lines longer than terminal width dotnet/fsharp#17215