fsiExtraParameters break error hints in editor while editing .fsx files

HoraceGonzalez commented 7 months ago

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

create a new workspace folder called temp (anywhere is fine)
Create a new temp/test.fsx file with the following text:
```
printfn "%d" "The number 1" 
```
create a new temp/.vscode/settings.json file with the following text:
```
{
"FSharp.fsiExtraParameters": [
    "--readline-"
]
}
```
open the folder in vscode.
open test.fsx

Details

Expected Behavior

Actual Behavior

Logs

Here's the notification from FSAC that's causing the issue:

[Trace - 6:13:28 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///Users/horacegonzalez/code/temp/test.fsx",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": -1,
                    "character": 0
                },
                "end": {
                    "line": -1,
                    "character": 0
                }
            },
            "severity": 1,
            "code": "243",
            "codeDescription": {
                "href": "https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/compiler-messages/fs0243"
            },
            "source": "F# Compiler",
            "message": "Unrecognized option: '--readline-'. Use '--help' to learn about recognized command line options.",
            "relatedInformation": []
        },
        {
            "range": {
                "start": {
                    "line": 1,
                    "character": 13
                },
                "end": {
                    "line": 1,
                    "character": 27
                }
            },
            "severity": 1,
            "code": "1",
            "codeDescription": {
                "href": "https://docs.microsoft.com/en-us/dotnet/fsharp/language-reference/compiler-messages/fs0001"
            },
            "source": "F# Compiler",
            "message": "The type 'string' is not compatible with any of the types byte,int16,int32,int64,sbyte,uint16,uint32,uint64,nativeint,unativeint, arising from the use of a printf-style format string",
            "relatedInformation": []
        }
    ]
}

Notice the range.start.line: -1 and range.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#L400

This results in the second The type 'string' is not compatible with any of the types ... error not being processed.

Checklist

[X] I have looked through existing issues to make sure that this bug has not been reported before
[X] I have provided a descriptive title for this issue
[X] I have made sure that that this bug is reproducible on the latest version of the package
[X] I have provided all the information needed to reproduce this bug as efficiently as possible
[X] I or my company would be willing to contribute this fix

HoraceGonzalez commented 7 months ago

The underlying issue here seems to be that fsi-specific options are being passed to the FSharp Checker in the project options, and fsc fails with the following error:

Unrecognized option: '--readline-'. Use '--help' to learn about recognized command line options.

I was able to workaround the problem locally in AdaptiveServerState.fs by filtering out --readline- right before the file it checked:

I'm happy to implement a more robust fix, but wasn't sure what the right way to fix this would be. The idea I had was to remove any fsi-specific options from the OtherOptions. Might be a bit brittle if new options are added in future releases, though.

Thoughts?

greggyb commented 1 month ago

I'm seeing the same issue with Ionide-vim with the same option (fsiExtraParameters). This is particularly annoying, because --readline- is necessary when doing interactive development with scripts to work around this issue in dotnet fsi.

It ends up showing a repeated diagnostic error at the top of the .fsx file:

baronfel commented 1 month ago

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 (0, 0)) so that the LSP client isn't getting invalid data.

Would either of you be open to submitting one or both of these changes?

greggyb commented 1 month ago

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 (FSIExtraParameters) ends up getting combined with options for things that are not, in fact, an interactive interpreter. That seems to be the core of the issue.

It appears that this is happening in four methods on the class FSharpCompilerServiceChecker. Relevant portions excerpted below from src/FsAutoComplete.Core/CompilerServiceInterface.fs.

You'll see that each of these appends the fsi arguments into an array which ends up as part of FSharpChecker's otherOptions field. Each of the four methods below does basically the same thing with respect to how they treat these parameters intended only for the FSI interpreter.

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 dotnet fsi itself and concatenates on the extra parameters locally in the vim process. There is no interaction I am aware of from Ionide-vim through the fsautocomplete process to the dotnet fsi process.

I have already burned more time than I should have in digging into this. Some questions:

Does fsautocomplete actually do anything to interact with a dotnet fsi process?
What is the intended purpose of the FSIExtraParameters configuration option?
Does it seem reasonable to rip out the Array.append that is happening in each of the methods below?

++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"
        )
...

baronfel commented 1 month ago

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.

greggyb commented 1 month ago

Some more digging and thinking. The option is a field on FSharpConfig and FSharpConfigDto. It is only ever set in two methods, both of which read from a Dto. One for instantiation and one for modification. It seems that this would be the place to strip out the dotnet fsi command line options that cannot be used with the compiler.

In the present codebase, and without any FSAC-managed interpreter

A couple options present themselves to me here:

Teach the members FromDto and AddDto to strip out dotnet fsi-only options.
Update docs and usage guidance to say that this parameter is only for shared options between dotnet fsi and the compiler. Delegate dotnet fsi-only options to the editor, where the editor configuration can take an additional, separate configuration (that is never sent to FSAC) for how to invoke dotnet fsi.

The future goal of moving the interpreter into FSAC's control

Question: do the hosted FSI APIs take similar parameters as the command line dotnet fsi?

If yes, then FSAC will need to learn to distinguish the interactive-only options from the shared-with-compiler options. I think this aligns with approach 1 above. For now, we teach the members that read config to ignore interpreter-only options. In the future, we enhance this to split the options up as necessary to invoke things correctly. Or, have two sets of config options, one for interactive-only options and one for shared-with-compiler options.
If no, then FSAC should never know about interactive-only options. This aligns more with approach 2 above. For now, editors manage the interactive-only options. In the future, they are unnecessary.

Here's a quick command to get just the options for dotnet fsi. I'm not sure how to get the same for the compiler. (command and sample output at end). This list is not immediately usable for direct string comparisons, as many options, as shown, would expand into multiple valid options. For example, the line below --realine[+|-] could expand to each of --readline- and readline+. Others would properly be a regex or some other pattern match, rather than a literal equality test.

$ 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[+|-]

baronfel commented 1 month ago

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.

greggyb commented 1 month ago

So I think that gets us to a design decision for implementing a fix here. I lean toward 1.2

Split the FSIExtraParameters into two fields: FSIExtraInteractiveParameters and FSIExtraSharedParameters. The configuration is such that FSIExtraInteractiveParameters is only used for interactive things and FSIExtraSharedParameters is used for both interactive, and when passing on parameters to compiler/checker. Today, FSIExtraInteractiveParameters is useless. Therefore:
1. Don't implement FSIExtraInteractiveParameters today. Let the editors deal with this on their own
2. Do implement it today. FSAC does nothing with it. Editors can use this directly (and drop their use of it in the future, just passing it on to FSAC)
Teach FSAC to select between the interactive-only parameters and those that are shared with compiler/checker. Today, we use that to select only the shared parameters today at the call-sites I mentioned above. In the future, we use all for the FSAC-managed interpreter.

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.

baronfel commented 1 month ago

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.

greggyb commented 1 month ago

Linking for reference here, and will include in documentation as part of PR:

Interactive options: https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/fsharp-interactive-options
Compiler options: https://learn.microsoft.com/en-us/dotnet/fsharp/language-reference/compiler-options

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.

greggyb commented 1 month ago

@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.

HoraceGonzalez commented 1 month ago

look towards a time when FSAC used the hosted-FSI APIs instead of driving and FSI instances over stdin/stdout

Oh, interesting.

greggyb commented 1 month ago

@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!

baronfel commented 1 month ago

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.

ionide / FsAutoComplete