-
Notifications
You must be signed in to change notification settings - Fork 154
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
fsiExtraParameters break error hints in editor while editing .fsx files #1210
Comments
I'm seeing the same issue with Ionide-vim with the same option ( It ends up showing a repeated diagnostic error at the top of the .fsx file: |
Thanks for the pokes on this folks - is it known which options are FSI-specific? If that set is stable (ideally it would be some kind of property that the FCS APIs expose?) then I agree that it makes the most sense to have FSAC strip out such arguments before they become an error. Separately, I think we should be able to 'correct' any diagnostics with negative ranges just to point at the start of the file (meaning position Would either of you be open to submitting one or both of these changes? |
I'm currently looking through the code and trying to figure out where a property that is named as if it is scoped to only the interpreter ( It appears that this is happening in four methods on the class You'll see that each of these appends the fsi arguments into an array which ends up as part of I find myself wondering if we should simply remove the references to these FSI parameters from each of the four members. At least in Ionide-vim, there should be no impact, because the plugin invokes I have already burned more time than I should have in digging into this. Some questions:
ninja edit : Some grepping around the code base makes it appear to me that all appearances of the string 'fsi' (case insensitive) refer to .fsi files and that none of them have to do with executing an interpreter process. type FSharpCompilerServiceChecker(hasAnalyzers, typecheckCacheSize, parallelReferenceResolution, useTransparentCompiler)
=
let checker =
FSharpChecker.Create(
projectCacheSize = 200,
keepAssemblyContents = hasAnalyzers,
keepAllBackgroundResolutions = true,
suggestNamesForErrors = true,
keepAllBackgroundSymbolUses = true,
enableBackgroundItemKeyStoreAndSemanticClassification = true,
enablePartialTypeChecking = not hasAnalyzers,
parallelReferenceResolution = parallelReferenceResolution,
captureIdentifiersWhenParsing = true,
useSyntaxTreeCache = true,
useTransparentCompiler = useTransparentCompiler
)
...
member private __.GetNetFxScriptSnapshot(file: string<LocalPath>, source) =
async {
optsLogger.info (
Log.setMessage "Getting NetFX options for script file {file}"
>> Log.addContextDestructured "file" file
)
let allFlags = Array.append [| "--targetprofile:mscorlib" |] fsiAdditionalArguments
let! (opts, errors) =
checker.GetProjectSnapshotFromScript(
UMX.untag file,
source,
assumeDotNetFramework = true,
useFsiAuxLib = true,
otherFlags = allFlags,
userOpName = "getNetFrameworkScriptOptions"
)
...
member private __.GetNetCoreScriptSnapshot(file: string<LocalPath>, source) =
async {
optsLogger.info (
Log.setMessage "Getting NetCore options for script file {file}"
>> Log.addContextDestructured "file" file
)
let allFlags =
Array.append [| "--targetprofile:netstandard" |] fsiAdditionalArguments
let! (snapshot, errors) =
checker.GetProjectSnapshotFromScript(
UMX.untag file,
source,
assumeDotNetFramework = false,
useSdkRefs = true,
useFsiAuxLib = true,
otherFlags = allFlags,
userOpName = "getNetCoreScriptOptions"
)
...
member private __.GetNetFxScriptOptions(file: string<LocalPath>, source) =
async {
optsLogger.info (
Log.setMessage "Getting NetFX options for script file {file}"
>> Log.addContextDestructured "file" file
)
let allFlags = Array.append [| "--targetprofile:mscorlib" |] fsiAdditionalArguments
let! (opts, errors) =
checker.GetProjectOptionsFromScript(
UMX.untag file,
source,
assumeDotNetFramework = true,
useFsiAuxLib = true,
otherFlags = allFlags,
userOpName = "getNetFrameworkScriptOptions"
)
...
member private __.GetNetCoreScriptOptions(file: string<LocalPath>, source) =
async {
optsLogger.info (
Log.setMessage "Getting NetCore options for script file {file}"
>> Log.addContextDestructured "file" file
)
let allFlags =
Array.append [| "--targetprofile:netstandard" |] fsiAdditionalArguments
let! (opts, errors) =
checker.GetProjectOptionsFromScript(
UMX.untag file,
source,
assumeDotNetFramework = false,
useSdkRefs = true,
useFsiAuxLib = true,
otherFlags = allFlags,
userOpName = "getNetCoreScriptOptions"
)
... |
Right now editors spawn the FSI instances, yes, but part of the intent of this parameter was to look towards a time when FSAC used the hosted-FSI APIs instead of driving and FSI instances over stdin/stdout communication. This would eliminate a source of mixed-results that exists today, namely that the FSI spawned from your SDK is not using the same analysis as the FSharpChecker analyzing your script inside FSAC. |
Some more digging and thinking. The option is a field on In the present codebase, and without any FSAC-managed interpreterA couple options present themselves to me here:
The future goal of moving the interpreter into FSAC's controlQuestion: do the hosted FSI APIs take similar parameters as the command line
Here's a quick command to get just the options for $ dotnet fsi --help | awk '/^-/ {print $1}'
--use:<file>
--load:<file>
--reference:<file>
--compilertool:<file>
--usesdkrefs[ |-]
--
--debug[ |-]
--debug:{full|pdbonly|portable|embedded}
--optimize[ |-]
--tailcalls[ |-]
--deterministic[ |-]
--pathmap:<path=sourcePath;...>
--crossoptimize[ |-]
--reflectionfree
--warnaserror[ |-]
--warnaserror[ |-]:<warn;...>
--warn:<n>
--nowarn:<warn;...>
--warnon:<warn;...>
--consolecolors[ |-]
--langversion:?
--langversion:{version|latest|preview}
--checked[ |-]
--define:<string>
--mlcompatibility
--strict-indentation[ |-]
--nologo
--version
--help
--codepage:<n>
--utf8output
--preferreduilang:<string>
--fullpaths
--lib:<dir;...>
--simpleresolution
--targetprofile:<string>
--clearResultsCache
--exec
--gui[ |-]
--quiet
--readline[ |-]
--quotations-debug[ |-]
--shadowcopyreferences[ |-]
--multiemit[ |-] |
The FSI hosting APIs do take the command line arguments directly, see the API reference and tutorial for some examples there, so we would need those in that case. |
So I think that gets us to a design decision for implementing a fix here. I lean toward 1.2
I think I can take on any of these, but would prefer input from @baronfel or another maintainer on the choice and any specifics of implementation that matter. |
Thanks for laying out some plans and thoughts! 1.2 is perfectly agreeable by me, and I'd love to review and merge a PR implementing it if you feel so inclined. |
Linking for reference here, and will include in documentation as part of PR:
The page for interactive options helpfully lists whether an option is shared by the compiler as well. I expect this will be a useful doc reference for instructing users how to configure things. |
`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
@baronfel , PR is raised. I am happy to take any feedback you may have. I've tried to capture and consolidate the discussion here, and also make clear the way that this is intended to address current use cases and grow cleanly into the future where FSAC learns to manage the interactive interpreter. |
Oh, interesting. |
Add `fsiExtraInteractiveParameters` and `fsiExtraSharedParameters`, which should both be used in favor of the old `fsiExtraParameters`. Some fsi parameters are exclusive to the interpreter and have no compiler equivalent. FSAC will now pass the shared parameters to the compiler and not the interactive parameters. Ionide-vim will append all parameters from both options to the execution of the interpreter (usually `dotnet fsi`). `fsiExtraParameters` will be deprecated and removed from FSAC, so it is best to migrate away from that. See ionide/FsAutoComplete#1210 for more detail.
# FIX: #1210; split `FSIExtraParameters` in two `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: - #1210: proximate bug: #1210 - `dotnet fsi` readline bug, making this option very salient for editor plugins: dotnet/fsharp#17215
@baronfel , I just wanted to say thanks for your thoughtful and prompt feedback on this. This was definitely a great first contribution experience for me (: Keep it up! |
Glad to hear it! It's very motivating from my end when someone reports an issue with great detail and is gung-ho about making a quality contribution as well. |
Version
ionide.vscode v7.16.1, fsac 0.68.0
Dotnet Info
.NET SDK:
Version: 7.0.400
Commit: 73bf45718d
Runtime Environment:
OS Name: Mac OS X
OS Version: 12.3
OS Platform: Darwin
RID: osx.12-arm64
Base Path: /Users/horacegonzalez/.dotnet/sdk/7.0.400/
Host:
Version: 8.0.0
Architecture: arm64
Commit: 5535e31a71
.NET SDKs installed:
6.0.108 [/Users/horacegonzalez/.dotnet/sdk]
6.0.300 [/Users/horacegonzalez/.dotnet/sdk]
6.0.303 [/Users/horacegonzalez/.dotnet/sdk]
6.0.400 [/Users/horacegonzalez/.dotnet/sdk]
6.0.408 [/Users/horacegonzalez/.dotnet/sdk]
7.0.201 [/Users/horacegonzalez/.dotnet/sdk]
7.0.400 [/Users/horacegonzalez/.dotnet/sdk]
8.0.100-preview.3.23178.7 [/Users/horacegonzalez/.dotnet/sdk]
8.0.100-rc.2.23502.2 [/Users/horacegonzalez/.dotnet/sdk]
8.0.100 [/Users/horacegonzalez/.dotnet/sdk]
.NET runtimes installed:
Microsoft.AspNetCore.App 6.0.5 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.8 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.16 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.3 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.10 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.0-preview.3.23177.8 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.0-rc.2.23480.2 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.0 [/Users/horacegonzalez/.dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 6.0.5 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.8 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.16 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.3 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.10 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.0-preview.3.23174.8 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.0-rc.2.23479.6 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.0 [/Users/horacegonzalez/.dotnet/shared/Microsoft.NETCore.App]
Other architectures found:
x64 [/usr/local/share/dotnet/x64]
registered at [/etc/dotnet/install_location_x64]
Environment variables:
DOTNET_ROOT [/Users/horacegonzalez/.dotnet]
global.json file:
/Users/horacegonzalez/code/FsAutoComplete/global.json
Learn more:
https://aka.ms/dotnet/info
Download .NET:
https://aka.ms/dotnet/download
Steps to reproduce
temp
(anywhere is fine)temp/test.fsx
file with the following text:temp/.vscode/settings.json
file with the following text:test.fsx
Details
Expected Behavior
Actual Behavior
Logs
Here's the notification from FSAC that's causing the issue:
Notice the
range.start.line: -1
andrange.end.line: -1
. This causes the LSP client to throw an error because the range is invalid: https://github.com/microsoft/vscode-languageserver-node/blob/02806427ce7251ec8fa2ff068febd9a9e59dbd2f/client/src/common/protocolConverter.ts#L400This results in the second
The type 'string' is not compatible with any of the types ...
error not being processed.Checklist
The text was updated successfully, but these errors were encountered: