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

[WIP] Enable double dollar for triple quoted interpolated strings #13259

Conversation

alfonsogarciacaro
Copy link
Contributor

@alfonsogarciacaro alfonsogarciacaro commented Jun 8, 2022

See fsharp/fslang-suggestions#1150

This doesn't include percent symbol yet, I'm sending the draft PR to see if the approach is correct. I started trying to implement variable dollar length as in C# but then I realized LexerStringKind is byte encoded in ServiceLexing.fs, so I kept is as a flag for simplicity. If we finally only allow double dollar (and not triple, quadruple) this should be stated in the RFC.

@dnfadmin
Copy link

dnfadmin commented Jun 8, 2022

CLA assistant check
All CLA requirements met.

// To continue token-by-token lexing may involve picking up the new args.stringNes
let cont = LexCont.Token(args.ifdefStack, args.stringNest)
LBRACE cont
}

| "|" { BAR }

| "}"
| ("}}" | "}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is parsing "}}" as a single "}" in regular F# code, hence the tests failing to compile

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing @dsyme! So I assume when this is "}}" but we detect this is not closing an interpolation gap we need to either consume only a single char of the lexeme or somehow be able to return RBRACE (RBRACE cont). How can I do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably add a new RBRACE_RBRACE token and smash it apart in the lexfilter. Though be careful of |}} too and check if anything else can be combined with }.

Likewise LBRACE of course

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sample smash is here:

https://github.com/dotnet/fsharp/blob/main/src/Compiler/SyntaxTree/LexFilter.fs#L2442

Not sure there's any better way to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme As I don't know well how the lexfilter works and as you say, we can have problems with things like {{|, I'm trying to move back the lexeme end position if we shouldn't consume the two braces. Is this a bad idea?

Also, for some reason CI is not running tests. I cannot get repo to build locally either. When I run Build.cmd at one point I get these error (same in main branch):

C:\Users\alfon\repos\fsharp\tests\FSharp.Core.UnitTests\FSharp.Core\RefStructs.fs(20,18): error FS0412: A type instanti
ation involves a byref type. This is not permitted by the rules of Common IL. [C:\Users\alfon\repos\fsharp\tests\FSharp
.Core.UnitTests\FSharp.Core.UnitTests.fsproj]
C:\Users\alfon\repos\fsharp\tests\FSharp.Core.UnitTests\FSharp.Core\RefStructs.fs(20,6): error FS0437: A type would sto
re a byref typed value. This is not permitted by Common IL. [C:\Users\alfon\repos\fsharp\tests\FSharp.Core.UnitTests\FS
harp.Core.UnitTests.fsproj]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is not running tests if there's a merge conflict, IIRC.
The local issue is weird, does this happen if you build with -noVisualStudio switch too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @vzarytovskii! I will fix the merge conflicts 👍 And the -noVisualStudio switch does indeed work! I do have Visual Studio installed but I don't use it a lot and maybe the installation is faulty (I had problems to open FSharp.sln with VS).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @vzarytovskii! I will fix the merge conflicts 👍 And the -noVisualStudio switch does indeed work! I do have Visual Studio installed but I don't use it a lot and maybe the installation is faulty (I had problems to open FSharp.sln with VS).

Huh, interesting, thanks. I will add some notes to the DEVGUIDE on how to deal with such errors (incl. VS repair and building with the flag).

@alfonsogarciacaro alfonsogarciacaro force-pushed the interpolation-double-dollar branch from 57e81fb to 5d4a3d7 Compare June 14, 2022 02:18
@@ -854,6 +854,9 @@ rule token args skip = parse
// Note, we do not update the 'm', any incomplete-interpolation error
// will be reported w.r.t. the first '{'
args.stringNest <- (counter + 1, style, isDoubleDollar, m) :: rest
| _ when isDouble ->
lexbuf.LexemeLength <- 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks plausible. Does it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dsyme Maybe, I'm not sure. Tests are failing but it seems that it's not detecting well when an interpolation gap closes now. I wasn't sure how to debug the lexer tests. Unfortunately I won't be able to work on the PR this week, so it'd be great if someone could take over.

@alfonsogarciacaro alfonsogarciacaro force-pushed the interpolation-double-dollar branch from f3f9d34 to c422958 Compare June 15, 2022 07:32
@alfonsogarciacaro
Copy link
Contributor Author

Sorry, have to close this PR because I couldn't make the parser work. If someone with better knowledge can take it over it'd be great, as this feature will be very beneficial when embedding HTML, CSS, JSON, etc in F#.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants