fantomas icon indicating copy to clipboard operation
fantomas copied to clipboard

String literals are not escaped properly when no source code is provided for formatting

Open brianberns opened this issue 4 years ago • 7 comments

Description

When None is passed as the source to CodeFormatter.FormatASTAsync, the resulting formatted F# code contains incorrectly escaped string literals.

Reproduction

Unit test:

let myFormatSourceString isFsiFile (s: string) config =

    let fileName =
        if isFsiFile then
            "/src.fsi"
        else
            "/src.fsx"

    let parsingOptions =
        { FSharp.Compiler.SourceCodeServices.FSharpParsingOptions.Default with
            SourceFiles = [| fileName |] }

    async {
        let! pairs =
            Fantomas.CodeFormatter.ParseAsync(
                fileName,
                Fantomas.SourceOrigin.SourceString s,
                parsingOptions,
                sharedChecker.Value)
        let ast, _ = Array.exactlyOne pairs
        let! formatted =
            Fantomas.CodeFormatter.FormatASTAsync(
                ast,
                fileName,
                List.empty,
                None,    // *** was: Some (SourceOrigin.SourceString s)
                config)

        return formatted.Replace("\r\n", "\n")
    }

    |> Async.RunSynchronously

[<Test>]
let ``string literals`` () =
    myFormatSourceString
        false
        """
let str2 = "c:\\test"
    """
        ({ config with
               MaxValueBindingWidth = 60 })
    |> prepend newline
    |> should
        equal
        """
let str2 = "c:\\test"
"""

Result:

  Message: 
      Expected string length 23 but was 22. Strings differ at index 16.
      Expected: "\nlet str2 = "c:\\\\test"\n"
      But was:  "\nlet str2 = "c:\\test"\n"
      -----------------------------^

Cause

I tracked the problem down to the logic in CodePrinter.fs that handles SynConst.String. When no trivia are present, the code successfully escapes double-quote characters, but does not escape backslash characters.

Suggested fix

Add the following line, or the equivalent:

let escaped = Regex.Replace(escaped, @"\\", @"\\")

See this StackOverflow question for details.

brianberns avatar Mar 26 '21 20:03 brianberns

Just to be sure, what is your use-case for using CodeFormatter.FormatASTAsync? The next version of FCS will allow more information to be stored in AST, so this would be easy to once that lands.

See https://github.com/dotnet/fsharp/blob/dbb8d47059ac1459c00b1ea84e6e2dbcde4b16a9/src/fsharp/SyntaxTree.fsi#L69-L74

Duplicate of https://github.com/fsprojects/fantomas/issues/560

nojaf avatar Mar 26 '21 20:03 nojaf

Thanks for the quick reply. I'm working on a C#-to-F# translator, so I'm building an F# AST from a C# AST, and then sending that F# AST through Fantomas. There's no F# source code to send into CodeFormatter or parse with the F# compiler.

brianberns avatar Mar 26 '21 20:03 brianberns

Thanks for the details on your use-case. In this case, you also want to set StrictMode = true. Sample

The unit test you want in this case is

[<Test>]
let ``escape slash in string using strict mode, 1546`` () =
    formatSourceString
        false
        """
let str2 = "c:\\test"
"""
        { config with StrictMode = true }
    |> prepend newline
    |> should
        equal
        """
let str2 = "c:\\test"
"""

in SynConstTests.

I would accept a PR with your suggested fix. Are you interested? If so, please read our Contribution Guidelines.

nojaf avatar Mar 27 '21 08:03 nojaf

Sure, I'll submit a PR. Thanks for the suggestion to use StrictMode.

brianberns avatar Mar 27 '21 15:03 brianberns

Unfortunately, my proposed fix breaks unit test quotes should be escaped in strict mode, because it incorrectly attempts to escape the backslash in \". I think someone with better regex skills than me will have to take a look at this problem.

  Message: 
    System.Exception : The formatted result is not valid F# code or contains warnings
    let formatter = sprintf "%i,\\"%s\\""

brianberns avatar Mar 29 '21 15:03 brianberns

Hmm, would it be easier perhaps to transforms the string in your tool so that is it double escaped there?

nojaf avatar Mar 30 '21 08:03 nojaf

Yes, I can work around the problem in my code for now.

brianberns avatar Mar 30 '21 12:03 brianberns

I'm going to close this, feel free to open a new issue if you think there is any action left here.

nojaf avatar Dec 26 '22 09:12 nojaf