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
Conversation
Signed-off-by: realvictorprm <mueller.vpr@gmail.com>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
</resheader> | ||
<resheader name="writer"> | ||
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert
src/fsharp/TypeChecker.fs
Outdated
else | ||
mkString cenv.g m s, tpenv | ||
else failwith "not supported yet" | ||
//if (AddCxTypeEqualsTypeUndoIfFailed env.DisplayEnv cenv.css m overallTy cenv.g.string_ty) then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to remove this, darn!
src/fsharp/TypeChecker.fs
Outdated
) | ||
|
||
// BuildFSharpMethodApp cenv.g m cenv.stringFormatEnv.Value.StringConcatMethod | ||
let coersed = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here lies the problem, I must change the string conversion, it doesn't work for non string types and produces failing IL code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved with the last commit. It's now converting all types to a string representation with the famous ToString
method.
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>
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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically only do this if ToString hasn't been overriden by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There shouldn't be any fails, what's wrong with the CI :( ? |
@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 |
Ah yes sorry. Thanks @cartermp |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if interpolated strings are used in patterns - i.e. do we get a good error saying this is not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking nice, though as I mentioned above I'd like to see test cases to understand better
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? |
This PR hasn't been actively looked at in a year … do you have plans to revisit it? Thanks Kevin |
I will close, it is linked from the RFC but and may be used as the basis for the implementation when we rework it |
Signed-off-by: realvictorprm mueller.vpr@gmail.com