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.

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

Closed
5 tasks done
HoraceGonzalez opened this issue Nov 27, 2023 · 14 comments · Fixed by #1299
Closed
5 tasks done

fsiExtraParameters break error hints in editor while editing .fsx files #1210

HoraceGonzalez opened this issue Nov 27, 2023 · 14 comments · Fixed by #1299
Labels

Comments

@HoraceGonzalez
Copy link
Contributor

HoraceGonzalez commented Nov 27, 2023

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

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

Details

Expected Behavior

image

Actual Behavior

image

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

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

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:

image

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
Copy link
Contributor

greggyb commented May 22, 2024

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:

image

@baronfel
Copy link
Contributor

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
Copy link
Contributor

greggyb commented May 22, 2024

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:

  1. Does fsautocomplete actually do anything to interact with a dotnet fsi process?
  2. What is the intended purpose of the FSIExtraParameters configuration option?
  3. 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
Copy link
Contributor

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
Copy link
Contributor

greggyb commented May 22, 2024

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:

  1. Teach the members FromDto and AddDto to strip out dotnet fsi-only options.
  2. 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
Copy link
Contributor

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
Copy link
Contributor

greggyb commented May 22, 2024

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

  1. 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)
  2. 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
Copy link
Contributor

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
Copy link
Contributor

greggyb commented May 22, 2024

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.

greggyb added a commit to greggyb/FsAutoComplete that referenced this issue May 23, 2024
`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
@greggyb
Copy link
Contributor

greggyb commented May 23, 2024

@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
Copy link
Contributor Author

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

Oh, interesting.

greggyb added a commit to greggyb/Ionide-vim that referenced this issue May 23, 2024
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.
baronfel added a commit that referenced this issue May 23, 2024
# 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
@greggyb
Copy link
Contributor

greggyb commented May 23, 2024

@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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants