Potential small bug in Tokenizer.Lookahead when using TakeAll(...)

[Antaris]

I'm currently working on an experimental port of the Razor parser to typescript, and on reviewing the tokenizer code, I've noticed what I perceive to be a small error when performing a lookahead.

https://github.com/aspnet/Razor/blob/dev/src/Microsoft.AspNet.Razor/Tokenizer/Tokenizer.cs#L288

The Tokenizer.Lookahead method allows you to perform a lookahead operation on the input text document. This is called through either Tokenizer.TakeAll and Tokenizer.At, but its the TakeAll method that passes the takeIfMatch parameter set to true which preserves the existing buffer content while attempting the operation, but I've noticed that you're not actually storing the buffer content. See the following:

// Capture the current buffer content in case we have to backtrack
string oldBuffer = null;
if (takeIfMatch)
{
  Buffer.ToString();
}

Later you're then trying to restore the content to the buffer:

https://github.com/aspnet/Razor/blob/dev/src/Microsoft.AspNet.Razor/Tokenizer/Tokenizer.cs#L301

if (takeIfMatch)
{
  // Clear the buffer and put the old buffer text back
  Buffer.Clear();
  Buffer.Append(oldBuffer);
}

At this stage, oldBuffer is never assigned, so nothing would be restored to the buffer.

Is this intentional or just an err? Happy to do a PR to correct it if its an issue, but having been in there for quite a while, I'm guessing it's likely not an issue?

[NTaylorMullen]

Nice find. I tried reproing the issue and it's actually incredibbbly uncommon to get into a state where this breaks Razor. Nevertheless. We should fix 😄

[Antaris]

Yeah, I figured it wasn't a massive issue :-)

[Antaris]

Just to follow up on this, I've noticed something else that probably doesn't really impact anything, but seems a bit erroneous.

https://github.com/aspnet/Razor/blob/dev/src/Microsoft.AspNet.Razor/Tokenizer/Tokenizer.cs#L108

The Tokenizer.TakeString method accepts a parameter caseSensitive to allow case-[in]sensitive matches, but the logic here seems to invert the filter you actually want:

if (caseSensitive)
{
    charFilter = Char.ToLower;
}

Shouldn't that actually be a if (!caseSensitive)?

The TakeString method isn't actually called from either the CSharpTokenizer or the HtmlTokenizer so it's probably just a utility method that doesn't get used, so doesn't actually cause any problems.

[NTaylorMullen]

Hey @Antaris so I'm working on fixing this and it turns out we don't use most of this code anymore in Razor 😄. This is primarily due to the removal of VisualBasic from Razor. I'll fix the Lookahead to prevent future issues and remove unneeded code like the TakeString method.

See commit for removal of dead code here.

[Antaris]

@NTaylorMullen Are you gonna have to port that back again if/when you reintroduce support for VB? Is VB support still even on the roadmap? Thanks for the fix, I can migrate those changes to my TypeScript port :-)