FSharp Refactor integration (GSoC 2013)

[Lewix]

This pull request contains the changes integrating the fsharp-refactor library into fsharpbinding. The project was carried out over the summer as part of Google Summer of Code, under the supervision of @rneatherway. Changes have been made in two main parts of the code base: the build process and the MonoDevelop add-in. I have merged in the most recent changes to the master branch of fsharpbinding.

Build process modifications

fsharp-refactor is included as a submodule in the root of the project. configure.sh has been modified to initialise this submodule and recursively call fsharp-refactor's configure script. The main makefile has also been updated to build fsharp-refactor.

Unrelated to the fsharp-refactor integration, I removed some unused code in configure.sh, and added auto-generated files such as monodevelop/MonoDevelop.FSharp.local.fsproj and monodevelop/Makefile to the .gitignore file. The aim of this is to avoid git constantly complaining that the fsproj file has been modified, and to avoid changes which are overwritten when the files are regenerated being committed.

MonoDevelop add-in modifications

FSharpLanguageBinding.fs has been updated to start fsharp-refactor's compile on project startup and to parse the current file in advance. This substantially reduces the lag when first right-clicking in a project. The .addin.xml file has been modified to include Rename as a refactoring (AddArgument and ExtractFunction aren't really usable enough yet). Unfortunately the issue with duplicate "Rename" refactorings (MonoDevelop's and fsharp-refactor's) has not yet been resolved.

The rest of the changes are self-contained. There is a class for each new refactoring with shared code in FSharpRefactor/FSharpRefactoring.fs. An additional gtk dialog was created for choosing refactoring arguments.

Cross-platform compatibility

The plugin has been developed and tested on Linux. I have however performed some basic testing in Xamarin Studio on Mac and Windows without issues (I had no menus on Mac OS, but that seems unrelated).

[ghost]

Hi @Lewix

Some comments

• the clean separation of the "logic" of refactoring into the fsharp-refactor project is nice
• the user interface elements are nice

However:

• I'm concerned about the lack of comments in fsharp-refactor. I'd encourage all F# code that is not self-explanatory to have a /// comment on each top-level type and module definition, and all significant function and member definitions.
• The refactoring logic is implemented over the untyped AST, and you do your own name resolution over this. In the long term it feels like the implementation needs to be on a trajectory to make use of future evolutions of @7sharp9's hard-binding to FSharp.Compiler.Editing.dll, and be based on the F# language service background compiler and typed AST (since this contains the results of the F# compiler's name resolution logic). However I understand that's non-trivial to implement.
• If I understand it correctly, the current implementation doesn't allow rename-refactoring of member definitions?

Just some initial thoughts. I know @rneatherway, @tpetricek and @7sharp9 will also be interested in where fsharpbinding is going with this.

[7sharp9]

Theres an awful lot to review here, Im feeling that the commits are a bit lacking in detail for me to understand how they relate to the front matter description.

[7sharp9]

The FSharp.Compiler.Editor.dll hasn't yet been merged into master yet, my hardbinding fork is the current work in progress in this area. This merge will have to occur after that.

[rneatherway]

So, yes, I haven't commented on this yet because we should get the
hardbinding in first.

On Sun, Sep 29, 2013 at 1:29 PM, Dave Thomas notifications@github.comwrote:

In monodevelop/MonoDevelop.FSharpBinding/FSharpBinding.addin.xml.orig:

@@ -7,6 +7,8 @@

The FSharp.Compiler.Editor.dll hasn't yet been merged into master yet, my
hardbinding fork is the current work in progress in this area. This merge
will have to occur after that.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209/files#r6647002
.

[rneatherway]

Thanks for taking a look Don.

I was mentoring Lewis during GSoC and we were both keen to get something
self-contained working by the end of the summer so the architecture
shouldn't be considered to be in stone. I agree with your comment about
using the typed AST eventually. Lewis and I have discussed using the hard
binding, and this is one reason I was keen to move forward with merging
that change even without the intellisense improvements.

On Sun, Sep 29, 2013 at 12:40 PM, Don Syme notifications@github.com wrote:

Hi @Lewix https://github.com/Lewix

Some comments

• the clean separation of the "logic" of refactoring into the
  fsharp-refactor project is nice
• the user interface elements are nice

However:

• I'm concerned about the lack of comments in fsharp-refactor. I'd
  encourage all F# code that is not self-explanatory to have a /// comment on
  each top-level type and module definition, and all significant function and
  member definitions.
• The refactoring logic is implemented over the untyped AST, and you
  do your own name resolution over this. In the long term it feels like the
  implementation needs to be on a trajectory to make use of future evolutions
  of @7sharp9 https://github.com/7sharp9's hard-binding to
  FSharp.Compiler.Editing.dll, and be based on the F# language service
  background compiler and typed AST (since this contains the results of the
  F# compiler's name resolution logic). However I understand that's
  non-trivial to implement.
• If I understand it correctly, the current implementation doesn't
  allow rename-refactoring of member definitions?

Just some initial thoughts. I know @rneatherwayhttps://github.com/rneatherway,
@tpetricek https://github.com/tpetricek and @7sharp9https://github.com/7sharp9will also be interested in where fsharpbinding is going with this.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209#issuecomment-25318774
.

[ghost]

@7sharp9 - note that most of the logic is in https://github.com/mono-soc-2013/fsharp-refactor

@rneatherway - yes, I can see value in the "get it stable and integrate" approach even if the direction needs changing on the next iteration - most importantly the implementation of the logic in fsharp-refactor can be exchanged out at any future point, and it is nice to have all the UI in place to allow someone to take that on in the future. The UI controls are also a goo template for adding future refactorings

[7sharp9]

@dsyme I really wanted to get the rename refactoring using the the build in one in MonoDevelop, by following the extension methodology rather than bolting on top would give us follow reference functionality into C# code.

[7sharp9]

So am I right in thinking there are two background code checkers running, one in the fsharpbinding itself and one in the reractor submodule?

[Lewix]

@7sharp9 Yes, there are essentially two code checkers running. I tried to use the TypeCheckResults that are already available through MonoDevelop but it didn't work very well. They're updated slowly so when trying to rename twice in a row you would get weird behaviour.

Which extension methodology are you referring to for Rename? There aren't any extension points that I could find in MonoDevelop.Refactoring.Rename. Or do you mean NRefactory stuff?

@dsyme I agree there needs to be some documentation in fsharp-refactor. Especially for the functions which get called from fsharpbinding.

Renaming class members is not supported. To support them, I think the main change that's needed is making the reference finder (https://github.com/Lewix/fsharp-refactor/blob/master/FSharpRefactor/ReferenceFinder.fs) find them properly.

[ghost]

Oh, the binding uses the "GetDeclarationLocation" logic to determine references based on an initial name filtering. That's very clever, and will indeed be accurate enough. https://github.com/Lewix/fsharp-refactor/search?q=TryGetDeclarationLocation&ref=cmdform

@7sharp9 - that gives me more confidence of future convergence with future FSharp.Compiler.Editor.dll functionality and that there is not major duplication of compiler logic going on here.

[7sharp9]

@Lewix " I tried to use the TypeCheckResults that are already available through MonoDevelop but it didn't work very well. They're updated slowly so when trying to rename twice in a row you would get weird behaviour."

Is that because its accessed via the agent via an async reply? There is a lag between whats currently compiled and the latest code on screen, especially is your changing a file. If you have a way of accessing the AST quicker then I would be keen to know?

[Lewix]

@7sharp9 I get the AST by calling UntypedParse on the InteractiveChecker. That's the same as what FSharp.AutoComplete is doing: https://github.com/fsharp/fsharpbinding/blob/master/FSharp.AutoComplete/Program.fs#L94

[7sharp9]

@Lewix So your just saying that the agent is slowing things down then?

The language service does an untyped parse here

This is followed by a typed parse of course which will slow thing down dramatically. I have experimented with adding an untyped parse to the language service agent so thats not out of the question. It would be nice to consolidate the core language service features rather than have multiple instances.

FYI: An untyped parse takes around 40ms for a typically file a few hundred lines long.

[Lewix]

@7sharp9 In practice what happens is that after the first Rename (which affects multiple files), a squiggly red line appears below the identifier that was just renamed. Until the red line has disappeared, a further Rename won't get the correct typed info.

When I was using MonoDevelop's TypeCheckResults, I was getting them from here. Looking at it more in detail, I imagine it might be because checker.TryGetRecentTypeCheckResultsForFile doesn't check that the contents of the file is unchanged, so I was getting old results.

Yes, it would be nice if I didn't have to do the parsing and type checking in fsharp-refactor.

[7sharp9]

@Lewix Im not sure where the dirty file checks currently are, its possible that they aren't updated properly.

There is a bit of a lag when the files changes, often too long to do any realtime AST analysis. The dot completion uses parser combinators for this reason. In my hardbinding branch I have been using the tokenizer to try and improve this process.

[dungpa]

@Lewix If you could sum up your GSoC experience with fsharp-refactor, that would be nice. For example, I would like to hear what were the difficulties, what have been done and what are planned for fsharp-refactor after GSoC.

On a side note, do you plan to support Visual Studio in near future?

[rneatherway]

This is the best way I could see of doing it using the existing exposed
services. It still means a call has to be made for every matching
identifier, to check the scope. I wonder if there is a more efficient way?

On Sun, Sep 29, 2013 at 3:20 PM, Don Syme notifications@github.com wrote:

Oh, the binding uses the "GetDeclarationLocation" logic to determine
references based on an initial name filtering. That's very clever, and will
indeed be accurate enough.
https://github.com/Lewix/fsharp-refactor/search?q=TryGetDeclarationLocation&ref=cmdform

@7sharp9 https://github.com/7sharp9 - that gives me more confidence of
future convergence with future FSharp.Compiler.Editor.dll functionality and
that there is not major duplication of compiler logic going on here.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209#issuecomment-25321120
.

[ghost]

Hi Lewis,

The binding has now been updated to use FSharp.Compiler.Editor.dll. Do you think you could update your PR to account for this, adjusting it to use the same nuget package DLL?

Thanks!

[ghost]

Hi @Lewix,

OK, great to see this updated!

Some more things:

• The "fsharp-refector" project doesn't configure on Windows. It's important that we be able to compile the FSharpBinding on Windows and Mac/Linux.
• The "fsharp-binding" project MonoDevelop.FSharp.Gui.csproj doesn't build on Windows because it includes references to specific versions of things like MonoDevelop.Refactoring. These should be configured to use the detected version of MonoDevelop or Xamarin Studio.

To fix this:

• In fsharp-refactor, replace the use of "configure.sh" and "autoconf" with an F# script that configures the project on both Windows and OSX/Linux. For example, see https://github.com/fsharp/fsharpbinding/tree/master/monodevelop, especially configure.bat, configure.sh and configure.fsx.
• In fsharp-binding, adjust the monodevelop\configure.fsx configuration to create MonoDevelop.FSharp.Gui.csproj from a csproj.orig file. In the .csproj.orig the references will look something like this:

    <Reference Include="MonoDevelop.Refactoring">
      <Private>False</Private>
      <HintPath>INSERT_FSPROJ_MDROOT\AddIns\MonoDevelop.Refactoring\MonoDevelop.Refactoring.dll</HintPath>
    </Reference>
```.

[7sharp9]

One of the things Im quite concerned about is performance. I think we should be using a single instance of the compiler service, the performance is currently borderline at times and we don't to add to this with another instance of the compiler also being loaded when it doesn't need to be.

In my recent work on pathed document navigation it is possible to do this by accessing the Document.ParsedDocument.AST. Have a look at the fork here to see what I have been doing: https://github.com/7sharp9/fsharpbinding/commits/PathedDocument

[ghost]

Yes, agreed. The main reason I was building it was just to see what the feel of the feature is like w.r.t. completeness and performance, on both Windows and OSX

[7sharp9]

The current AST can be accessed in the Document.ParsedDocument.AST. The compiler service doesn't currently fill this property even though the saveAST property is true, however, in that fork the AST is saved and can be accessed. I haven't tested enough to submit a PR yet.

[rneatherway]

Lewis and I discussed this and tried setting things up to use the binding
type information. It gets tricky because the refactoring needs completely
up to date information for every file, whether or not it is currently saved.

On Tue, Oct 22, 2013 at 1:27 PM, Dave Thomas notifications@github.comwrote:

One of the things Im quite concerned about is performance. I think we
should be using a single instance of the compiler service, the performance
is currently borderline at times and we don't to add to this with another
instance of the compiler also being loaded when it doesn't need to be.

In my recent work on pathed document navigation it is possible to do this
by accessing the Document.ParsedDocument.AST. Have a look at the fork here
to see what I have been doing:
https://github.com/7sharp9/fsharpbinding/commits/PathedDocument

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209#issuecomment-26798372
.

[7sharp9]

Theres not room for two instances of the compiler running, nor does it make sense, I vote to save all then wait for the compiler to finish. I added a callback to rebuild the path if the AST changes too. We may need to add further functionality to the compiler service maybe adding tag so you know your version has been built. I think thats the only way forward.

On 22 Oct 2013, at 13:44, Robin Neatherway notifications@github.com wrote:

Lewis and I discussed this and tried setting things up to use the binding
type information. It gets tricky because the refactoring needs completely
up to date information for every file, whether or not it is currently saved.

On Tue, Oct 22, 2013 at 1:27 PM, Dave Thomas notifications@github.comwrote:

One of the things Im quite concerned about is performance. I think we
should be using a single instance of the compiler service, the performance
is currently borderline at times and we don't to add to this with another
instance of the compiler also being loaded when it doesn't need to be.

In my recent work on pathed document navigation it is possible to do this
by accessing the Document.ParsedDocument.AST. Have a look at the fork here
to see what I have been doing:
https://github.com/7sharp9/fsharpbinding/commits/PathedDocument

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209#issuecomment-26798372
.

—
Reply to this email directly or view it on GitHub.

[rneatherway]

Yes, obviously it's undesirable. That's the thing is how to know that the
type checking results are up to date, is that information available in the
current binding?

On Tue, Oct 22, 2013 at 1:53 PM, Dave Thomas notifications@github.comwrote:

Theres not room for two instances of the compiler running, nor does it
make sense, I vote to save all then wait for the compiler to finish. I
added a callback to rebuild the path if the AST changes too. We may need to
add further functionality to the compiler service maybe adding tag so you
know your version has been built. I think thats the only way forward.

On 22 Oct 2013, at 13:44, Robin Neatherway notifications@github.com
wrote:

Lewis and I discussed this and tried setting things up to use the
binding
type information. It gets tricky because the refactoring needs
completely
up to date information for every file, whether or not it is currently
saved.

On Tue, Oct 22, 2013 at 1:27 PM, Dave Thomas notifications@github.comwrote:

One of the things Im quite concerned about is performance. I think we
should be using a single instance of the compiler service, the
performance
is currently borderline at times and we don't to add to this with
another
instance of the compiler also being loaded when it doesn't need to be.

In my recent work on pathed document navigation it is possible to do
this
by accessing the Document.ParsedDocument.AST. Have a look at the fork
here
to see what I have been doing:
https://github.com/7sharp9/fsharpbinding/commits/PathedDocument

—
Reply to this email directly or view it on GitHub<
https://github.com/fsharp/fsharpbinding/pull/209#issuecomment-26798372>
.

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/209#issuecomment-26799906
.

[Lewix]

@7sharp9 I actually haven't tried to get only the AST from MonoDevelop. I did try to get the type check results, but as Robin mentioned it didn't work very well. After a refactoring, it was necessary to save all the files and wait a bit. The experience might be a bit better now that the files are saved automatically. Is there a way to know if the type check results have been refreshed?

@dsyme Thanks, I'll try to look at the configuration issues this weekend.

@dungpa
I would say that GSoC was an altogether positive experience. It gave me something to do over summer and I'm pretty happy with what I accomplished in that time.

The hardest part of the summer was probably putting the hours in. Since I was working on a project of my own without any hard deadlines (I had set a few, but they were quite far apart) I had a lot of freedom in when to work. This meant I had time to do other stuff as well, but didn't necessarily work as regularly as I would have liked.

In terms of what I was coding, most of it was my work so I knew my way around it well. Getting it to play nicely with Monodevelop was a lot harder though, and it's still not quite there. There aren't masses of documentation on the subject, but the code is mostly clear (though I found addins problems a bit hard to diagnose).

An interesting problem that came up is how to do project-wide renames, while avoiding name capturing. The solution I have currently looks at import statements to see which names are taken, but it's not perfect yet.

(Sorry for the delay in answering)

[ghost]

@Lewix Excellent, it will be great to have this building on windows.

[ghost]

I believe this request has now been subsumed by the other work on refactoring that has been done by @7sharp9.

@Lewix, thank you for the contribution, I think it was really helpful in spurring improved work on F# tooling and helped show the way both for fsharpbinding and for the new Visual F# Power Tools project.