Skip to content
This repository has been archived by the owner on Sep 24, 2020. It is now read-only.

Combination of script.Remove/script.Link behaves differently in MonoDevelop than in NRefactory tests #190

Open
luiscubal opened this issue Jun 3, 2013 · 0 comments

Comments

@luiscubal
Copy link
Contributor

First of all, I don't know if this belongs here on in the MonoDevelop bug tracker.

When I was developing LockThisIssue, I encountered a bug that occurred in MonoDevelop when running the issue, but not in the unit tests.
The problem seems to be caused by a combination of script.Remove and script.Link. While this is may be a nonsensical operation, the NRefactory test context does NOT warn against it (no exceptions).
NRefactory seems to just ignore the removed nodes, while MonoDevelop attempts to write text for those nodes.

Consider this test case:

public class TestClass
{
    public void Foo()
    {
        if (true) { int x = 1; }
    }
}

Now, apply this action:

using ICSharpCode.NRefactory.CSharp.Refactoring;
using System.Collections.Generic;
using System.Linq;

namespace ICSharpCode.NRefactory.CSharp
{
    [ContextAction("Remove block contents", Description = "Remove block contents to reproduce weird NRefactory inconsistency")]
    public class WeirdAction : ICodeActionProvider
    {
        public IEnumerable<CodeAction> GetActions(RefactoringContext context)
        {
            var node = context.GetSelectedNodes().FirstOrDefault();
            var block = node as BlockStatement;

            if (block == null) {
                yield break;
            }

            yield return new CodeAction(context.TranslateString("Remove block contents"), script => {

                var newIdentifiers = new List<Identifier>();

                var identifiers = block.Descendants.OfType<Identifier>();
                foreach (var identifier in identifiers) {
                    var newIdentifier = Identifier.Create("replacement");
                    script.Replace(identifier, newIdentifier);
                    newIdentifiers.Add(newIdentifier);
                }

                foreach (var statement in block.Statements) {
                    script.Remove(statement);
                }

                script.Link(newIdentifiers.ToArray());

            }, block);
        }
    }
}

This NRefactory test passes:

using ICSharpCode.NRefactory.CSharp.CodeActions;
using NUnit.Framework;

namespace ICSharpCode.NRefactory.CSharp.CodeActions
{
    public class WeirdActionTests : ContextActionTestBase
    {
        [Test]
        public void TestAction() {
            var input = @"
class TestClass
{
    void Foo()
    {
        if (true) <-{ int x = 1; }->
    }
}";

            var output = @"
class TestClass
{
    void Foo()
    {
        if (true) {  }
    }
}";

            Test<WeirdAction>(input, output);
        }
    }
}

However, if we run this on MonoDevelop, it will still try to put text inside the block.
monodevbug

Additionally, if there is more than one linked and removed node, MonoDevelop will attempt to put text for each block.

class TestClass
{
    void Foo()
    {
        if (true) { int x = 1; int y = 2; }
    }
}

monodevbug2

Things get worse when not all fields are removed.

using ICSharpCode.NRefactory.CSharp.Refactoring;
using System.Collections.Generic;
using System.Linq;

namespace ICSharpCode.NRefactory.CSharp
{
    [ContextAction("Remove block contents", Description = "Remove block contents to reproduce weird NRefactory inconsistency")]
    public class WeirdAction : ICodeActionProvider
    {
        public IEnumerable<CodeAction> GetActions(RefactoringContext context)
        {
            var node = context.GetSelectedNodes().FirstOrDefault();
            var block = node as BlockStatement;

            if (block == null) {
                yield break;
            }

            yield return new CodeAction(context.TranslateString("Remove block contents"), script => {

                var newIdentifiers = new List<Identifier>();

                var identifiers = block.Descendants.OfType<Identifier>();
                foreach (var identifier in identifiers) {
                    var newIdentifier = Identifier.Create("replacement");
                    script.Replace(identifier, newIdentifier);
                    newIdentifiers.Add(newIdentifier);
                }

                foreach (var statement in block.Statements.Skip(1)) {
                    script.Remove(statement);
                }

                script.Link(newIdentifiers.ToArray());

            }, block);
        }
    }
}

Now, this test passes:

            var input = @"
class TestClass
{
    void Foo()
    {
        if (true) <-{ int x = 1; int y = 2; }->
    }
}";

            var output = @"
class TestClass
{
    void Foo()
    {
        if (true) { int replacement = 1;  }
    }
}";

            Test<WeirdAction>(input, output);

But if we run this with MonoDevelop, we get this:
monodevbug3

I understand that linking and removing the same node is not a good idea and that workarounds are trivial, but still having some sort of exception when running the test cases would be very helpful - because right now actions and issues could pass the tests but still fail in real world usage.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant