handzlikchris/FastScriptReload

BUG - FSR doesn't check for Roslyn diagnostics, leading to very confusing behaviour when Roslyn fails to parse (due to old version)

Closed this issue · 0 comments

Note: The initial version of this issue attributed this to a Roslyn bug. I've since delved further, and realised that it's actually a version incompatibility, not a bug. FSR appears to be using a very old Roslyn version that can't handle modern language features. That makes this an instance of #22.

This is the entire content of a repro file which causes FSR problems.

public static class Foo
{
    public static void Step()
    {
        // If we put the instructor call directly here (without the inner function), bug goes away.

        static void Foo()
        {
            // If we call this as new() instead of new(0), bug goes away.
            // If we call this as new Bar(0) instead of new(0), bug goes away.
            Bar a = new(0);
        }
    }

    // Swap the order so this is before Step(), bug goes away.
    public readonly struct Bar
    {
        public Bar(int foo)
        {
        }
    }
}

FSR's output is like this. Note that Bar's identifier has been patched, even though it's a nested type.

public static class Foo__Patched_{
    public static void Step()
    {
        // If we put the instructor call directly here (without the inner function), bug goes away.

        static void Foo()
        {
            // If we call this as new() instead of new(0), bug goes away. 
            // If we call this as new Bar(0) instead of new(0), bug goes away.
            Bar a = new(0);
        }
    }

    // Swap the order so this is before Step(), bug goes away.
    public readonly struct Bar__Patched_    {
        public Bar__Patched_(int foo)
        {
        }
    }
}

Looking into it a little, HotReloadCompliantRewriter.VisitStructDeclaration is getting called for Bar in the cases where it fails. Checking the StructDeclarationSyntax's .Parent, it actually thinks its parent is the CompilationUnit.


Digging into it a little further, I realised that what's happening here is that FSR's Roslyn version is failing to parse the syntax used. For example, it can't handle the local function.

Roslyn is designed to continue parsing when it hits a syntax error, and to create a best effort output. This is to support scenarios such as Visual Studio's intellisense working while code is being typed, and therefore incomplete.

What's happening here is that Roslyn encounters unparseable code (because it's an out of date Roslyn version), but FSR doesn't check for parsing errors. It continues on blindly, transforming and outputting the mangled parse tree. This leads to very confusing silent errors.

There are two things which should be fixed here:

  1. Update the Roslyn version ASAP, AKA #22.
  2. FSR should check tree.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error) in CreateSourceCodeCombinedContents, and bail if Roslyn hasn't understood the underlying source code.

I've verified the cause of my issue by logging those diagnostics.