Add new csharp-type type and rework type name validation

Fixes #253 Using Include validation is incorrect for types that
have CLR generics

MonoDevelop.MSBuild.Tests/Analyzers/CoreDiagnosticTests.cs

@@ -656,5 +656,33 @@ public void SdkNameExpressionWithProperty ()
 				ignoreDiagnostics: [CoreDiagnostics.UnwrittenProperty]
 			);
 		}
+
+		[Test]
+		public void InvalidCSharpType ()
+		{
+			var source = TextWithMarkers.Parse (@"<Project>
+<PropertyGroup>
+	<SomeType>|Foo-Bar|</SomeType>
+	<SomeType>System.Collections.Generic.Dictionary%3CMicrosoft.NET.Sdk.WorkloadManifestReader.WorkloadId, Microsoft.NET.Sdk.WorkloadManifestReader.WorkloadDefinition%3E</SomeType>
+</PropertyGroup>
+</Project>", '|');
+
+			var schema = new MSBuildSchema {
+				new PropertyInfo ("SomeType", "", MSBuildValueKind.CSharpType)
+			};
+
+			var spans = source.GetMarkedSpans ('|');
+
+			var expected = new MSBuildDiagnostic (
+				CoreDiagnostics.InvalidCSharpType,
+				spans[0],
+				messageArgs: [ "Foo-Bar" ]
+			);
+
+			VerifyDiagnostics (source.Text, out _,
+				includeCoreDiagnostics: true,
+				expectedDiagnostics: [ expected ],
+				schema: schema);
+		}
 	}
 }

MonoDevelop.MSBuild/Language/CoreDiagnostics.cs

@@ -511,10 +511,17 @@ class CoreDiagnostics
 		public const string InvalidClrType_Id = nameof (InvalidClrType);
 		public static readonly MSBuildDiagnosticDescriptor InvalidClrType = new (
 			InvalidClrType_Id,
-			"Invalid qualified .NET type name",
+			"Invalid .NET type name",
 			"The value `{0}` is not a valid qualified .NET type name string",
 			MSBuildDiagnosticSeverity.Error);
 
+		public const string InvalidCSharpType_Id = nameof (InvalidCSharpType);
+		public static readonly MSBuildDiagnosticDescriptor InvalidCSharpType = new (
+			InvalidCSharpType_Id,
+			"Invalid C# type name",
+			"The value `{0}` is not a valid qualified C# type name string",
+			MSBuildDiagnosticSeverity.Error);
+
 		public const string InvalidClrTypeName_Id = nameof (InvalidClrTypeName);
 		public static readonly MSBuildDiagnosticDescriptor InvalidClrTypeName = new (
 			InvalidClrTypeName_Id,

MonoDevelop.MSBuild/Language/MSBuildDocumentValidator.cs

@@ -857,22 +857,28 @@ void VisitPureLiteral (MSBuildElementSyntax elementSyntax, MSBuildAttributeSynta
 					break;
 				}
 			case MSBuildValueKind.ClrNamespace:
-				if (!IsValidTypeOrNamespace (value, out _)) {
+				if (!TypeNameValidation.IsValidClrNamespace (value)) {
 					AddErrorWithArgs (CoreDiagnostics.InvalidClrNamespace, value);
 				}
 				break;
 
 			case MSBuildValueKind.ClrType:
-				if (!IsValidTypeOrNamespace (value, out _)) {
+				if (!TypeNameValidation.IsValidClrTypeOrNamespace (value, out _, out _)) {
 					AddErrorWithArgs (CoreDiagnostics.InvalidClrType, value);
 				}
 				break;
 
 			case MSBuildValueKind.ClrTypeName:
-				if (!(IsValidTypeOrNamespace (value, out int componentCount) && componentCount == 1)) {
+				if (!TypeNameValidation.IsValidClrTypeName (value)) {
 					AddErrorWithArgs (CoreDiagnostics.InvalidClrTypeName, value);
 				}
 				break;
+
+			case MSBuildValueKind.CSharpType:
+				if (!TypeNameValidation.IsValidCSharpTypeOrNamespace (value)) {
+					AddErrorWithArgs (CoreDiagnostics.InvalidCSharpType, value);
+				}
+				break;
 			}
 
 			void AddErrorWithArgs (MSBuildDiagnosticDescriptor d, params object[] args) => Diagnostics.Add (d, new TextSpan (trimmedOffset, escapedLength), args);
@@ -889,18 +895,6 @@ void AddMisspelledValueError (MSBuildDiagnosticDescriptor d, params object[] arg
 					args
 				);
 			}
-
-			static bool IsValidTypeOrNamespace (string value, out int componentCount)
-			{
-				string[] components = value.Split ('.');
-				componentCount = components.Length;
-				foreach (var component in components) {
-					if (!System.CodeDom.Compiler.CodeGenerator.IsValidLanguageIndependentIdentifier (component)) {
-						return false;
-					}
-				}
-				return true;
-			}
 		}
 
 		void CheckPropertyWrite (PropertyInfo resolvedProperty, TextSpan span)

MonoDevelop.MSBuild/Language/TypeNameValidation.cs

@@ -0,0 +1,230 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+#if NETCOREAPP
+#nullable enable
+#endif
+
+using System.Globalization;
+
+namespace MonoDevelop.MSBuild.Language;
+
+static class TypeNameValidation
+{
+	public static bool IsValidCSharpTypeOrNamespace (string value) => IsValidCSharpTypeOrNamespace(value, out _, out _);
+
+	public static bool IsValidCSharpTypeOrNamespace (string value, out int componentCount, out bool isGenericType)
+	{
+		int offset = 0;
+		if (!ConsumeCSharpGenericTypeOrNamespace(value, ref offset, out componentCount, out isGenericType)) {
+			return false;
+		}
+		return offset == value.Length;
+	}
+
+
+	static bool ConsumeCSharpGenericTypeOrNamespace (string value, ref int offset, out int componentCount, out bool isGenericType)
+	{
+		isGenericType = false;
+
+		componentCount = ConsumeIdentifiers(value, ref offset);
+
+		if (componentCount == 0) {
+			return false;
+		}
+
+		if (offset == value.Length) {
+			return true;
+		}
+
+		// try to consume generics in the form SomeType<SomeOtherType,AnotherType>
+		if (value[offset] != '<') {
+			// complete but not a generic type, we are done consuming, caller decides what to do with next char
+			return true;
+		}
+		offset++;
+
+		isGenericType = true;
+		do {
+			// Ignore whitespace after a comma as that's pretty common e.g. SomeType<SomeOtherType, AnotherType>
+			// and is valid if this is going to be injected verbatim into a C# file.
+			// We could be more strict and disallow whitespace, but that will likely trip people up.
+			// We could also be more liberal and allow general whitespace around the angle brackets and commas,
+			// but that's a bit of a rabbit hole and not likely to be very useful.
+			if (value[offset] == ',') {
+				offset++;
+				ConsumeSpaces(value, ref offset);
+			}
+			if (!ConsumeCSharpGenericTypeOrNamespace(value, ref offset, out int consumedComponents, out _)) {
+				return false;
+			}
+		} while (value[offset] == ',');
+
+		if (offset < value.Length && value[offset] == '>') {
+			offset++;
+			return true;
+		}
+
+		return false;
+	}
+
+	static void ConsumeSpaces(string value, ref int offset)
+	{
+		while (value[offset] == ' ') {
+			offset++;
+		}
+	}
+
+	public static bool IsValidClrNamespace (string value) => IsValidClrTypeOrNamespace(value, out _, out var hasGenerics) && !hasGenerics;
+
+	public static bool IsValidClrTypeName (string value) => IsValidClrTypeOrNamespace(value, out var componentCount, out var hasGenerics) && componentCount == 1 && !hasGenerics;
+
+	public static bool IsValidClrTypeOrNamespace (string value) => IsValidClrTypeOrNamespace(value, out _, out _);
+
+	public static bool IsValidClrTypeOrNamespace (string value, out int componentCount, out bool isGenericType)
+	{
+		int offset = 0;
+		if (!ConsumeClrGenericTypeOrNamespace(value, ref offset, out componentCount, out isGenericType)) {
+			return false;
+		}
+		return offset == value.Length;
+	}
+
+	static bool ConsumeClrGenericTypeOrNamespace (string value, ref int offset, out int componentCount, out bool isGenericType)
+	{
+		isGenericType = false;
+
+		componentCount = ConsumeIdentifiers(value, ref offset);
+
+		if (componentCount == 0) {
+			return false;
+		}
+
+		if (offset == value.Length) {
+			return true;
+		}
+
+		// try to consume generics in the form SomeType`2[SomeOtherType,AnotherType]
+		if (value[offset] != '`') {
+			// complete but not a generic type, we are done consuming, caller decides what to do with next char
+			return true;
+		}
+		offset++;
+
+		int argCountOffset = offset;
+		if(!ConsumeNumbers(value, ref offset) || !int.TryParse(value.Substring(argCountOffset, offset - argCountOffset), out int genericArgCount)) {
+			return false;
+		}
+
+		isGenericType = true;
+		if (value[offset++] != '[') {
+			return false;
+		}
+
+		for (int i = 0; i < genericArgCount; i++) {
+			// recursively consume the generic type arguments
+			if (!ConsumeClrGenericTypeOrNamespace(value, ref offset, out int consumedComponents, out _)) {
+				return false;
+			}
+			if (i < genericArgCount - 1) {
+				if (offset < value.Length && value[offset] == ',') {
+					offset++;
+				} else {
+					return false;
+				}
+			}
+		}
+
+		if (offset < value.Length && value[offset] == ']') {
+			offset++;
+			return true;
+		}
+
+		return false;
+	}
+
+	static bool ConsumeNumbers (string value, ref int offset)
+	{
+		int start = offset;
+		while(char.IsDigit(value[offset])) {
+			offset++;
+		}
+		return offset > start;
+	}
+
+	static int ConsumeIdentifiers (string value, ref int offset)
+	{
+		int componentCount = 0;
+
+		while(ConsumeIdentifier(value, ref offset)) {
+			componentCount++;
+			if (offset >= value.Length || value[offset] != '.') {
+				return componentCount;
+			}
+			offset++;
+		}
+
+		if (componentCount > 0 && value[offset - 1] == '.') {
+			// we consumed a dot but no identifier followed it, so walk it back
+			offset--;
+		}
+
+		return componentCount;
+	}
+
+	static bool ConsumeIdentifier (string value, ref int offset)
+	{
+		bool nextMustBeStartChar = true;
+
+		while (offset < value.Length) {
+			char ch = value[offset];
+			// based on https://github.com/dotnet/runtime/blob/96be510135829ff199c6c341ad461c36bab4809e/src/libraries/Common/src/System/CSharpHelpers.cs#L141
+			UnicodeCategory uc = CharUnicodeInfo.GetUnicodeCategory(ch);
+			switch (uc) {
+				case UnicodeCategory.UppercaseLetter:        // Lu
+				case UnicodeCategory.LowercaseLetter:        // Ll
+				case UnicodeCategory.TitlecaseLetter:        // Lt
+				case UnicodeCategory.ModifierLetter:         // Lm
+				case UnicodeCategory.LetterNumber:           // Lm
+				case UnicodeCategory.OtherLetter:            // Lo
+					nextMustBeStartChar = false;
+					offset++;
+					continue;
+
+				case UnicodeCategory.NonSpacingMark:         // Mn
+				case UnicodeCategory.SpacingCombiningMark:   // Mc
+				case UnicodeCategory.ConnectorPunctuation:   // Pc
+				case UnicodeCategory.DecimalDigitNumber:     // Nd
+					// Underscore is a valid starting character, even though it is a ConnectorPunctuation.
+					if (nextMustBeStartChar && ch != '_')
+						return false;
+					offset++;
+					continue;
+
+				default:
+					// unlike CSharpHelpers.cs, don't check IsSpecialTypeChar here because we're only looking
+					// for identifier components that are valid for all languages, and generic syntax is
+					// handled separately.
+					//
+					break;
+			}
+			break;
+		}
+
+		// return true if we have been able to consume a valid identifier. it need
+		// not be the entire string.
+		return !nextMustBeStartChar;
+	}
+
+	public static bool IsValidCSharpType (string value, out int componentCount)
+	{
+		string[] components = value.Split ('.');
+		componentCount = components.Length;
+		foreach (var component in components) {
+			if (!System.CodeDom.Compiler.CodeGenerator.IsValidLanguageIndependentIdentifier (component)) {
+				return false;
+			}
+		}
+		return true;
+	}
+}

MonoDevelop.MSBuild/Language/Typesystem/MSBuildValueKind.cs

@@ -103,10 +103,35 @@ public enum MSBuildValueKind
 		NuGetVersion,
 
 		// .NET namespace and type names
+
+		/// <summary>
+		/// A .NET namespace name, in language-agnostic form.
+		/// </summary>
 		ClrNamespace,
+
+		/// <summary>
+		/// A .NET namespace or qualified type name, in language-agnostic form i.e. C# generic syntax is not allowed,
+		/// but CLR generic syntax <c>A`1[B,C]</c> is allowed.
+		/// </summary>
+		/// <remarks>
+		/// If you need to support C# generic syntax, use <see cref="CSharpType"/> instead.
+		/// </remarks>
 		ClrType,
+
+		/// <summary>
+		/// A .NET unqualified type name. May not include generics of any form. This is typically used for the name of
+		/// a type to be generated at build time.
+		/// </summary>
 		ClrTypeName,
 
+		/// <summary>
+		/// C# namespace or qualified type name. May include generics in C# format e.g. <c>MyNamespace.MyType&lt;int></c>.
+		/// </summary>
+		/// <remarks>
+		/// If you need .NET language-agnostic format, use <see cref="ClrType"/> instead.
+		/// </remarks>
+		CSharpType,
+
 		/// <summary>
 		/// Base type for warning codes. Tools should use a derived custom type to define their warning codes.
 		/// Anything of this type will receive completion and validation for all derived warning code types.

MonoDevelop.MSBuild/Schema/DescriptionFormatter.cs

@@ -286,6 +286,8 @@ static string FormatKind (MSBuildValueKind kind, CustomTypeInfo customTypeInfo)
 				return "clr-type-name";
 			case MSBuildValueKind.WarningCode:
 				return "warning-code";
+			case MSBuildValueKind.CSharpType:
+				return "csharp-type";
 			}
 			return null;
 		}

MonoDevelop.MSBuild/Schema/MSBuildSchema.TypeInfoLoader.cs

@@ -146,6 +146,7 @@ public bool TryHandle(string key, JToken token)
 		{ "clr-namespace", MSBuildValueKind.ClrNamespace },
 		{ "clr-type", MSBuildValueKind.ClrType },
 		{ "clr-type-name", MSBuildValueKind.ClrTypeName },
-		{ "warning-code", MSBuildValueKind.WarningCode }
+		{ "csharp-type", MSBuildValueKind.CSharpType },
+		{ "warning-code", MSBuildValueKind.WarningCode },
 	};
 }

MonoDevelop.MSBuild/Schemas/CSharp.buildschema.json

@@ -77,8 +77,8 @@
   "items": {
     "Using": {
       "description": "A C# global using to add to the project.",
-      "includeDescription": "The namespace or type identifier to add, e.g. Microsoft.AspNetCore",
-      "type": "clr-namespace",
+      "includeDescription": "The namespace or type identifier to add, e.g. `Microsoft.AspNetCore`",
+      "type": "csharp-type",
       "metadata": {
         "Alias": {
           "description": "Optional alias for the namespace or type.",