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.

Sign up for GitHub

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

Jump to bottom

Verbatim string ast #10769

Merged
merged 9 commits into from
Feb 2, 2021
Merged

Verbatim string ast #10769

merged 9 commits into from
Feb 2, 2021

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 21, 2020 •
edited
Loading

An attempt to keep track in the AST whether a string is verbatim and/or triple quote or not.
Fixes #10209.

Many thanks again @baronfel for doing some pair programming on this.

@nojaf nojaf mentioned this pull request Dec 21, 2020
Closed
@cartermp
Copy link
Contributor

Hmm, saw the same test. We regressed opening static classes somehow. cc @KevinRansom

@cartermp
Copy link
Contributor

errr, default interface tests

@KevinRansom KevinRansom reopened this Dec 21, 2020
@nojaf
Copy link
Contributor Author

nojaf commented Dec 22, 2020

This is ready for review.
Any ideas if the failing test

 Failed C# diamond complex hierarchical interfaces then combined in one C# interface and then implemented - Runs - 3 [528 ms]
  Error Message:
     'CSharpIFinalCombinedTest-Method1-CSharpIFinalCombinedTest-Method2' = ''
  Expected string length 65 but was 0. Strings differ at index 0.
  Expected: "CSharpIFinalCombinedTest-Method1-CSharpIFinalCombinedTest-Met..."
  But was:  <string.Empty>
  -----------^

  Stack Trace:
     at <StartupCode$FSharp-Test-Utilities>[email protected](String output) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 499
   at <StartupCode$FSharp-Test-Utilities>[email protected](Tuple`2 tupledArg) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 494
   at FSharp.Test.Utilities.CompilerAssert.compileCompilation[a](Boolean ignoreWarnings, Compilation cmpl, FSharpFunc`2 f) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 391
   at <StartupCode$FSharp-Test-Utilities>[email protected](Unit unitVar0) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 487
   at FSharp.Test.Utilities.CompilerAssert.Execute(Compilation cmpl, FSharpOption`1 ignoreWarnings, FSharpOption`1 beforeExecute, FSharpOption`1 newProcess, FSharpOption`1 onOutput) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 485
   at FSharp.Test.Utilities.CompilerAssert.ExecutionHasOutput(Compilation cmpl, String expectedOutput) in D:\a\1\s\tests\FSharp.Test.Utilities\CompilerAssert.fs:line 499
   at FSharp.Compiler.UnitTests.DefaultInterfaceMemberConsumptionTests.C# diamond complex hierarchical interfaces then combined in one C# interface and then implemented - Runs - 3() in D:\a\1\s\tests\fsharp\Compiler\Language\DefaultInterfaceMemberTests.fs:line 3198

is related to my changes?

@cartermp
Copy link
Contributor

@nojaf No, your changes are fine, but something is wrong with your merge of main into this branch, since it's not picking up a fix to our test helpers that was made recently. Would it be possible to just rebase atop latest main?

@nojaf nojaf force-pushed the verbatim-string-ast branch 2 times, most recently from e478314 to 98139c8 Compare December 22, 2020 18:00
@nojaf
Copy link
Contributor Author

nojaf commented Dec 22, 2020 •
edited
Loading

@cartermp I think the rebase is ok now. Getting different problems now.
Let me know if there is anything I can do.

@cartermp
Copy link
Contributor

Are you against the very latest main? The issues now are compile-time because of recent FCS changes:

error FS0001: This expression was expected to have type�    'LayoutTag'    �but here has type�    'string'

Either that or we have our internal build broken? Will investigate

@baronfel
Copy link
Member

Oh, looks like there are changes required to the vsintegration as a result of this. We were only looking at FCS-level changes, which explains that. @nojaf you'll need to update the vsintegration in FSharp.Editor as well. My bad, man.

@cartermp
Copy link
Contributor

No, it's on our end. @dsyme some of your changes were merged without updating VS, leading to compile errors.

@cartermp
Copy link
Contributor

This fixes it: #10783

@nojaf
Copy link
Contributor Author

nojaf commented Dec 22, 2020

Great, thanks for the quick response 👍.

@nojaf nojaf force-pushed the verbatim-string-ast branch from 98139c8 to a9c4bdd Compare December 22, 2020 20:57
@cartermp cartermp closed this Dec 22, 2020
@cartermp cartermp reopened this Dec 22, 2020
@nojaf nojaf force-pushed the verbatim-string-ast branch from a9c4bdd to 2a1e51b Compare December 24, 2020 12:08
TIHan
TIHan reviewed Jan 4, 2021
View reviewed changes
@nojaf nojaf force-pushed the verbatim-string-ast branch from 1a327cb to e91e8bf Compare January 5, 2021 20:14
@cartermp cartermp closed this Jan 5, 2021
@cartermp cartermp reopened this Jan 5, 2021
KevinRansom
KevinRansom previously requested changes Jan 15, 2021
View reviewed changes
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

APIs with bool->bool->bool are real easy to get wrong, can you simplify, use enums, or a flag enum or something please.

Otherwise this looks great.
Thanks

@nojaf nojaf force-pushed the verbatim-string-ast branch from e91e8bf to f7dbaf3 Compare January 15, 2021 14:22
@nojaf nojaf force-pushed the verbatim-string-ast branch from f7dbaf3 to 0aebae1 Compare January 17, 2021 14:06
cartermp
cartermp approved these changes Jan 17, 2021
View reviewed changes
Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is great.

@cartermp cartermp requested a review from KevinRansom January 17, 2021 20:17
@nojaf
Copy link
Contributor Author

nojaf commented Feb 1, 2021

I also extended SynExpr.InterpolatedString to check if there were triple quotes present.

TIHan
TIHan reviewed Feb 1, 2021
View reviewed changes
TIHan
TIHan approved these changes Feb 1, 2021
View reviewed changes
Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

This looks good. I had one comment regarding the use of bools for the token definitions.

@cartermp cartermp merged commit bc1375d into dotnet:main Feb 2, 2021
@nojaf nojaf deleted the verbatim-string-ast branch May 16, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TIHan TIHan TIHan approved these changes

@cartermp cartermp cartermp approved these changes

@KevinRansom KevinRansom Awaiting requested review from KevinRansom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Keep track of verbatim string in Untyped Syntax Tree.
5 participants
@nojaf @cartermp @baronfel @TIHan @KevinRansom