Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight "string[]", "string []", "string[,]", "string [,]", "string… #670

Merged
merged 1 commit into from
Aug 17, 2016

Conversation

ivanz
Copy link
Contributor

@ivanz ivanz commented Aug 17, 2016

Highlight "string[]", "string []", "string[,]", "string [,]", "string[ , ]", etc properly inside method parameter list.

Fixes #657

Before:

before

After:

after

… [ , ]", etc properly inside method parameter list.

Fixes dotnet#657
@DustinCampbell
Copy link
Member

LGTM. Thanks @ivanz!

@DustinCampbell DustinCampbell merged commit 1c06c9d into dotnet:master Aug 17, 2016
@seraku24
Copy link
Contributor

Hi, folks. Not sure if this is the best place to comment, but I reviewed the updated regex and found that it is not complete and, unfortunately, regresses behavior. Also, the original regex was incorrect with respect to C# syntax in a few ways, so the updated one inherited those flaws.

Here is a program I put together to test out two candidates for the regex:

using System;
using System.Linq;
using System.Text.RegularExpressions;

namespace Seraku.SyntaxHighlightingRegexTest
{
    internal static class Program
    {
        private static void Main(string[] args) {
            var regexes = new [] {
                new Regex(@"\b(ref|params|out)?\s*\b(\w+(?:\s*<.*?>)?(?:\s*\?)?(?:\s*\[[\s,]*\])*)\s+(\w+)\s*(=)?"),
                new Regex(@"\b(ref|params|out)?\s*\b(\w+(?:\s*<.*?>)?(?:\s*\?)?(?:\s*\[.*?\])?)\s+(\w+)\s*(=)?"),
            } ;
            var types = "int|double?|IEnumerable<string>|Func<object, bool>|Foo<long>?|Outer<Inner<object>>".Split('|');
            var suffixes = "|[]|[][]|[,]|[,,]|[][,]|[,,][][]".Split('|');
            var query = from type in types from suffix in suffixes select $"{type}{suffix} name";
            foreach (var regex in regexes) {
                Console.Write($"{regex} -> ");
                if (query.All(x => regex.IsMatch(x))) {
                    Console.WriteLine("All match.");
                } else {
                    Console.WriteLine("Some failed to match:");
                    Console.WriteLine(string.Join("\n", query.Where(x => !regex.IsMatch(x))));
                }
            }
        }
    }
}

The regexes above both address the following issues:

  1. Array specification must be the final element of a type. (Current behavior matches invalid types like Func[]<int> but not valid types like Func<int>[].)
  2. Array specification may have any number of dimensions. (Current behavior matches int[,] but not int[,,] or beyond.)
  3. Array specification may include arrays of arrays. (Old behavior matches int[][], updated behavior regresses this, but neither support mixed array types like int[][,].)
  4. Nullable type specification ? may appear after any non-nullable type, including templated value types. (Current behavior matches invalid types like Foo?<int> but not valid types like Foo<int>?.)

The only difference between the two candidates is how the array syntax is handled. The first regex is more syntactically correct with respect to array specification; whereas the second regex is much less picky with the arrays, similar to how template types are parsed. But given that invalid syntax like Func<:-)> smiley is currently being highlighted, it is hardly a problem if string[O_o] what gets highlighted as a side effect.

@ivanz
Copy link
Contributor Author

ivanz commented Aug 18, 2016

@seraku24 Good catch. I suggest creating a pull request and re-opening the original issue linked in this pull request.

seraku24 added a commit to seraku24/omnisharp-vscode that referenced this pull request Aug 18, 2016
Fixes (again) dotnet#657 as well as issues brought up in dotnet#670.

- Corrects the order of type, template arguments, nullable type specification and array specifications.
- Ensures array specification support all forms (simple arrays, multi-dimensional arrays, arrays of arrays).
@seraku24
Copy link
Contributor

(Off-topic) Whew. First time forking, committing, and submitting a pull request. Here is hoping I've crossed my I's and dotted by T's. (:

@ivanz ivanz deleted the fix-highlighting-md-arrays branch August 26, 2016 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants