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

Fix ImplementInterface (previously GenerateInterfaceStub) #929

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

Booksbaum
Copy link
Contributor

(Mostly port of dotnet/fsharp(VS)/FSharpImplementInterfaceCodeFixProvider -> use InterfaceStubGenerator from dotnet/fsharp instead of custom one)

  • Rename GenerateInterfaceStub to ImplementInterface
    • Note: I didn't rename settings (InterfaceStubGeneration, InterfaceStubGenerationObjectIdentifier, InterfaceStubGenerationMethodBody) to not break existing tools
  • Fix: Doesn't trigger for types (just for object expressions)
  • Fix: Inserts already implemented members too
  • Enhancement: CodeFix triggers now on full diagnostic range
    -> cursor can be anywhere Error FS0366 is highlighted. For type that's just interface name, but in object expression that's over the complete object expression
  • Add: Implement Interface without type annotation
    • Previously: always with type annotation
    • Now: User can choose between both (-> two CodeFixes)
  • Rename title to Implement interface (and Implement interface without type annotation)
    -> align with dotnet/fsharp(VS)/FSharpImplementInterfaceCodeFixProvider and better title (I think)
  • Enhancement: Add new members after existing members
    • Question: Should there be a empty line between existing and new members? Or maybe just in type, but not obj expr (-> obj expr: "inline" -> favor shortness)
      • Currently: directly after last member
      • Or get from existing member? -> if empty line between existing members then insert empty line too
        • But: no empty line between new members (-> is from FCS)
  • Fix: Issues with triggering of CodeFix, alignment of members, indentation of members
    Some actual fixes: (listed to remind myself to write corresponding tests (✓done) and left here because why not?!...)
    • Fix: Incorrect alignment when new or interface on different line than interface identifier

    • Fix: with gets inserted when existing with on different line than interface identifier

    • Fix: Sometimes no indentation of members in main interface in obj expr

    • Fix: Wrong indentation when implementing sub/additional interfaces in object expression (aligned under interface identifier instead of aligned and indented from interface keyword)

    • Note: There are quite a lot of special cases around alignment and when it is valid vs invalid.

      • Example of special case and difference between interface in type and obj expr:
        type A () =
            // `member` must be behind `interface`
            interface IDisposable with
        //member _.Dispose () = ()            // invalid
        //  member _.Dispose () = ()          // invalid
                member _.Dispose () = ()      // valid
        
        {   new IDisposable with
        //member _.Dispose () = ()            // valid
        //  member _.Dispose () = ()          // valid
                member _.Dispose () = ()      // valid
        }
        let _ = { new IDisposable with
        //member _.Dispose () = ()              // invalid: possible incorrect indentation
        //^^^^^^ should be at line start/no leading spaces (`//` just here to uncomment)
          member _.Dispose () = ()              // valid
            }
        -> used alignment:
        • when existing member: align with existing member
        • otherwise: column of interface or new and then indent a bit further

      There are probably still some cases I didn't catch or aren't covered by tests

    • Note: most (now adjusted) alignments/indentations were valid -- but suboptimal position (too far indented to right)

  • Change: member instead of override
    • Reason: FSharp.Compiler.EditorServices.InterfaceStubGenerator uses member
  • Internal Restructure:
    • Use FSharp.Compiler.EditorServices.InterfaceStubGenerator instead of FsAutoComplete.InterfaceStubGenerator
      (mostly same -- I think previously EditorServices.InterfaceStubGenerator wasn't public -> copied into FSAC)
    • Remove all InterfaceStub Code from FsAutoComplete.Core
      • I think it fits better into location for CodeFixes. And because InterfaceStubGenerator is now public in FSharp.Compiler.Service (-> most stuff for InterfaceStub in Commands is now unnecessary)
      • Note: This removes public Functionality: FsAutoComplete.Commands.GetInterfaceStub(...) and FsAutoComplete.InterfaceStubGenerator
        -> TODO: needs review
  • Add tests
    • Note: quite a lot results have a space after =. But because of linebreaks that's not visible in editors -> use Render Whitespace (or similar) in editor
      • Space comes from FCS


  • Add: Setting IndentationSize
    -> fallback when indentation cannot deduced from context
    • default: 4
    • Currently only used in ImplementInterface (-> indentation of members)
    • Should be provided by LSP client
      • TODO: Add to Ionide
        • source? tabSize, .editorconfig, VSCode even auto-detect indentation for each file -> possible to specify per file?
    • In editors it's usually called something like tabSize (in VS Code) -> use TabSize instead? (indent_size in .editorconfig)

Copy link
Contributor

@baronfel baronfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! To answer some of your outstanding questions:

Newlines between members: the current behavior is fine. If a user has fantomas configured (and they should if they're using FSAC), then saving the file will re-format anyway, and their desired configuration will be applied.

Configuration setting: we should expose it in VSCode, but we may be able to get language-specific settings (like editor.tabSize) automatically via the workspace/didChangeConfiguration call. If so that would be a good way to stay notified of user-led changes. I'm not sure if that setting reflects editorconfig though.

src/FsAutoComplete/CodeFixes/ImplementInterface.fs Outdated Show resolved Hide resolved
test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs Outdated Show resolved Hide resolved
test/FsAutoComplete.Tests.Lsp/CodeFixTests.fs Outdated Show resolved Hide resolved
(Mostly port of `dotnet/fsharp`(VS)/`FSharpImplementInterfaceCodeFixProvider` -> use `InterfaceStubGenerator` from `dotnet/fsharp` instead of custom one)

* Rename `GenerateInterfaceStub` to `ImplementInterface`
  * Note: I didn't rename settings (`InterfaceStubGeneration`, `InterfaceStubGenerationObjectIdentifier`, `InterfaceStubGenerationMethodBody`) to not break existing tools
* Fix: Doesn't trigger for `type`s (just for object expressions)
* Fix: Inserts already implemented members too
* Enhancement: CodeFix trigger now on full diagnostic range
  -> cursor can be anywhere Error `FS0366` is highlighted. For type that's just interface name, but in object expression that's over the complete object expression
* Add: Implement Interface without type annotation
  * Previously: always with type annotation
  * Now: User can choose between both (-> two CodeFixes)
* Rename title to `Implement interface` (and `Implement interface without type annotation`)
  -> align with `dotnet/fsharp`(VS)/`FSharpImplementInterfaceCodeFixProvider` and better title (I think)
* Enhancement: Add new members after existing members
* Fix: Issues with triggering of CodeFix, alignment of members, indentation of members
  Some actual fixes: (listed to remind myself to write corresponding tests (✓done) and left here because why not?!...)
  * Fix: Incorrect alignment when `new` or `interface` on different line than interface identifier
  * Fix: `with` gets inserted when existing `with` on different line than interface identifier
  * Fix: Sometimes no indentation of members in main interface in obj expr
  * Fix: Wrong indentation when implementing sub/additional interfaces in object expression (aligned under interface identifier instead of aligned and indented from `interface` keyword)
  * Note: There are quite a lot of special cases around alignment and when it is valid vs invalid.
    * Example of special case and difference between interface in type and obj expr:
      ```fsharp
      type A () =
          // `member` must be behind `interface`
          interface IDisposable with
      //member _.Dispose () = ()            // invalid
      //  member _.Dispose () = ()          // invalid
              member _.Dispose () = ()      // valid

      {   new IDisposable with
      //member _.Dispose () = ()            // valid
      //  member _.Dispose () = ()          // valid
              member _.Dispose () = ()      // valid
      }
      let _ = { new IDisposable with
      //member _.Dispose () = ()              // invalid: possible incorrect indentation
      //^^^^^^ should be at line start/no leading spaces (`//` just here to uncomment)
        member _.Dispose () = ()              // valid
          }
      ```
      -> used alignment:
      * when existing member: align with existing member
      * otherwise: column of `interface` or `new` and then indent a bit further

    There are probably still some cases I didn't catch or aren't covered by tests
  * Note: most fixed alignments/indentations were valid -- but suboptimal position (too far indented to right)
* Change: `member` instead of `override`
  * Reason: `FSharp.Compiler.EditorServices.InterfaceStubGenerator` uses `member`
* Internal Restructure:
  * Use `FSharp.Compiler.EditorServices.InterfaceStubGenerator` instead of `FsAutoComplete.InterfaceStubGenerator`
  * Remove all InterfaceStub Code from `FsAutoComplete.Core`
    * I think it fits better into location for CodeFixes. And because `InterfaceStubGenerator` is now public in `FSharp.Compiler.Service` (-> most stuff for InterfaceStub in `Commands` is now unnecessary)
    * Note: This removes public Functionality: `FsAutoComplete.Commands.GetInterfaceStub(...)` and `FsAutoComplete.InterfaceStubGenerator`
    -> **TODO**: needs review
* Add tests
  * Note: quite a lot results have a space after `=`. But because of linebreaks that's not visible in editors -> use `Render Whitespace` (or similar) in editor
    * Space comes from `FCS`
@Booksbaum Booksbaum force-pushed the ImplementInterfaceCodeFix branch from 004a0ca to 298f841 Compare April 26, 2022 11:55
@baronfel baronfel merged commit d90597c into ionide:main Apr 27, 2022
@Booksbaum Booksbaum deleted the ImplementInterfaceCodeFix branch April 27, 2022 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants