fix: Interpolated String Nesting #273
Conversation
|
Sorry for taking so long to look at this. I appreciate the pull request! However, I'm not sure what some of the changes you did actually do.... There are a couple of things that would need to be fixed:
What happens is that when we're processing the text buffer to identify braces, the scanner starts looking just within a portion of the file. Viasfora doesn't parse the entire buffer in one go, usually, but rather some part of it that is visible (plus some additional text to allow for faster scrolling). When you scroll, then, we'll restart scanning, but not from the top of the file, but from the last brace found. Before doing so, the scanner needs to be "reset" to avoid maintaining any state when the scanning is restarted. This is because we might also be restarting because the buffer was modified, and so any state in the scanner object could cause wrong behavior. That also means that any state that the scanner stores needs to be restartable. There are a couple of ways this is done:
So that means that in your code:
Does this clarify things somewhat? |
|
Thanks for taking your time trying to explain. Did i understand correctly that the the I am guessing it will be a performance problem if you haven't yet typed the end of the nested string so the scanner won't stop in a large file. I mean i could live with that. |
|
It's not that the scanner stops on its own. It's that the text buffer parser will simply stop feeding it content at any arbitrary time, based on the buffer region it decides to parse. What's important is how it restarts parsing. Let's say the previous parse operation stopped at character 1035, and it needs to restart. It will then start backtracking to braces found before position 1035, and for each one it will call Does that make sense? So since you can't keep the complex state you are creating, then you want to make sure |
|
Ah i see okay. |
|
You should not care, since you can't keep any state at all after |
|
Mh but i thought there is the So if i tell the Barce it is an nestedText instead of stText like it is right now because nesting currently wont get parsed it will not resume at that position. I'm gonna make an example hold on :) |
|
Yes, that is used to reconstruct your state. But that's only an int. You can't put a vector in there in any way. The biggest issue here isn't restarting where you left off. It's restarting after the buffer contents have changed. Let me give you an example. Let's say the last parse operation parsed until buffer position 1032. That's in the middle of an interpolated string, so you have nesting state in your scanner instance. Then the buffer is changed at position 974, so Viasfora will try to restart from position 952, which is before the interpolated string opens. Then all the nesting state in your current scanner instance is completely invalid. You can't use it at all because it doesn't reflect the actual buffer state from the position you will restart. Does that make sense? |
Sorry I can not understand the problem. 😟 string test = "";
List<string> testList = new List<string>();
test = string.IsNullOrEmpty($"{test} {(string.IsNullOrEmpty(test) ? "" : testList.Select(n => $"pre_{n}_suf").Aggregate((n, m) => n + ", " + m))}");the code stops parsing the interpolated string as an interpolated string after the round ) in My modification handled the nesting so stText will not be given back. Or am i missing something? |
| var indx = this.nestingIndex; | ||
| if(indx -1 < 0) { | ||
| this.multiLine = false; | ||
| this.nestingIndex = 0; | ||
| this.status = stText; |
There was a problem hiding this comment.
This in combination with the start in line 177.
Nesting got lost and stText was returned but it was still inside the nested string
|
I'm not sure I fully follow here, but yes, the biggest danger is going to be when the text buffer contents change. |
|
I'm gonna try saying it a bit differently. If we not look at the nested interpolated strings, your current code with normal interpolated strings will start with parsing the braces at the start of the interpolated string right? because the CanResume functions only returns true if the brace. State |
|
If I'm understanding this right, then the answer would be yes, that's correct. Sorry if I wasn't getting what you were driving at. |
|
oh i am sitting in a car and a bump on the street accedently made me close the ticket. i wanted to say the CanResume will backpadel until brace.State is stText so before the Interpolated string did start right? |
|
That will force the buffer processor to backtrack until the last brace before the interpolated string beginning when restarting. |
|
Your current code doesnt backtrack because it has a problem in detecting the nesting level. |
|
Looking at public void Bug263_InterpolatedStringWithInterpolatedString1() {
// bug is the missing ) in following section of the teststring:
// n => $\"pre_{n}_suf\").Aggregate
String input = "string.IsNullOrEmpty($\"{test} {(string.IsNullOrEmpty(test) ? \"\" : testList.Select(n => $\"pre_{n}_suf\").Aggregate((n,m) => n + \", \" + m))}\")";
var extractor = new CSharpBraceScanner();
var chars = Extract(extractor, input.Trim(), 0, 0);
Assert.Equal(1 + 1 + 1 + 2 + 1 + 1 + 1 + 1 + 1 + 1 + 2 + 1 + 3 + 1, chars.Count);
}Before: Debug Output: Resulting in: After: Debug Output:
|
Regarding Issue 263
Could you please check.
Your current interpolated nesting check is losing all its previous progress to the state.
I mean i you end an nested interpolated string you think that you are inside an normal Text again but you are still inside the parent interpolated string.
I have no idea what happens on your Reset method so i left the original variables.
Probably should implement a check in the nestingIndex setter so that there is always the default NestingOption.