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

Add tests & fixes for Code Fixes #915

Merged
merged 29 commits into from
Apr 11, 2022
Merged

Conversation

Booksbaum
Copy link
Contributor

In this PR: Just "simple" CodeFix cases (-> not to many tests necessary, no huge issues or big necessary changes).

New CodeFixes & more complex cases (like GenerateInterfaceStub) get each an extra PR.

Code Fixes

  • Add tests for CodeFixes:
  • Rename some CodeFix modules & files to match their counterpart in dotnet/fsharp (VS)
    • Note: Not all names were adjusted. Some names I kept because I think their names are more descriptive.
    • Note: In listed fixes below: new names are used

  • Fix: GenerateAbstractClassStub triggers twice
    • Note: Triggered for Errors 54 ('Type is abstract') and error 365 ('No implementation for this members').
      I think both errors always occur together
      -> Now just triggers for error 365
    • Note: CodeFix still has some other issues. See pending tests & commit msg (-> is a more complex case)
  • Fix: AddMissingRecKeyword uses wrong function name in title
  • Fix: AddTypeToIndeterminateValue doesn't trigger
  • Fix: UseMutationWhenValueIsMutable adds <- after = instead of replacing it
  • Fix: ConvertCSharpLambdaToFSharpLambda joins multi-line body to single-line body
  • Fix: MakeDeclarationMutable doesn't trigger in untitled file
  • Fix: AddMissingEqualsToTypeDefinition doesn't trigger
  • Fix: AddMissingEqualsToTypeDefinition adds space on wrong side of =
    • Note: Was valid, just looked odd
    • Note: Wasn't visible because CodeFix didn't trigger
  • Fix: ConvertPositionalDUToNamed doesn't put space before wildcard
    • Note: Was valid, just looked odd
  • Change: Trigger ChangeDerefBangToValue on Diag 3370
    • prev: always check: cursor on ref expression that's accessed with !

    • now: trigger on diag 3370
      -> simpler (and most likely faster)

    • But change in behaviour:

      • prev: trigger on ref expression
      • now: trigger on bang (! -> location of diag)

      Example:

      let fr a = ref a
      let v = !(fr 5)
      //       ^^^^^^ prev trigger range
      //      ^ new trigger range

      I think that's ok: bang gets replaced -> trigger ON bang

  • Fix: ResolveNamespace doesn't trigger when target is in last line
    • Note: Threw exception -> prevented other CodeFixes from running
  • Fix: ReplaceWithSuggestion surrounds identifiers in double-backticks in additional double-backticks (-> 4 backticks: ````hello world````)
  • Fix: RemoveUnusedOpens doesn't trigger in first line

Other Fixes

  • Fix: Goto Declaration fails when declaration is in untitled file

Note: I only fixed what I stumbled across while adding tests.
There might or might not be any corresponding existing issues, which get fixed (or not fixed) in this PR.




Not handled (-> complex cases)

  • ResolveNamespace
    • Needs unification with Auto-open via Code Completion (currently disjoint implementation and open in different location)
    • -> share (at least some) tests
  • GenerateInterfaceStub
    • I'm currently working on this (-> use InterfaceStubGenerator in dotnet/fsharp instead of custom/copied one (I think was previously not public in dotnet/fsharp) )
  • GenerateAbstractClassStub
  • GenerateRecordStub
  • GenerateUnionCases
    • I'm currently working on this

Note: There are some fixes for some of these CodeFixes,
but that's just something small and doesn't cover everything.


Next steps (already in progress):

  • Add missing CodeFixes from dotnet/fsharp (-> Visual Studio)
  • Update GenerateInterfaceStub
  • Update GenerateUnionCases


Question that came up while toying with GenerateUnionCases: Is there a way in FCS to get the type of expression? It seems it's just possible for identifiers, but not expressions in general?

Triggered for two error codes in same location:
* Once for error 54 (`This type is 'abstract' since some abstract members have not been given an implementation.  If this is intentional then add the '[<AbstractClass>]' attribute to your type.`)
* And once for error 365 (`No implementation was given for those members [...]`)

Solution: Trigger only for Error 365

I *think* both errors always occur together
-> triggering just for 365 shouldn't change behaviour

Note:
`GenerateAbstractClassStub` still has some issues: (-> pending tests)
* Inserts text in incorrect location: Seems to inserts members always 1 line down and column with alignment
  -> might be out of text or step over some other code
  * Reason: Location is currently where text should end up -- not where text should be inserted and without leading indentation & new line
* Adds always all overrides instead of just missing ones

-> Same or similar issues (and similar code) as `GenerateInterfaceStub`
-> Delay fixing
Issue was: incorrect file name

Add tests for AddTypeToIndeterminateValue
Examples based on:
dotnet/fsharp#11236 & https://gist.github.com/cartermp/995584694fb6f2f7b228fa8e939795c9
…eplacing it

Rename: `ChangeComparisonToMutableAssignment` to `UseMutationWhenValueIsMutable`
  -> Align with CodeFix in `dotnet/fsharp` (VS)

Add tests for `UseMutationWhenValueIsMutable`
…gle-line body

Rename `ChangeCSharpLambdaToFSharp` to `ConvertCSharpLambdaToFSharpLambda`
  -> align with `dotnet/fsharp` (VS)

Add tests for `ConvertCSharpLambdaToFSharpLambda`
Add tests for `ChangeEqualsInFieldTypeToColon`
Note: It's called `ConvertToNotEqualsEqualityExpression` in `dotnet/fsharp` (VS).
      Unlike other CodeFixes this wasn't renamed
      because `ConvertBangEqualsToInequality` is more descriptive.
Note: Not renamed to match `dotnet/fsharp` (VS; `ConvertToAnonymousRecord`): Current name is more descriptive.
Rename `DoubleEqualsToSingleEquals` to `ConvertDoubleEqualsToSingleEquals`
* Note: Name in `dotnet/fsharp` (VS): `ConvertToSingleEqualsEqualityExpression`
* Reason: `ParseAndCheckResults.TryFindIdentifierDeclaration` fails when declaration is in untitled file (-> `File.Exists` is false)
* Solution: Succeed when decl is in same file as lookup pos

Fix: `MakeDeclarationMutable` doesn't trigger in untitled file

Add tests for `MakeDeclarationMutable`
* Reason: Checking for incorrect diagnostics messages
* Note: CodeFix in `dotnet/fsharp` (VS) only checks for error `3360`, while previous CodeFix in FSAC did additional check for error `10` and additional matching diagnostics messages (which were wrong/outdated?)
  -> CodeFix now checks just for error `3360` like `dotnet/fsharp` (VS)

Fix: `AddMissingEqualsToTypeDefinition` adds space on wrong side of `=`
* Note: Was valid, just looked odd
* Note: Wasn't visible because Fix didn't trigger

Rename `MissingEquals` to `AddMissingEqualsToTypeDefinition` to align with `dotnet/fsharp` (VS)

Add tests for `AddMissingEqualsToTypeDefinition`
…nvocation`

-> align with `dotnet/fsharp` (VS)

Simplify `AddNewKeywordToDisposableConstructorInvocation`: (-> align with `dotnet/fsharp`)
* Trigger on diag code, instead of diag message
* Just add `new ` instead of replacing disposable constructor

Add tests for `AddNewKeywordToDisposableConstructorInvocation`
-> align with `dotnet/fsharp` (VS)

Simplify `WrapExpressionInParentheses`: don't replace text, but instead just add parens

Add tests for `WrapExpressionInParentheses`
Note: called `SimplifyName` in `dotnet/fsharp` (VS).
  But root diag message isn't part of normal F# compiler diags, but instead requires custom analyzer.
  Is called `SimplifyNameAnalyzer` in FSAC, but returns different message than `dotnet/fsharp`
    ('This qualifier is redundant' vs. 'Name can be simplified').
  CodeFix name and title reflect that difference ('Remove redundant qualifier' vs. 'Simplify name `xxx`')
  -> Might be reasonable to align, but for now: Is kept the way it is

Add tests for `RedundantQualifier`
-> ~ align with `dotnet/fsharp` (VS) (`ChangeRefCellDerefToNotExpression`)

Add tests for `ChangeRefCellDerefToNot`
Note: `RemoveReturnOrYield` in `dotnet/fsharp` (VS)
* prev: always check: cursor on ref expression that's accessed with `!`
* now: trigger on diag `3370`
  -> simpler (and most likely faster)
* But change in behaviour:
  * prev: trigger on ref expression
  * now: trigger on bang (`!` -> location of diag)

  Example:
  ```fsharp
  let fr a = ref a
  let v = !(fr 5)
  //       ^^^^^^ prev trigger range
  //      ^ new trigger range
  ```
  I think that's ok: bang gets replaced -> trigger ON bang

Rename `ReplaceBangWithValueFunction` to `ChangeDerefBangToValue`
  * combination of old name and `dotnet/fsharp` (VS) (`ChangeDerefToValueRefactoring`)

Add tests for `ChangeDerefBangToValue`
in `dotnet/fsharp` (VS): `ChangeToUpcast`

Add tests for `ChangeDowncastToUpcast`
`let x = Min(1,2)` (NO newline at end) -> Cursor on `M` -> `ResolveNamespace` throws exception.
Also prevents other CodeFixes from running (for example: above doesn't produce Fix 'Replace with min')

No other tests:
* Tests for `ResolveNamespace` are a bit complex because of nested modules (-> where to `open`)
* Additional: Best to unify with `AutoOpen` (Tests in `CompletionTests.fs` -> `autoOpenTests`).
  But `ResolveNamespace` and `AutoOpen` currently use different implementations and `open` in different locations.
  -> postpone unification and tests
…s in additional double-backticks (-> 4 backticks)

Rename `SuggestedIdentifier` to `ReplaceWithSuggestion`
-> align with `dotnet/fsharp` (VS)

Note: `dotnet/fsharp` checks for Diags `"FS0039"; "FS1129"; "FS0495"`,
       while FSAC checks for `"Maybe you want one of the following:"`.
       (Test for message is kept: is more general)

Add tests for `ReplaceWithSuggestion`
Rename `UnusedOpens` to `RemoveUnusedOpens`
-> align with `dotnet/fsharp` (VS)

Add `isFirstLine`, `isLastLine`, `afterLastCharacterPosition` helper functions for `SourceText`
Note: `SourceText` treats empty string (`""`) as no source.
      `SourceText.WithEmptyHandling` contains functions that consider empty string (`""`) as valid line.

Add `tryEndOfPrevLine`, `tryStartOfNextLine`, `rangeToDeleteFullLine` helper functions for CodeFixes

Add tests for `tryEndOfPrevLine`, `tryStartOfNextLine`, `rangeToDeleteFullLine`
-> align with `dotnet/fsharp` (VS)
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.

I've got over it commit by commit (thank you for the nice separation, by the way!) and this is another wonderful chunk of work. Thank you for taking on this task! It's incredibly appreciated.

I have two small notes where we should be able to remove some of our patched/ported FCS functions now that we've updated, if you wouldn't mind doing that now, since we have the safety net of your new tests.

@baronfel
Copy link
Contributor

@Booksbaum anything else you want to get into this? Otherwise I'll merge.

@Booksbaum
Copy link
Contributor Author

No, all good

@baronfel baronfel merged commit a896858 into ionide:main Apr 11, 2022
@Booksbaum Booksbaum deleted the CodeFixTests branch April 11, 2022 18:20
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