Skip to content
This repository has been archived by the owner on Oct 31, 2021. It is now read-only.

[Rename Refactoring] Rename doesn't work correctly with Active Patterns #77

Closed
dungpa opened this issue Feb 28, 2014 · 6 comments · Fixed by #111
Closed

[Rename Refactoring] Rename doesn't work correctly with Active Patterns #77

dungpa opened this issue Feb 28, 2014 · 6 comments · Fixed by #111

Comments

@dungpa
Copy link
Contributor

dungpa commented Feb 28, 2014

Example:

let (|Float|_|) (str: string) =
   let mutable floatvalue = 0.0
   if System.Double.TryParse(str, &floatvalue) then Some(floatvalue)
   else None

let parseNumeric str =
   match str with
   | Integer i -> printfn "%d : Integer" i
   | Float f -> printfn "%f : Floating point" f
   | _ -> printfn "%s : Not matched." str

Renaming the use of Float to Float2 resulting:

let (|Float2) (str: string) =
     let mutable floatvalue = 0.0
     if System.Double.TryParse(str, &floatvalue) then Some(floatvalue)
     else None

that doesn't compile.

Additionally:

  • Rename dialog doesn't prevent me to use lowercase identifiders (e.g. float2)
  • Users CAN'T rename definition of Float in (|Float|_|)

This might include a bug in FSharp.Compiler.Service which should be reported upstream.

@dungpa dungpa added the bug label Feb 28, 2014
@ghost
Copy link

ghost commented Feb 28, 2014

Yes, again this is an FCS bug/limitation. Please report upstream, and ideally add a failing test case to FCS

@ghost
Copy link

ghost commented Mar 5, 2014

The underlying problem is fixed in FCS 0.0.26.

However your mileage may vary, particularly when renaming within an active pattern definition let (|AAA|BBB|) x = ..., as it depends on how your identifier-cracking works - does it treat a cursor location of AAA as the idenitifier AAA or the compund identifier representing the overall function.

Renaming at other locations should work OK

@dungpa
Copy link
Contributor Author

dungpa commented Mar 5, 2014

Have you tried the failing tests in fsharp/fsharp-compiler-docs#62? It's the way I fancy it (renaming only individual identifiers).

@ghost
Copy link

ghost commented Mar 5, 2014

Yes, the added tests were great ad pass (not the exception ones). A modified copy of them was included.

@dungpa
Copy link
Contributor Author

dungpa commented Mar 5, 2014

@dsyme Is there any easy to check whether a symbol is active pattern? https://github.com/fsharp/FSharp.Compiler.Service/blob/master/src/fsharp/vs/Typed.fs#L758 doesn't seem to help.

@ghost
Copy link

ghost commented Mar 5, 2014

That property should work for an active pattern function (|A|B|) (i.e. the symbol for the active pattern function)

But I think you want to test whether a symbol is an active pattern label (like A)? I don't think there is a way to do that, at least just yet. We probably need a subtype of FSharpSymbol called FSharpActivePatternLabel - it's somewhat of a special case.

Also there is no way to retrieve the FSharpActivePatternLabels for a active pattern function. We should add that. Add an FCS issue, thanks

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

Successfully merging a pull request may close this issue.

1 participant