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

Collection of findings related to string interpolation #9893

Closed
22 of 34 tasks
abelbraaksma opened this issue Aug 7, 2020 · 26 comments
Closed
22 of 34 tasks

Collection of findings related to string interpolation #9893

abelbraaksma opened this issue Aug 7, 2020 · 26 comments
Labels
Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Aug 7, 2020

EDIT: if you want to test this awesome feature yourself (or any other "latest" feature), I've written up (and today, 9 Aug, corrected) a little instruction here: #9893 (comment)

Just some findings I collect here about the upcoming string interpolation feature. Some may be "by design", or may already have been reported, but I offered @cartermp to do some testing, so here it goes ;).

For: #8907 (I used the latest successful build of the PR, of earlier today). Details of feature in RFC: https://github.com/fsharp/fslang-design/blob/master/preview/FS-1001-StringInterpolation.md.

Some additional notes (but not bugs afaict) in this comment by @cartermp: #8907 (comment)

Summary and todo-list

  • Half of the expressions are not evaluated [fixed&tested]
  • Non-escaped closing curly considered legal [fixed&tested]
  • Empty expr between curlies
  • Prefix operators don't work on interp strings
  • Colors of curlies in tooling
  • Expression past closing quote considered part of the expression, making comments part of the interp string
  • Wrong error range for missing closing curly, or extra open curly-at-end
  • With combinator for Printf.StringFormat, errors can be all over the place
  • Use with literal gives too many errors
  • Really weird behavior when (wrongly) using compiler directives + multiline expr in interp strings
  • Multiline woes: wrong error-underline calculation
  • Multiline woes: coloring is off, part of string default color
  • Multiline woes: mysterious errors that are not an error when compiled
  • Potential error improvements

Summary of things tested so far (8/8/2020)

  • Multiline strings with continuation character (bugs found, see above)
  • #if statement in multiline expressions just work as expected
  • # lineno statements just work as expected inside interp expr
  • #compdirective, when invalid, wreaks havoc, much more than it used to (see "weird behavior" issue above)
  • breakpoints in multiline expressions just work and tooltips show correct values
  • Coloring and ranges tested, some findings, see above
  • Combinators and other functions working on StringFormat tested, some findings, see above
  • Attempted to break the "no nested interp strings unless single-inside-triple", works as expected
  • Escape sequences in strings, works as expected
  • Non-closing curlies and other bad combinations, works as expected but some issues with tooling (see above)
  • Quotations (see @cartermp reference above), works as expected
  • Go-to-definition, works
  • Nested curlied expressions (records, anonymous records, seq) work as expected (require space between curlies)
  • Type inference works as expected ($"%i{x} requires x to be integer)
  • Mixing %-style and interp-style disallowed, this is as expected
  • Interaction with kprintf works
  • Interaction with fully-typed StringFormat does not work with interp strings, I guess this is as expected:
    let log msg a = sprintf msg a
    log "%d" 24   // fine
    let f x = log "{x}"   // error, wrong arity
  • Outdent leniency of nested expr: not supported. Perhaps this can be added? (not a bug, though).
    // warning on wrong indentation:
    $"Some interpolated string {
        let x = 12
        let y = 14
        x + y }"
    Can be fixed by outdenting very far:
    $"Some interpolated string {
                                let x = 12
                                let y = 13
                                x + y }"
  • Use with literals: some issues, see above, but works in general.
  • Use of invalid keywords in expr like module and type give the proper errors.

Half of the expressions are not evaluated fixed/tested

It turns out that there's an issue with the third and subsequent string interpolation section in a composed string, leading to weird results (note how the "." get misplaced). It appears thatonly noOfExpr idiv 2 + 1 get evaluated:

// Missing data, half the expressions are not evaluated, or in the wrong position:
printfn $"""{1}{2}{3}"""                  // prints "12"
printfn $"""{1}{2}{3}{4}{5}{6}"""         // prints "123"
printfn $"""{1}{2}{3}{4}{5}{6}{7}"""      // prints "1234"
printfn $"""{$"{1}"}{$"{2}"}{$"{3}"}"""   // prints "12"
printfn $"""{1},{2}{3}{4}.{5}"""          // prints "1,23.4"
Console.WriteLine $"""{1}{2}{3}"""           // prints "12"
Console.WriteLine $"""{1},{2}{3}{4}.{5}"""   // prints "1,23.4"

Non-escaped closing curly considered legal fixed/tested

The following came as a surprise to me, but may be based on how C# does things? This came as a surprise, I'd expect orthogonality here:

// inconsistency with escaping: "{" must be escaped as "{{", but "}" can be either literal, or escaped as "}}"
// Is this by accident? It makes more sense to always require escaping "{" AND "}", which means this is invalid:
printfn $"{1}}"    // should be invalid, but gives "1}"
printfn $"{1}}}"   // is valid and correctly gives "1}"

Empty expr between curlies

I'd prefer this to be legal, esp in light of autogen tools, that now have to put in null or None to eval to nothing, or remove the expr part:

let x = $"{}"  // error

Prefix operators don't work on interp strings

It's (quite) customary to (re)define unary prefix operators and they used to work "just fine" with strings, but they don't work the same way with interpolated strings, because $ is considered part of the prefix operator. Since it is already forbidden to have an operator contain the dollar sign (except for dollar-only operators), I think this should be legal:

image

let (!) x = x
printfn !$"{42}"   // workaround: use parens

Colors of curlies in tooling

Maybe we can distinguish colors of active { vs escaped {? It's pretty hard to notice the difference. Maybe we can make the active ones bright-red or something?

Expression past closing quote considered part of the expression, making comments part of the interp string

This gives "this value is not a function an cannot be applied" error, and wrongly colors the rest of the line:

printfn $"{1}{"   // gives wrong error "1}"

image

This same behavior leads to other weird stuff, where the syntax checker keeps looking beyond the last quote:

image

Or this one, each line getting red to the end of file:

image

Wrong error range for missing closing curly, or extra open curly-at-end

Like this:

image

With combinator for Printf.StringFormat, errors can be all over the place

(it's actually awesome that this works mostly flawlessly and that I can re-use existing stringformat combinators!)
The following code gets errors all over the place in the whole file:

/// indent each line by 4 spaces
let inline (<|>) a b = Printf.kprintf (sprintf "%s\n    %s" a) b

let f x =                  // wrong error on let, the string is closed, should not happen here
    sprintf ""
            <|> "Line one is twelve: %d" <| 12
            <|> $"Line two is twentyfour: {24}"
            <|> $"{1}{2}{3"      // error should be only here and coloring should not be gone...

f 1 |> printfn "%s"        // wrong error on interp string, this is NOT an interp string and not an error
exit 0
// wrong error at end-of-file for unfinished let, which is untrue, the string has a closing quote

See the 4 (!) errors in this animation, it should ideally be only one on wrong interp string:

8020843a-8db8-4fba-aec6-0576f30549cc

Use with literal gives too many errors

I can understand that use with literals is not allowed, not even when it can be assessed that the result of an interp string will be constant. But there are two errors raised, instead of one:

image

Really weird behavior when (wrongly) using compiler directives + multiline expr in interp strings

I've no idea what happens here, there seems to be nothing consistent. Everything gets green and improper errors pop up. The red # is as it is now (unexpected symbol in binding), the rest getting green not at all. Notice that this behavior disappears when the interpolated expression part is a single constant.

Repo code:

let x = 1 in printfn "\4\{42}%d{x}" x

/// indent each line by 4 spaces
let inline (<|>) a b = Printf.kprintf (sprintf "%s\n    %s" a) b
let log msg a = sprintf msg a

let x = log "%d" 12
//let x = log $"%O{x}"  // not possible


let f x =
    #nowarn "12"   // any wrong compiler directive makes everything green and shows improper errors
    sprintf $"ML str {
                        let x = 12    // replace this multiline with single expr and the green and wrong errors disappear
                        x }"

4b3d8078-3db2-4241-b11d-ffbb21656148

Multiline woes, wrong error-underline calculation

let f configFile =
    sprintf $"This \
               Is \
                    A \
                            Multiline string error '{configFile}' %s"

image

Multiline woes, coloring is off, part of string default color

image

Multiline woes: mysterious errors that are not an error when compiled

This situation appears to depend on the size of the input (and is not necessarily related to the new feature, I can confirm this was the case previously):

Error is depending on text length in last line, it seems:

4b649c89-ac05-45f6-a603-97dbaf9548d4

Potential error improvements

Here are some areas where we may want to improve error reporting.

  1. Something about hinting like "remove whitespace between '{' and % expression" perhaps?
    image

  2. Text doesn't match the error, should be something like "Unclosed opening bracket in interpolated string found at position X in string":
    image

  3. Not sure how this can be improved, but the user here escaped the curly, but then gets an error that (s)he needs to escape the curly? Maybe we can report all situations where openingCurlies - closingCurlies <> 0 (excluding escapes) as a variant of "missing opening or closing bracket"? This would solve a range of error issues, I think.
    image

This is cool!

I just have to say I really like it that the result inside {...} is not just a string, but keeps type information, and that the result can be formatted. So this is entirely expected (and %f fixes the error):

image

I'll update this list if I find more stuff (it's getting a bit late over here ;) ).

Anybody else who'd like to try out this new feature and iron out some bugs, here's how to use the absolute latest, I've written some instructions in the PR: #9893 (comment)

@dsyme
Copy link
Contributor

dsyme commented Aug 7, 2020

@abelbraaksma Thanks so much for trying this out! I'll get these fixed right now

dsyme added a commit that referenced this issue Aug 7, 2020
@dsyme
Copy link
Contributor

dsyme commented Aug 7, 2020

It turns out that there's an issue with the third and subsequent string interpolation section in a composed string, leading to weird results (note how the "." get misplaced). It appears thatonly noOfExpr idiv 2 + 1 get evaluated:

Thanks, this bug hits when the specifiers are next to each other (which wasn't covered in our test suite).

I've fixed it now in the feature branch

@dsyme
Copy link
Contributor

dsyme commented Aug 7, 2020

printfn $"{1}}" // should be invalid, but gives "1}"

I agree this should be invalid, thanks

@dsyme
Copy link
Contributor

dsyme commented Aug 7, 2020

The first two problems are fixed in feature/string-interp

Maybe we can distinguish colors of active { vs escaped {? It's pretty hard to notice the difference. Maybe we can make the active ones bright-red or something?

@cartermp would like this too, I think it's reasonable.

@cartermp
Copy link
Contributor

cartermp commented Aug 7, 2020

Interesting, Rider and ReSharper highlight the "holes" in interpolated strings and string.Format calls differently:

  • interpolation holes ({ and }) are plaintext
  • string.format holes {0}, {1}, etc. are green

So that's some interesting prior art

@abelbraaksma
Copy link
Contributor Author

Glad it was helpful and could be fixed easily 👍. But this got auto-closed due to the merge, with parts of the issue still open/unaddressed. Shall I post the rest again separately, or can you (@KevinRansom, @dsyme) reopen this? (I don't have enough rights to do that).

I assume subsequent fixes will go to master and the feature branch gets removed?

@KevinRansom
Copy link
Member

@abelbraaksma , issues will be addressed as bugs in the normal way.

@KevinRansom KevinRansom reopened this Aug 7, 2020
@cartermp
Copy link
Contributor

cartermp commented Aug 7, 2020

@abelbraaksma can you go through and add a checklist item for each thing you noticed, marking the ones that are addressed?

@abelbraaksma
Copy link
Contributor Author

@cartermp, done, I gave each bug a sensible (sub)title and on top there's a simple checklist of each of them.

baronfel pushed a commit to baronfel/FSharp.Compiler.Service that referenced this issue Aug 7, 2020
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f396177996a7d63e1ce650423993f07facd8239.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet/fsharp#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (#9516) (#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (#9576) (#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (#9576) (#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (#9644) (#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (#9657) (#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (#9657) (#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (#9839) (#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet/fsharp#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 7, 2020

@dsyme, I've added a bunch of issues in the last hour or so. Nothing egregious, I think.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 8, 2020

I've also added a list of "Summary of things tested so far", so that others don't have to re-asses those areas. I'm sure there are gaps there, please advice if there's an area that needs extra attention (@dsyme, @cartermp).

@zanaptak
Copy link
Contributor

zanaptak commented Aug 9, 2020

I tried @abelbraaksma's directions and got FS3353 saying I need language version preview, even though I have <LangVersion>preview</LangVersion> defined. I'll probably just wait for a formal preview for now but will point out I noticed that there are two 3353's in FSComp.txt in case that's an issue:

3353,chkFeatureNotSupportedInLibrary,"Feature '%s' requires the F# library for language version %s or greater."
3353,fsiInvalidDirective,"Invalid directive '#%s %s'"

@charlesroddie
Copy link
Contributor

Nice work @abelbraaksma .

let x = $"{}" // error

This should be an error. Representing an empty string, or "nothing", or null, or None, as a empty space is not allowed in other places in F# (e.g. let x = // error) and would create confusion and irregularity.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 9, 2020

saying I need language version preview, even though I have preview

EDIT: there was an error in my original instruction, I wrote <LanguageVersion>, which should be <LangVersion>, but it appears you got that right, tx ;).

But the error you saw, @zanaptak, is about the reference to the FSharp.Core.dll library. You need to also add <DisableImplicitFSharpCoreReference>true</... and you must explicitly add a reference to the local FSharp.Core.dll build. See next message for a full example of an fsproj file. Could you please try it again?

@charlesroddie, you've got a good point. There's actually a PR right now that's addressing that very thing to get a more meaningful error.

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 9, 2020

@zanaptak, for reference, I'll copy the instructions here as well (and created an issue to smoothen the situation in the future: #9904):

For anybody wanting to play around with the latest build of this feature (esp. those that aren't aware how to do this and are curious where to start):

  • Checkout this feature branch (feature/string-interp)
  • Build by running build.cmd (if you already had a build before switching branches, use git clean -xdf first)
  • Open VisualFSharp.sln, select Release and Build All
  • Select VisualFSharpFull as startup project, hit Ctrl-F5

A new VS instance is started with the new features selected, but when you create a project, it will, by default, reference an FSharp.Core.dll from NuGet. To fix this:

  • Open your test fsproj file and add a line <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference> in the top property group
  • In the same property group, add <LangVersion>preview</LangVersion>
  • OR: In the same property group, add <OtherFlags>--langversion:preview</OtherFlags>
  • Add a standard file assembly reference to [fsharp-checkoutdir]\artifacts\bin\VisualFSharpFull\Release\net472\FSharp.Core.dll

A minimal project file for netcoreapp3.1 could look something like this:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.1</TargetFramework>
    <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference>
    <LangVersion>preview</LangVersion>
    <!-- either the prev. line, or the following line -->
    <OtherFlags>--langversion:preview</OtherFlags>
  </PropertyGroup>

  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|AnyCPU'">
	<WarningsAsErrors>3239;25</WarningsAsErrors>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Program.fs" />
  </ItemGroup>

  <ItemGroup>
    <Reference Include="FSharp.Core">
      <HintPath>D:\Projects\OpenSource\FSharp\artifacts\bin\VisualFSharpFull\Release\net472\FSharp.Core.dll</HintPath>
    </Reference>
  </ItemGroup>
</Project>

Now you can start typing your code without errors along the line of "Feature 'string interpolation' requires the F# library for language version 4.7 or greater." (an error you will also get if your project actually references 4.7.0, but that's a bug for another time, or maybe I just screwed something up locally).

abelbraaksma added a commit to abelbraaksma/fsharp that referenced this issue Aug 9, 2020
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f39617.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (dotnet#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (dotnet#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (dotnet#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (dotnet#9516) (dotnet#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (dotnet#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (dotnet#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (dotnet#9644) (dotnet#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (dotnet#9657) (dotnet#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (dotnet#9657) (dotnet#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839) (dotnet#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
@zanaptak
Copy link
Contributor

zanaptak commented Aug 9, 2020

I already had the disabling and the file reference in place, I'll try the new flag later. Should we use master branch now since it's merged and may get ongoing fixes?

@abelbraaksma
Copy link
Contributor Author

Yes, you can use the master branch. Feel free to ping me on slack if you have trouble, I'm also trying to get a clear picture of the issues with running from a fresh build.

Btw, you do need to run build from a vs 2019 developer command prompt, I think.

@dsyme
Copy link
Contributor

dsyme commented Aug 15, 2020

printfn $"{1}{" // gives wrong error "1}"

I don't get this. This is an interpolated single quote string containing a single quote string applied to 1 as a function

Single quote strings containing a single quote string are not allowed but that is separate.

Adding some spacing may help see what's going on:

printfn $"{1}{   "<---->"   1     }"

@dsyme
Copy link
Contributor

dsyme commented Aug 15, 2020

Thank you so much for your amazing QA work here!!

@dsyme
Copy link
Contributor

dsyme commented Aug 15, 2020

(I'll be reading the rest more closely on Monday)

@abelbraaksma
Copy link
Contributor Author

I don't get this. This is an interpolated single quote string containing a single quote string applied to 1 as a function

Single quote strings containing a single quote string are not allowed but that is separate.

@dsyme I meant to say (see screenshot) that I expect the error to end at the invalid last opening { of the first string.

$" starts the string, the first following, non escaped " closes the string. So the error should be about printfn $"{1}{" only, and complain that there's a non terminated { at the end.

The rest of that line is whitespace and comment (but becomes part of the error).

It appears as if there's a greedy regex match going on, instead of a non greedy one. This may also be because currently, the parser tries to find out whether the user attempts to nest another interpolated string inside it, and is conflicts with the user being half way inside typing a valid expression (he just opened the next curly), coloring the rest of the file as error.

@dsyme
Copy link
Contributor

dsyme commented Aug 24, 2020

I'm first trying to work out if there are must-fix issues here

I think these are by-design:

  • Empty expr between curlies - this was never intended to work.

  • "Expression past closing quote considered part of the expression" - I don't think so, see analysis above, which I believe still holds

  • "Wrong error range for missing closing curly, or extra open curly-at-end" - I think this is similar

  • "With combinator for Printf.StringFormat, errors can be all over the place" - i think this is similar, again 3"... is being treated as a interpolation fill expression where 3 is being applied to a string".... This is just always going to create strange errors. But it is hard to see how they can be easily improved.

These are all potential future improvements?

  • Prefix operators don't work on interp strings: Seems reasonable, could be done later

  • Colors of curlies in tooling: Seems reasonable, could be done later

  • Use with literal gives too many errors

  • Really weird behavior when (wrongly) using compiler directives + multiline expr in interp strings

  • Multiline woes: wrong error-underline calculation

  • Multiline woes: coloring is off, part of string default color

  • Multiline woes: mysterious errors that are not an error when compiled

  • Potential error improvements

@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 24, 2020

These are all potential future improvements?

I would consider some of them bugs, and not just improvements (esp the multiline issues), but fixing a bug is an improvement, isn't it? ;). Though none of them are likely showstoppers, before this gets wildly used, it may be good if we can iron out some of these.

I don't think so, see analysis above, which I believe still holds

@dsyme The problem with these is that $"{SomeExpr}{" (i.e. a valid string holding an opening curly, being invalid interp. string) causes issues with the underlining of the errors. Since it is invalid to have a nested quoted interpolated string in a single-quoted string, the error here is not that the last { starts a new expression with an expression starting with a " (quote), but that the whole expression here is (ignoring escaped quotes) the regex \$"[^]*?", and the error-checker (or recovery) doesn't consider this, instead it greedily runs to the next quote (as if the regex \$"[^]*" were used instead).

Surprising stuff happens when you are just typing, which creates a scary experience while you are in de middle of an interpolated string. In heavy files, the constant "whole file is an error" experience may lead to heavy spinning of VS.

@dsyme dsyme added the Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. label Aug 26, 2020
@cartermp cartermp added this to the Backlog milestone Sep 1, 2020
@HoraceGonzalez
Copy link

HoraceGonzalez commented Sep 16, 2020

@abelbraaksma, reporting this from the slack channel:

There seems to be the following issue with embedding string literals inside interpolated strings. I was able to produce the following error in FSI:

$"{""}" // error FS0010: Unexpected interpolated string (final part) in interaction
$"""{""}""" // works fine

This is my fsi version info:

Microsoft (R) F# Interactive version 11.0.0.0 for F# 5.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

My dotnet info:

.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.8.20417.9
 Commit:    fc62663a35

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  20.04
 OS Platform: Linux
 RID:         ubuntu.20.04-x64
 Base Path:   /usr/share/dotnet/sdk/5.0.100-preview.8.20417.9/

Host (useful for support):
  Version: 5.0.0-preview.8.20407.11
  Commit:  bf456654f9

.NET SDKs installed:
  3.1.201 [/usr/share/dotnet/sdk]
  3.1.301 [/usr/share/dotnet/sdk]
  5.0.100-preview.3.20216.6 [/usr/share/dotnet/sdk]
  5.0.100-preview.7.20366.6 [/usr/share/dotnet/sdk]
  5.0.100-preview.8.20417.9 [/usr/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.1.3 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.5 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0-preview.3.20215.14 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0-preview.7.20365.19 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0-preview.8.20414.8 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.1.3 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.5 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0-preview.3.20214.6 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0-preview.7.20364.11 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0-preview.8.20407.11 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download
-----------------------------------------------

my FSharp.Core version is 4.7.2 per my paket.lock file.

nosami pushed a commit to xamarin/visualfsharp that referenced this issue Feb 23, 2021
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f39617.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (dotnet#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (dotnet#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (dotnet#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (dotnet#9516) (dotnet#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (dotnet#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (dotnet#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (dotnet#9644) (dotnet#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (dotnet#9657) (dotnet#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (dotnet#9657) (dotnet#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839) (dotnet#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
@dsyme
Copy link
Contributor

dsyme commented Aug 30, 2021

I'm going to close out this old issue - many thanks to @abelbraaksma for the very detailed work which was crucial in shipping this feature

@dsyme dsyme closed this as completed Aug 30, 2021
nosami pushed a commit to xamarin/visualfsharp that referenced this issue Jan 26, 2022
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f39617.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (dotnet#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (dotnet#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (dotnet#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (dotnet#9516) (dotnet#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (dotnet#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (dotnet#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (dotnet#9644) (dotnet#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (dotnet#9657) (dotnet#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (dotnet#9657) (dotnet#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839) (dotnet#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
nosami pushed a commit to xamarin/visualfsharp that referenced this issue Jan 26, 2022
* string interploation implementation

* string interploation tests

* escape {{ }}, test verbatim and triple quote, implement .NET specifiers

* fix tests

* string interpolation tests: internal representation corner cases

* string-interp tests should have --langversion:preview

* string interop tests: sprintf

* string interp tests: format specifier negative cases

* string interp tests: format specifier negative cases, .NET-style padding

* fix nested interp strings

* style cleanup

* lex: unify string interp stack and counter

* string-interp: add test cases

* fix mixed quote nested string interpolation

* string-interp: add test case for multiple interpolation points with different indentation

* lexfilter: push new CtxtParen at endPos for INTERP_STRING_PART and INTERP_STRING_BEGIN_PART

* lexfilter: do not check undentation limit for string interpolation tokens.

* FormattableString prototype

* add FormattableString support

* negative error checking

* remove diagnostics

* simpler FormattableString implementation

* fix test

* add testing for nested

* add IFormattable support

* tweak error message

* tests: StringInterpolation: fix case errors

* fix error message

* check number of values matches

* allow use of format strings with printf and friends

* update baselines

* fix baselines

* add Experimental attributes

* update string interp negative tests

* stringinterp test: add PrintFormat tests

* printf: fix empty interpolation string evaluates to null in printf env

* enable test corectly

* Revert "printf: fix empty interpolation string evaluates to null in printf env"

This reverts commit 7f39617.

* simplify codegen for interpolated strings

* fix build

* fix build

* Merge master to feature/string-interp (dotnet#9580)

* Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#9577)

Microsoft.DotNet.Arcade.Sdk
 From Version 1.0.0-beta.20302.3 -> To Version 1.0.0-beta.20326.2

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve perf for String.filter up to 3x (dotnet#9509)

* Improve perf for String.filter 2-2.5x

* Cleanup: remove "foo" etc in tests

* Add tests for new execution path for LOH in String.filter

* Change test string

* String map performance improvement (dotnet#9470)

* Simplify and improve perf of String.length

* Improve performance of String.map

* Revert "Simplify and improve perf of String.length"

* Resolves dotnet#9470 (comment)

* Lingering space

* Change `String` to use `new` to clarify use of ctor

* Add some better tests for String.map, add side-effect test

* Add tests to ensure the mapping function is called a deterministically amount of times

* Fix typo

* Remove "foo" from String.map tests

* Perf: String.replicate from O(n) to O(log(n)), up to 12x speed improvement (dotnet#9512)

* Turn String.replicate from O(n) into O(log(n))

* Cleanup String.replicate tests by removing usages of "foo"

* String.replicate: add tests for missing cases, and for the new O(log(n)) cut-off points

* Improve String.replicate algorithm further

* Add tests for String.replicate covering all lines/branches of algo

* Fix accidental comment

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Re enable tests for operators: OperatorsModule1.fs and OperatorsModule2.fs (dotnet#9516) (dotnet#9589)

* Re-enabling tests from OperatorsModule1/2.fs (compile errors)

* Fix compile errors in OperatorsModule1/2.fs, fix tests. Note tanh test comment.

* Fix `tanh` test, ensure stable result on x86 vs x64 runtimes

* Stop using exception AssertionException, so that test window shows useful info

* Whitespace cleanup and redundant code removal

* Cleanup spelling etc

* Re-enabling int, int16, int32, int64, nativeint, incr, nullArg etc tests

* Special-case floating-point assertion messages for higher precision output

* Fix/update/add tests (some still failing)

* Separate Checked tests, add & fix others, differentiate framework/bitness for some tests

* Add branch for .NET Native (ignore cos test)

* Resorting to comparing floats with a delta using Assert.AreNearEqual

* Add some more tests

Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9599)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576) (dotnet#9604)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>

* Merge master to feature/string-interp (dotnet#9615)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* Merge master to feature/string-interp (dotnet#9619)

* Moved fsharpqa/Libraries/Core/Unchecked test cases to NUnit (dotnet#9576)

* Moved fsharpqa/Libraries/Core/Reflectiontest cases to NUnit (dotnet#9611)

* Migrated PreComputedTupleConstructor01.fs test case

* Migrated PreComputedTupleConstructor02.fs test case

* Migrated DU.fs and Record.fs test cases

* Allow notebook to discover location of shared framework (dotnet#9596)

Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

* Text tweeks

* don't auto-resolve types from System.Runtime.WindowsRuntime (dotnet#9644) (dotnet#9648)

Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>

* yeet (dotnet#9657) (dotnet#9661)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* yeet (dotnet#9657) (dotnet#9670)

yeet

Co-authored-by: Phillip Carter <pcarter@fastmail.com>

* fix up tokenizer tests

* fix code review things

* fix code review things

* fix code review things

* fix code review things

* add various testing

* correct continuations for interpolated strings

* fix lexer continuations and colorization for multi-line interpolated strings

* revert xlf changes

* fix assert

* completion and brace matching (not all tests passing yet)

* Fix rebuild

* fix various niggles and get tests working

* fix printf when '%a' in final position

* fix test case

* interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix triple quote interpolated string specifer highlighting

* fix build

* fix missing error message

* fix % specifiers for interpolated strings

* fix % specifiers for interpolated strings

* fix FCS tests

* minor nits from code review

* code review feedback and use struct tuples in more places

* revert struct tuples

* use struct tuples where possible, byrefs for index

* fix byref for index

* fix ksprintf block size

* make recent cache entry more explicit (cleanup)

* improve performance

* remove unused code

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839) (dotnet#9848)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>

* Move existing Compiler.ComponentTests to a new Compiler.fs framework (dotnet#9839)

* Move existing Compiler.ComponentTests to a new Compiler.fs framework; Add 'parse' function

* Changed some wording in error messages

* fix dotnet#9893

* fix unmantched right brace in interp string

Co-authored-by: Yatao Li <yatli@microsoft.com>
Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>
Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Abel Braaksma <abel.online@xs4all.nl>
Co-authored-by: Thorsten Reichert <ThorstenReichert@users.noreply.github.com>
Co-authored-by: Phillip Carter <pcarter@fastmail.com>
Co-authored-by: Brett V. Forsgren <brettfo@microsoft.com>
Co-authored-by: Vlad Zarytovskii <vzaritovsky@hotmail.com>
@abelbraaksma
Copy link
Contributor Author

abelbraaksma commented Aug 15, 2022

@dsyme, I know you closed this, but some of the unaddressed issues are still quite apparent. For instance:

image

And this should just be an error like "String with unclosed opening curly bracket", but instead:

image

I mean, while they only "wreck" the editor temporarily, I think they are still bug and people see these things while they're typing the opening curly inside a string.

An error like this would make more sense:

image

But yeah, I get that these are all pretty fringe in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
None yet
Development

No branches or pull requests

7 participants