Location of Diagnostic Incorrect For Multiple Files When Using EOD

[broderickt2]

I am doing some advanced stuff with this library which involves having two classes in my code string. Here is the basis of what my test case looks like:

[TestMethod]
public void TestExample()
{
    const string code = @"
using System;
using System.ComponentModel.DataAnnotations;

namespace Rev.App.Validators.DataAnnotations
{
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter, AllowMultiple = false)]
    public class ValidEnumValueAttribute : ValidationAttribute
    {
    }
}

/*EOD*/

using Rev.App.Validators.DataAnnotations;
using System;
using System.ComponentModel.DataAnnotations;

namespace App
{
    public class Model
    {
        [|public Enum_Sample MyProperty { get; set; }|]
    }

    public enum Enum_Sample
    {
        Low,
        Medium,
        High
    }
}";

    var fixture = RoslynFixtureFactory.Create<SyncValidEnumValueAttributeAnalyzer>(new AnalyzerTestFixtureConfig
    {
        References = new[]
        {
            ReferenceSource.FromType<DataTypeAttribute>(),
            MetadataReference.CreateFromFile(Assembly.Load(new AssemblyName("netstandard")).Location)
        }
    });

    fixture.HasDiagnostic(code, SyncValidEnumValueAttributeAnalyzer.DiagnosticId);
}

Actual property location was between 163 and 206 as shown below.

After the "[|" and "|]" characters are removed and the file is trimmed, 163 is the start of public Enum_Sample MyProperty { get; set; } and 206 is the end as shown below.

However, the exception that gets thrown reports the location differently as shown below:

I think the string location for this is being based off the full string code and not the individual classes after they are split and trimmed if /*EOD*/ exists in the string. Here is the proof it starts at 497:

Let me know if this makes sense or if you have any questions. Thanks in advance!

[cezarypiatek]

Hi, Thanks for reporting this issue. However, the multi-file scenario was implemented on the assumption that there's always a single marker and the code with the marker must be the first chunk in the file. Change the order of the chunks in your file and it will work.

[broderickt2]

That makes sense and when I switch the order of the classes in my string, then it does work as expected. Is what you just described documented somewhere? Maybe an example using EOD in the Readme could be added to help explain this.

Although I am curious around the assumption you described, why not allow the marker to be in any file (like 2nd or 3rd or so on)? Is it not possible or adds too much complexity? As someone who is new to this library, this is something I didn't know about and confused me. I would advocate more flexibility for a smoother developer experience but glad there is a workaround.

[cezarypiatek]

I don't remember exactly, but I guess it was the easiest way to implement that. I definitely need to add an example to a readme.

[broderickt2]

I was looking around the repo and I think the changes to tweak this would done be in TryFindMarkedTimeSpan. Maybe something like this would work:

private static bool TryFindMarkedTimeSpan(string markupCode, out TextSpan textSpan)
{
    internal const string FileSeparator = "/*EOD*/";
    private readonly static Regex FileSeparatorPattern = new Regex(Regex.Escape(FileSeparator));

    var start = -1;
    var end = -1;   
    textSpan = default;

    if (markupCode.Contains(FileSeparator)
    {
        var codeFiles = FileSeparatorPattern.Split(markupCode).Reverse().ToList();

        foreach (var file in codeFiles)
        {
            start = file.Trim().IndexOf("[|", StringComparison.InvariantCulture);
            end = file.Trim().IndexOf("|]", StringComparison.InvariantCulture);

            if (start > -1 && end > -1)
            {
                break;
            }
            else
            {
                //Reset these values just in case
                start = -1;
                end = -1;
            }
        }
    }
    else
    {
        start = markupCode.IndexOf("[|", StringComparison.InvariantCulture);
        end = markupCode.IndexOf("|]", StringComparison.InvariantCulture);
    }   

    if (start < 0)
    {
        return false;
    }

    if (end < 0)
    {
        return false;
    }

    textSpan = TextSpan.FromBounds(start, end - 2);
    return true;
}

It would need cleaned up a little especially using the Base class constant FileSeparator instead of declaring it again here. What do you think?

[cezarypiatek]

Before I realized about the assumption I told you, I tried to fix it. It turned out to be quite complex to handle properly, so if it works, don't touch it :D

[broderickt2]

@cezarypiatek Thanks for the extra info and researching this. I will use the workaround