Moving class with syntax errors to another file

[miloush]

Version Used: 15.6.0 Preview 1

Steps to Reproduce:

namespace ConsoleApp1
{
    class Program
    {
        private class Nested()
        {
            throw new System.NotImplementedException();
        }

        #region Hello
        private void M() { }
        #endregion
    }
}

Invoke Move type to Program.Nested.cs on the class Nested() line.

Actual Behavior:
Program.cs:

namespace ConsoleApp1
{
    partial class Program
    {
()
        {
            throw new System.NotImplementedException();
        }

        #region Hello
        private void M() { }
        #endregion
    }
}

Program.Nested.cs:

namespace ConsoleApp1
{
    partial class Program
    {
        private class Nested        }
        #endregion
    }
}

Expected Behavior:
I understand this is syntactically broken scenario, but I think the result is perhaps unnecessarily bad. Taking the whole

    class Nested()
    {
        throw new System.NotImplementedException();
    }

and putting it into a new file was the expected though I agree naive behavior in this case. Having only

partial class Program
{
    class Nested
}

in the nested file would be also fine, but the extra } and random #endregion seems to be coming out of nowhere - done this in a larger class, should I be worrying it got moved from somewhere it shouldn't have?

Context:
I wanted to move a method to a new file the same way I can move nested classes, so I just changed void to class to get the refactoring. Maybe the Move to file could work on members too?

[CyrusNajmabadi]

So:

        private class Nested()
        {
            throw new System.NotImplementedException();
        }

Is one of those "very broken" syntactic cases. Like "the parser goes off the rails" syntactic cases :)

We could attempt to make the parser a it better here (For example, seeing () and tryign to skip it better). But that would only be a spot fix. If you had something like "private class Nested(int i)" then it would be equally bad.

Similarly, we could try to replace the "class" with "void" and treat this like a method. But that would be hard with lookahead (given that generics might be involved).

Now, once you have a really busted syntax tree, all bets are off with how features are going to behave. In a case like thsi, i'd probably best prefer that move-type not even be offered as it will screw things up majorly. The problem is trying to figure out how to suppress this feature here. We don't want to suppress move-type just because a tiny, non-problematic syntax error (like a missing semicolon deep in some code in the class). but we do want to suppress when the type is totally broken. The issue is knowing which is which.

Perhaps a good check would be "does the class have missing { and } tokens". If so, then don't offer the fix.

[CyrusNajmabadi]

PRs welcome. I could walk someone through how to do this.

[CyrusNajmabadi]

Note: because we may eventually do primary constructors, we could consider having the parser try to parse out a parameter list after it's type arguent list. That parameter list could then be attached as a skipped node to the name or > token. We have precedence for that type of strategy. For example:

ParseMethodDeclaration has the following in it:

                else if (this.CurrentToken.Kind == SyntaxKind.ColonToken)
                {
                    // Use else if, rather than if, because if we see both a constructor initializer and a constraint clause, we're too lost to recover.
                    var colonToken = this.CurrentToken;

                    ConstructorInitializerSyntax initializer = this.ParseConstructorInitializer();
                    initializer = this.AddErrorToFirstToken(initializer, ErrorCode.ERR_UnexpectedToken, colonToken.Text);
                    paramList = AddTrailingSkippedSyntax(paramList, initializer);

                    // CONSIDER: Parsing an invalid constructor initializer could, conceivably, get us way
                    // off track.  If this becomes a problem, an alternative approach would be to generalize
                    // EatTokenWithPrejudice in such a way that we can just skip everything until we recognize
                    // our context again (perhaps an open brace).
                }

This is to deal with someone accidentally writing:

public void MyClass(int i, int j) : this(i) { }

Here the person meant to write a constructor, but had a return type accidenty there. We don't want the ": this(i)" to throw everything off so we look for it, parse it, and just attach it as bogus syntax in the tree. I think doing hte same for a parameter list on a class/struct declaration would be feasible and would help here.

[miloush]

Yes, I found this worth mentioning because I know there was a working prototype where parameters after type were valid syntax. Not sure how interesting is to fix this particular case, but it's a bit unfortunate that the parser gets that off when there even isn't anything unbalanced.

[CyrusNajmabadi]

Not sure how interesting is to fix this particular case, but it's a bit unfortunate that the parser gets that off when there even isn't anything unbalanced.

Error recovery is exceptionally difficult. For all the parser knows, these parens could have been anything. Maybe they're the start of a lambda. Maybe they're a parameter list to a method. Maybe it's a tuple. When it seems extremely strange stuff in a very unexpected place in the tree, it's really hard to tell what to do :) this is truly a 'panic mode' scenario where recovery (especially without keywords) is super hard. it's always possible to put in one-off fixes. But the general problem is actually 'unsolved' as far as the theory and literature are concerned. There are further techniques and approaches we can take (i did a lot of research in this space over the last couple of decades). However:

1. They're complex.
2. They can dramatically impact the cost of parsing (both in terms of time and memory).
3. The vast majority of syntax errors don't benefit from them.

The general problem of "people take chunks of code that are legal in one context (liek expressions/statements) and place them into random other locations (like after a declaration name) don't tend to appear that often. and, if they do, they're fairly ephemeral. I.e. the user will edit things to fix it up. It's usually very unlikely that someone both does this, and then performs high level refactorings. So investing in this space isn't very bang/bucky.