[WIP] Add flag to the %A printf specifier that escapes control characters #2558

saul · 2017-03-08T22:05:01Z

This PR adds a flag to the %A format specifier: @. This flag escapes control characters and other characters that need to be escaped in F# source code string literals.

Before

After

This allows us to directly copy strings that have been formatted with %@A straight into code - and they'll 'just work'.

Remaining work:

Print strings with %@A as verbatim strings if they are 'path like' (i.e only contain backslashes as their escaped characters)
Write test for %+A and %@A (and a combination of the two)
Write test showing that FSI outputs the 'it' variable as %@A by default
Fix current tests

smoothdeveloper · 2017-03-08T22:19:58Z

It makes a lot of sense :)

Can we get an idea if performance changed / improved on large strings?

I'm seeing we had List.rev but were using indexer on the string rather than Seq.cast.

saul · 2017-03-08T22:21:15Z

Neither check nor conv were used in the old implementation. This implementation will definitely be slower as it is actually doing something, whether this is something we care about I'll leave that for @dsyme to decide.

saul · 2017-03-08T22:25:38Z

Locally (uncommitted) I'm trying out formatting "path like" strings (no escaped characters other than backslashes) as verbatim strings. I'd say the vast majority of times that I've seen verbatim strings, it's for paths (in C# and F#):

smoothdeveloper · 2017-03-08T23:02:34Z

@saul I see that the code is not printing the whole string, but is it correct that formatString can be used in other parts?

I've made another implementation of the function:

  let formatString2 (s:string) =
      let builder = System.Text.StringBuilder(s.Length)
      builder.Append("\"") |> ignore
      for c in s do
          builder.Append(formatChar false c) |> ignore
      builder.Append("\"") |> ignore
      string builder

and it seems to run lot faster (tried on a 4mb xml file):

I think using StringBuilder is sensible here.

saul · 2017-03-08T23:11:10Z

That's a shame - I was hoping that writing it in idiomatic F# would be performant enough. I'll try your 'large string' benchmark tomorrow.

Thanks for looking into this @smoothdeveloper

smoothdeveloper · 2017-03-08T23:23:59Z

@saul I'm sure you can turn my code into idiomatic F#, Seq.iter and making local function for builder.Append, we will have best of both worlds :)

dsyme · 2017-03-09T11:58:10Z

My suggestion is that we should use %@A or some other new qualifier to activate this, since otherwise it will likely be a breaking change.

KevinRansom · 2017-03-09T15:40:31Z

I thought that I had already commented that this would be a breaking change, lots of code uses %A to output readable text, and undoubtedly works around or perhaps relies on the current quirky behavior, so an alternate formatting char would be useful.

saul · 2017-03-09T20:47:26Z

@dsyme @KevinRansom What about if a string is nested within a record? How would we print it with %A then?

Are you suggesting that we keep all string formatting the same unless a different flag has been supplied? How do we do %+A with this new string formatting flag then?

Personally I think it makes more sense if we accept it as a breaking change - if people want the old behaviour it's very easily replicated with sprintf "\"%O\"" s.

I think another flag just adds even more complexity to printf. I did a straw poll of seasoned F# developers at my office today and about 10% of them knew what %+A did. I think the discoverability of flags is shoddy and it would be best if we were correct by default.

KevinRansom · 2017-03-09T21:11:16Z

@saul

FSharp has many thousands of active customers, a breaking change in this will affect a portion of them in some way. The pain addressed by this PR is probably insufficient to justify a language breaking change. Certainly if C# had this feature there is no way it would get past the breaking change committee. I have seen code-reviews were ToString() modifications were rejected. I'm sure that we can figure out a non-breaking way of adding the type of formatting you are suggesting.

saul · 2017-03-09T21:34:17Z

Thanks @KevinRansom, that's fine - I'll implement it as a '@' flag for now and update this PR in due course :)

Also @smoothdeveloper I did some perf tests today, and here are the results:

$ fsi formatString.fsx --optimize+
52 msecs (imperative for loop with string builder)
51 msecs (imperative for loop with string builder (double capacity))
53 msecs (String.iter (piped) with string builder (double capacity))
52 msecs (String.iter (composition) with string builder (double capacity))
53 msecs (String.iter (composition) with string builder (quad capacity))
51 msecs (String.iter (composition) with string builder (double capacity, string constructor))
188 msecs (functional cast, map and String.concat)
74 msecs (functional cast, map and String.Join)

This is the average of 25 runs of each benchmark, parsing a .dll file and a .json file. The first 6 are very similar, with each run they tend to swap numbers. I think I'll go with the String.iter solution and update this PR.

saul · 2017-03-09T22:23:28Z

Latest commit:

Current behaviour:

FSI prints 'it' with strings escaped
%A works as before
%@A dictates that a string should be escaped

I'm going to mark this WIP as I still have some work to do (will update original post).

cartermp · 2017-03-10T05:12:50Z

👍, I like it with %@A since it's consistent with the @ string literals. Do we think we'll need a quick RFC for this? I think it's a small enough feature that it doesn't necessarily need a new RFC, but it would be good to track somewhere so that we can go back around F# 4.2 timeframe and list it as a new thing in the FSharp.Core.

dsyme · 2017-03-10T14:43:30Z

src/fsharp/FSharp.Core/printf.fs

-                let o =
+                Microsoft.FSharp.Text.StructuredPrintfImpl.FormatOptions.Default
+                |> fun o ->
                    if useZeroWidth then { o with PrintWidth = 0} 


Just use let bindings - that's normal style in FSharp.Core. If you like give all the o different names.

dsyme · 2017-03-10T14:46:36Z

src/utils/sformat.fs

            fieldInfo.GetValue(obj)

        let formatChar isChar c = 
            match c with 


I'm not sure but perhaps we should be escaping some entire classes of Unicode characters as well. See https://en.wikipedia.org/wiki/Template:General_Category_(Unicode). Though I suppose it's not strictly necessary - the rule is that F# should be able to parse what it prints.

Am I right in saying that F# files are parsed as UTF-8? In which case I don't think we need to escape anything else other than what's there already.

The relevant part of the spec will be the part on strings - and yes, I believe the ones you've got are the only characters that must be escaped. Even some of those need not be - like \t in strings - but I think you're aiming for a string that looks "good" rather than full of strange characters.

dsyme · 2017-03-10T14:47:41Z

src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Core/PrintfTests.fs

+
+    [<Test>]
+    member __.``Standard characters are correctly escaped``() =
+        test "%A" "\n" "\"\\n\""


These tests will need updating. Also cover +@A and @+A etc.

saul · 2017-03-10T19:42:13Z

@dsyme See my latest commit - how do I add the new SprintfAFormatSpecifiers.fs and FsiEscapesStringsByDefault.fs QA tests to the fsharpqa suite?

@cartermp Where should I raise the RFC? Do you want me to do it in https://github.com/fsharp/fslang-suggestions?

KevinRansom · 2017-03-11T04:56:01Z

Add the test cases to this file: visualfsharp/tests/fsharpqa/Source/Printing/env.lst

Something like:

	SOURCE=SprintfAFormatSpecifiers.fs			# SprintfAFormatSpecifiers.fs
	SOURCE=FsiEscapesStringsByDefault.fs			# FsiEscapesStringsByDefault.fs

Kevin

saul · 2017-03-11T12:18:15Z

Thanks @KevinRansom, I'll make that change when I'm back.

The more pressing issue is that the tests don't compile as they're being compiled by F# 4.1 which doesn't support the @ flag. Do I need to make these QA tests instead of unit tests too?

cartermp · 2017-03-11T16:44:26Z

@saul I think just an issue here to track the work + state the basic design of the format string is good enough.

Edit: Yup, RFC

dsyme · 2017-03-11T17:27:45Z

The more pressing issue is that the tests don't compile as they're being compiled by F# 4.1 which doesn't support the @ flag. Do I need to make these QA tests instead of unit tests too?

That's odd. Have you run build.cmd proto on your machine to update the Proto compiler? Or is this on CI (which rebuilds the proto compiler automatically)?

KevinRansom · 2017-03-11T17:31:20Z

@dsyme I will take a look, right now.
The tests should be built with the compiler made by the build not the proto-compiler.

dsyme · 2017-03-11T17:38:15Z

@cartermp Where should I raise the RFC? Do you want me to do it in https://github.com/fsharp/fslang-suggestions?

Yes, add the RFC there, thanks!

KevinRansom · 2017-03-11T18:13:42Z

Sadly ci_part1 works fine on my machine .... doh!!! that was sauls master branch!!! trying again with the real deal.

KevinRansom · 2017-03-11T20:09:28Z

When building FSharp.Core.dll for the portable profiles we get this:

  C:\Users\kevinr.REDMOND\AppData\Local\Temp\.NETPortable,Version=v4.5,Profile=Profile7.AssemblyAttributes.fs


c:\saul\visualfsharp\src\utils\sformat.fs(940,71): error FS0001: This expression was expected to have type�    'char'    �but here has type�
    'obj' [c:\saul\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj]

c:\saul\visualfsharp\src\utils\sformat.fs(944,45): error FS0001: This expression was expected to have type�    'obj'    �but here has type�
   'char' [c:\saul\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj]
Done Building Project "c:\saul\visualfsharp\src\fsharp\FSharp.Core\FSharp.Core.fsproj" (Build target(s)) -- FAILED.

Done Building Project "c:\saul\visualfsharp\build-everything.proj" (default targets) -- FAILED.


Build FAILED.

I think there may be an incompatability in the portable profile that causes us to prefer IEnumerable, rather than IEnumerable<char>

Yep!!! profile 7 version of string does not support IEnumerable<char> .... wow!!!!!
extracted using reflector from profile7 System.Runtime

.class public auto ansi sealed beforefieldinit String
    extends System.Object
    implements System.Collections.IEnumerable, System.IComparable, System.IComparable`1<string>, System.IEquatable`1<string>

extracted using reflector from net45 mscorlib

.class public auto ansi serializable sealed beforefieldinit String
    extends System.Object
    implements System.IComparable, System.ICloneable, System.IConvertible, System.IComparable`1<string>, System.Collections.Generic.IEnumerable`1<char>, System.Collections.IEnumerable, System.IEquatable`1<string>
{

Just testing the fix, I'll send a PR when it's done.

KevinRansom · 2017-03-11T20:27:19Z

Now it looks like a regression here:

Compiled with built compiler:

c:\saul\visualfsharp\tests\fsharpqa\Source\Printing>\saul\visualfsharp\release\net40\bin\fsc WidthForAFormatter.fs
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

WidthForAFormatter.fs(9,13): error FS0741: Unable to parse format string ''A' format does not support precision'

c:\saul\visualfsharp\tests\fsharpqa\Source\Printing>uedit64 WidthForAFormatter.fs

Compiled with released compiler:

c:\saul\visualfsharp\tests\fsharpqa\Source\Printing>fsc WidthForAFormatter.fs
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

c:\saul\visualfsharp\tests\fsharpqa\Source\Printing>

I have pushed the portable7 fix to your fork.

I will let you take a look at the regression.

Kevin

saul · 2017-03-12T21:23:51Z

Thanks @KevinRansom - I've committed a fix for that regression and added the new test to the suite.

If the tests go green then it's ready.

KevinRansom · 2017-03-14T16:40:31Z

@saul a couple more test failures that look they are related to the change.

+++ Conformance\ObjectOrientedTypeDefinitions\InterfaceTypes (E_InheritInterface.fs) +++

*** The following necessary lines were never matched:
***	\(10,21-10,22\):.+error FS1207:.+Interfaces inherited by other interfaces should be declared using 'inherit \.\.\.' instead of 'interface \.\.\.'$


*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output 
FAIL

+++ Printing (FsiEscapesStringsByDefault.fs) +++
Microsoft (R) F# Compiler version 4.1
Copyright (c) Microsoft Corporation. All Rights Reserved.

*** The following necessary lines were never matched:
***	val s : string = "Complex\n\"string"


*** The following necessary lines were incorrectly matched:

Unexpected Compiler Output 
FAIL

saul · 2017-03-14T21:08:40Z

Thanks @KevinRansom - I believe I know what the second error is, but I'm afraid I've no idea how I could have caused the first one.

I'll get around to fixing the latter ASAP!

KevinRansom · 2017-05-22T23:59:53Z

@dotnet-bot test this please

saul · 2017-08-23T21:36:24Z

@dotnet-bot test this please

KevinRansom · 2017-10-30T17:57:27Z

@dotnet-bot test this please

cartermp · 2018-02-05T01:19:34Z

@KevinRansom Now that we don't build PCLs anymore, are the concerns there no longer valid?

Also conflicts.

KevinRansom · 2018-09-11T21:07:54Z

@saul do you think we should still pursue this PR?

The conflict is easy to resolve PrintfTests.fs has moved from below src to below test.

KevinRansom · 2018-09-26T17:57:50Z

@saul,

please reopen this PR when you are ready to pursue it further.

Kevin

saul added 6 commits December 23, 2016 11:34

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

4c2e98a

Merge branch 'master' of https://github.com/Microsoft/visualfsharp

9b1a4e9

Merge remote-tracking branch 'Microsoft/master'

b6f757c

Merge remote-tracking branch 'Microsoft/master'

3b6a01c

Merge remote-tracking branch 'Microsoft/master'

6245412

Escape control characters when %A printing strings

54c1164

msftclas added the cla-already-signed label Mar 8, 2017

saul mentioned this pull request Mar 9, 2017

String.concat casts to an array before concatenating #2569

Closed

Only escape strings if '@' is supplied as a flag to %A.

3afb2f8

saul changed the title ~~Escape control characters when %A printf'ing strings~~ Add flag to the %A printf specifier that escapes control characters Mar 9, 2017

saul changed the title ~~Add flag to the %A printf specifier that escapes control characters~~ [WIP] Add flag to the %A printf specifier that escapes control characters Mar 9, 2017

dsyme reviewed Mar 10, 2017

View reviewed changes

Support verbatim 'path like' strings, add more tests

d83358f

Add more tests

d1d3a25

fix portable7 build

649fe9b

Add test for FSI default output

2fec30f

saul mentioned this pull request Mar 12, 2017

Add new format flag to the %A specifier which escapes necessary string characters fsharp/fslang-suggestions#551

Open

6 tasks

Merge branch 'master' into escape-percent-a-strings

decda27

msftgits removed the cla-already-signed label Sep 26, 2017

KevinRansom closed this Sep 26, 2018

OwnageIsMagic mentioned this pull request Oct 1, 2023

Do not use Unicode aware API and CurrentCulture in compiler #16066

Closed

[WIP] Add flag to the %A printf specifier that escapes control characters #2558

[WIP] Add flag to the %A printf specifier that escapes control characters #2558

Uh oh!

Conversation

saul commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Remaining work:

Uh oh!

smoothdeveloper commented Mar 8, 2017

Uh oh!

saul commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saul commented Mar 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smoothdeveloper commented Mar 8, 2017

Uh oh!

saul commented Mar 8, 2017

Uh oh!

smoothdeveloper commented Mar 8, 2017

Uh oh!

dsyme commented Mar 9, 2017

Uh oh!

KevinRansom commented Mar 9, 2017

Uh oh!

saul commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Mar 9, 2017

Uh oh!

saul commented Mar 9, 2017

Uh oh!

saul commented Mar 9, 2017

Uh oh!

cartermp commented Mar 10, 2017

Uh oh!

dsyme Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

dsyme Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

saul Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

dsyme Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme Mar 10, 2017

Choose a reason for hiding this comment

Uh oh!

saul commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Mar 11, 2017

Uh oh!

saul commented Mar 11, 2017

Uh oh!

cartermp commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Mar 11, 2017

Uh oh!

KevinRansom commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme commented Mar 11, 2017

Uh oh!

KevinRansom commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Mar 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KevinRansom commented Mar 11, 2017

Uh oh!

saul commented Mar 8, 2017 •

edited

Loading

saul commented Mar 8, 2017 •

edited

Loading

saul commented Mar 8, 2017 •

edited

Loading

saul commented Mar 9, 2017 •

edited

Loading

dsyme Mar 10, 2017 •

edited

Loading

saul commented Mar 10, 2017 •

edited

Loading

cartermp commented Mar 11, 2017 •

edited

Loading

KevinRansom commented Mar 11, 2017 •

edited

Loading

KevinRansom commented Mar 11, 2017 •

edited

Loading

KevinRansom commented Mar 11, 2017 •

edited

Loading