ionide / FsAutoComplete

F# language server using Language Server Protocol
Other
413 stars 154 forks source link

Not finding record properties in Emacs or Atom #131

Closed Tombert closed 5 years ago

Tombert commented 8 years ago

In this example, when I try doing

type Foo = {
   Bar : string
   Baz : string
} with 
    static member Empty = {
       Foo.Bar
       Foo.Baz
    }

I don't get any autocomplete suggestion for Bar or Baz in either Atom, VS Code, or Emacs when I was defining Empty. This does work with vanilla Visual Studio, leading me to believe that it's either a bug in fsautocomplete or the implementations.

Is this the result of these plugins just not sending the correct flags to fsautocomplete or is it something that's an issue with the core itself? If it's the latter I'd love to help fix it!

rneatherway commented 7 years ago

Hi @Tombert, I think this is an issue with fsautocomplete itself. It's an interesting one, I can't see immediately why there would be a problem here. I do notice that this works fine interacting with a record in the usual way e.g:

type Foo = {
   Bar : string
   Baz : string
}

let y = { Bar = ""; Baz = ""}

y.Bar

The only thing that springs to mind as a possible candidate is Program.fs:48, where a reparse is required in the case of typed expressions e.g. (f x).$ ($ is cursor) in order to know what the completions are. Perhaps that applies here too? But then I would have thought that eventually this data would be available anyway.

If Empty is static though, what would the value of those fields be? I'm not quite sure how this is supposed to behave.

Tombert commented 7 years ago

@rneatherway I should amend my snippet for Empty to this:

static member Empty = {
       Foo.Bar = ""
       Foo.Baz = ""
 }

so that I may have a way to generate an empty base-instance of a type.

Regardless, the autocomplete doesn't work, and looking at the line you cited, it appears that there's a TODO for what you mentioned. How hard would a reparse be? Would that be too expensive to just add in?

rneatherway commented 7 years ago

Regardless, the autocomplete doesn't work

After further thought, I think it is correct that no completions are offered here. I don't think it makes sense to offer completions for a field name, after all it isn't possible to call ToString() on the field name, only on a value of that field. I get completions perfectly in a non-static method (see ToUpper() below):

type Foo = {
   Bar : string
   Baz : string
} with 
    static member Empty = {
       Foo.Bar = ""
       Foo.Baz = ""
    }

    member x.Add s = {
        Foo.Bar = x.Bar.ToUpper() + s
        Foo.Baz = x.Baz + s
    }

The reparse would be useful in other cases, but it is a case of working out when you want to do it, and as it is a while since I wrote that TODO I can't remember what my plans were back then.

Tombert commented 7 years ago

@rneatherway That's fair enough, I'm mostly curious now to why it works that way in vanilla VS.

Since Foo is a kind of contrived example, I realize it's not abundantly obvious why I would care about this feature; I have a type with around 30 properties, so the autocomplete in Empty would be useful to prevent me from a) typing a lot cuz I'm lazy and b) offset my incompetence and prevent typos.

If this breaks paradigm too much, I obviously won't blame anyone for not adding it, I mostly wanted to see if this was expected behavior or a bug and/or feature parity between VS and fsautocomplete was planned.

rneatherway commented 7 years ago

Actually, I think I am wrong after all, sorry. I thought you wanted autocompletions of Foo.Bar.$ (like ToCharArray, ToString, etc.), but you actually want completions of Foo.$. I absolutely think that the field names should be offered here. I have in fact verified that FCS returns the correct completions if called in isolation:

module Project41 = 
    open System.IO

    let fileName1 = Path.ChangeExtension(Path.GetTempFileName(), ".fs")
    let base2 = Path.GetTempFileName()
    let dllName = Path.ChangeExtension(base2, ".dll")
    let projFileName = Path.ChangeExtension(base2, ".fsproj")
    let fileSource1 = """
type Foo = {
   Bar : string
   Baz : string
} with 
    static member Empty = {
       Foo.
    }
    """

    File.WriteAllText(fileName1, fileSource1)
    let fileNames = [fileName1]
    let args = mkProjectCommandLineArgs (dllName, fileNames)
    let options =  checker.GetProjectOptionsFromCommandLineArgs (projFileName, args)
    let cleanFileName a = if a = fileName1 then "file1" else "??"

[<Test>]
let ``Test Project41 all symbols`` () = 

    let parseResults, checkAnswer = 
        checker.ParseAndCheckFileInProject(Project41.fileName1, 0, Project41.fileSource1, Project41.options) 
        |> Async.RunSynchronously

    let checkFileResults = 
        match checkAnswer with
        | FSharpCheckFileAnswer.Succeeded(res) -> res
        | res -> failwithf "Parsing did not finish... (%A)" res

    let decls = 
        checkFileResults.GetDeclarationListInfo
          (Some parseResults, 7, 12, "       Foo.Bar = \"\"", [], "Foo", fun _ -> false)
        |> Async.RunSynchronously

    for decl in decls.Items do
        printfn "%A" decl.Name

So I'll dig a bit more into the FSAC calls to make sure they are as expected and where the results differ.

Tombert commented 7 years ago

@rneatherway It appears that it works ok with the first field name, but if you do any more than one, all it offers are the methods, not the properties.

rneatherway commented 7 years ago

How are you testing now? Emacs/VsCode?

Tombert commented 7 years ago

I'm using Atom for testing this out with the Ionide plugin.

rneatherway commented 7 years ago

I confirm the behaviour in FSAC. I find it confusing that repeated requests yield different results. Perhaps it is related to some caching.

Tombert commented 7 years ago

I'd be happy to help fix it somehow, if I had any intuition on how to do so; what would be a good place to start looking?

rneatherway commented 7 years ago

To be honest it will be quite involved to do so. However, if you are feeling brave, I started investigating by creating a new small integration test based on one of the existing ones, with a single script file, and sending completion requests from there. You can run the integration tests from the command line with FAKE (e.g. ./build.sh test). The output.json file created in the test directory contains the results of the parse. Once you have that, you can try inserting print statements, which will also go to the output.json, to try to figure out where things are going wrong. Somewhere in Commands.fs or CompilerServiceInterface.fs would be my guess. One thing to try would be to avoid any caching that you find. I'll be away for a few days, but I haven't forgotten about this issue.

Krzysztof-Cieslak commented 5 years ago

This is really outdated, so I'll just close this one. We've probably fixed that ;-)