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

[WIP, FS-1001] string interpolation #3593

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@realvictorprm
Contributor

realvictorprm commented Sep 15, 2017

Signed-off-by: realvictorprm mueller.vpr@gmail.com

First attempt towards C# like string interpolation
Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
First working interpolation for strings
Most of the added code is from the old PR of @forki. I copied it partly because I tried to better understand how it should work roughly.

However I think it's better to move a part of the parsing to the parser and lexer. But that's for later.

Do not try to interpolate integers, this WILL result in an Fatal-Execution-Engine-Error. I'm afraid.

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
@realvictorprm

should be done later

@@ -886,7 +886,7 @@
<value>This expression is a function value, i.e. is missing arguments. Its type is {0}.</value>
</data>
<data name="UnitTypeExpected" xml:space="preserve">
<value>The result of this expression is implicitly ignored. Consider using 'ignore' to discard this value explicitly, e.g. 'expr |> ignore', or 'let' to bind the result to a name, e.g. 'let result = expr'.</value>

This comment has been minimized.

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

</resheader>
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>

This comment has been minimized.

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

@@ -112,10 +112,10 @@
<value>2.0</value>
</resheader>
<resheader name="reader">
<value>System.Resources.ResXResourceReader, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>

This comment has been minimized.

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Revert

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Sep 16, 2017

Contributor

Stuff like this is already possible:
usecase

I love it <3 this is exactly my use case!

Contributor

realvictorprm commented Sep 16, 2017

Stuff like this is already possible:
usecase

I love it <3 this is exactly my use case!

realvictorprm added some commits Sep 16, 2017

Fixed Fatal-Execution-Engine-Error.
Added first real-working string-interpolation.

Okay now the string interpolation should work for all types.
I try to explain with this example what the compiler should produce if a string is interpolated:

Example code:
```fsharp
let value = 10
let interpolatedResult = $"Value as string: {value}"
```

The compiler should produce:

```fsharp
let value = 10
let interpolatedResult = "Value as string: " + value.ToString()
```

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
Fixes, cleanup and adding ability for escaping brackets.
However the lexer must be improved to respect nested interpolated strings. Currently it fails to parse them correctly.

The parsing of the nested interpolated strings in CheckFormatStrings.fs needs improvement too.

Removed commented code, corrected error reports.

Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
| _ -> false
| _ -> false
)
let tostring =

This comment has been minimized.

@baronfel

baronfel Sep 16, 2017

Contributor

per @vasily-kirichenko's comment on the RFC discussion thread, would it be possible here to detect if the type of the current expression is an F# type and use the equivalent of the %A format string instead of the ToString method for the type?

@baronfel

baronfel Sep 16, 2017

Contributor

per @vasily-kirichenko's comment on the RFC discussion thread, would it be possible here to detect if the type of the current expression is an F# type and use the equivalent of the %A format string instead of the ToString method for the type?

This comment has been minimized.

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

If it produces better results I'll definitely do so. I know thanks to you already the details why it should be different treated. To implement it I would need to know from someone which is familiar with the compiler whether I can just check if a type is an F# type.

@realvictorprm

realvictorprm Sep 16, 2017

Contributor

If it produces better results I'll definitely do so. I know thanks to you already the details why it should be different treated. To implement it I would need to know from someone which is familiar with the compiler whether I can just check if a type is an F# type.

This comment has been minimized.

@saul

saul Sep 17, 2017

Contributor

Specifically only do this if ToString hasn't been overriden by the user.

@saul

saul Sep 17, 2017

Contributor

Specifically only do this if ToString hasn't been overriden by the user.

This comment has been minimized.

@realvictorprm

realvictorprm Sep 18, 2017

Contributor

I don't like the current casting code too. I would like to change it as soon as I found out how to inject format functions into the casting.

@realvictorprm

realvictorprm Sep 18, 2017

Contributor

I don't like the current casting code too. I would like to change it as soon as I found out how to inject format functions into the casting.

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Sep 18, 2017

Contributor

There shouldn't be any fails, what's wrong with the CI :( ?

Contributor

realvictorprm commented Sep 18, 2017

There shouldn't be any fails, what's wrong with the CI :( ?

@cartermp

This comment has been minimized.

Show comment
Hide comment
@cartermp

cartermp Sep 18, 2017

Collaborator

@realvictorprm If you click on Details and view the Console Output, you can see why. In this case, it's a timeout.

@dotnet-bot test Windows_NT Release_ci_part2 Build

Collaborator

cartermp commented Sep 18, 2017

@realvictorprm If you click on Details and view the Console Output, you can see why. In this case, it's a timeout.

@dotnet-bot test Windows_NT Release_ci_part2 Build

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Sep 18, 2017

Contributor

Ah yes sorry. Thanks @cartermp

Contributor

realvictorprm commented Sep 18, 2017

Ah yes sorry. Thanks @cartermp

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 20, 2017

Contributor

@realvictorprm I went to do a review and I was going to start by looking at the tests, to understand the proposal from that perspective, rather just the implementation. Could you add a set of tests that capture both the basics and the nuances in the design? Perhaps look at the Roslyn tests for the corresponding C# feature?

thanks!

Contributor

dsyme commented Sep 20, 2017

@realvictorprm I went to do a review and I was going to start by looking at the tests, to understand the proposal from that perspective, rather just the implementation. Could you add a set of tests that capture both the basics and the nuances in the design? Perhaps look at the Roslyn tests for the corresponding C# feature?

thanks!

@@ -5415,7 +5427,7 @@ let TypeCheckOneInputEventually
// Typecheck the signature file
let! (tcEnv, sigFileType, createsGeneratedProvidedTypes) =
TypeCheckOneSigFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, tcConfig.conditionalCompilationDefines, tcSink) tcState.tcsTcSigEnv file
TypeCheckOneSigFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, tcConfig.conditionalCompilationDefines, tcSink) tcState.tcsTcSigEnv (GetExpressionParser(tcConfig, new Lexhelp.LexResourceManager())) file

This comment has been minimized.

@dsyme

dsyme Sep 20, 2017

Contributor

Bind (GetExpressionParser(tcConfig, new Lexhelp.LexResourceManager())) as a let please

@dsyme

dsyme Sep 20, 2017

Contributor

Bind (GetExpressionParser(tcConfig, new Lexhelp.LexResourceManager())) as a let please

@@ -2672,6 +2674,7 @@ rawConstant:
| BIGNUM { SynConst.UserNum $1 }
| stringOrKeywordString { SynConst.String ($1,lhs parseState) }
| BYTEARRAY { SynConst.Bytes ($1,lhs parseState) }
| STRING_INTERPOLATED { SynConst.InterpolatedString ($1,lhs parseState) }

This comment has been minimized.

@dsyme

dsyme Sep 20, 2017

Contributor

What happens if interpolated strings are used in patterns - i.e. do we get a good error saying this is not supported?

@dsyme

dsyme Sep 20, 2017

Contributor

What happens if interpolated strings are used in patterns - i.e. do we get a good error saying this is not supported?

@dsyme

It's looking nice, though as I mentioned above I'd like to see test cases to understand better

@realvictorprm

This comment has been minimized.

Show comment
Hide comment
@realvictorprm

realvictorprm Sep 20, 2017

Contributor

I understand @dsyme, I'm going to provide some tests. There are plenty of cases which needs to be caught.

Can you or someone else give me a tip on how to change the lexer / parser so that nested interpolated string expressions would be allowed?

Contributor

realvictorprm commented Sep 20, 2017

I understand @dsyme, I'm going to provide some tests. There are plenty of cases which needs to be caught.

Can you or someone else give me a tip on how to change the lexer / parser so that nested interpolated string expressions would be allowed?

@dsyme dsyme changed the title from [WIP] First attempt towards C# like string interpolation to [WIP, F# 4.2 ] First attempt towards C# like string interpolation Sep 20, 2017

@dsyme dsyme changed the title from [WIP, F# 4.2 ] First attempt towards C# like string interpolation to [WIP, F# 4.2 ] string interpolation Dec 1, 2017

@dsyme dsyme changed the title from [WIP, F# 4.2 ] string interpolation to [WIP, RFC FS-1001 ] string interpolation Dec 1, 2017

@dsyme dsyme changed the title from [WIP, RFC FS-1001 ] string interpolation to [WIP, FS-1001] string interpolation Dec 1, 2017

@KevinRansom

This comment has been minimized.

Show comment
Hide comment
@KevinRansom

KevinRansom Sep 12, 2018

Contributor

@realvictorprm, @dsyme

This PR hasn't been actively looked at in a year … do you have plans to revisit it?

Thanks

Kevin

Contributor

KevinRansom commented Sep 12, 2018

@realvictorprm, @dsyme

This PR hasn't been actively looked at in a year … do you have plans to revisit it?

Thanks

Kevin

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Sep 19, 2018

Contributor

I will close, it is linked from the RFC but and may be used as the basis for the implementation when we rework it

Contributor

dsyme commented Sep 19, 2018

I will close, it is linked from the RFC but and may be used as the basis for the implementation when we rework it

@dsyme dsyme closed this Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment