Metadata tooltip fixes

MonoDevelop.MSBuild.Editor/DisplayElementFactory.cs

@@ -368,7 +368,7 @@ KnownImages GetPackageImageId (FeedKind kind)
 			case TargetInfo:
 				return isPrivate ? KnownImages.MSBuildTargetPrivate : KnownImages.MSBuildTarget;
 			case MetadataInfo:
-				return isPrivate ? KnownImages.MSBuildMetadata : KnownImages.MSBuildMetadataPrivate;
+				return isPrivate ? KnownImages.MSBuildMetadataPrivate : KnownImages.MSBuildMetadata;
 			case TaskInfo:
 				return KnownImages.MSBuildTask;
 			case ConstantSymbol:

MonoDevelop.MSBuild.Tests/MSBuildIdentifierTests.cs

@@ -0,0 +1,98 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using MonoDevelop.MSBuild.Language;
+using MonoDevelop.MSBuild.Language.Syntax;
+using MonoDevelop.MSBuild.Language.Typesystem;
+
+using NUnit.Framework;
+
+namespace MonoDevelop.MSBuild.Tests;
+
+[TestFixture]
+class MSBuildIdentifierTests
+{
+	[TestCase ("_FooBar", true)]
+	[TestCase ("FooBar", true)]
+	[TestCase ("fooBar", true)]
+	[TestCase ("foo", true)]
+	[TestCase ("Foo", true)]
+	[TestCase ("", false)]
+	[TestCase ("-FooBar", false)]
+	[TestCase (" FooBar", false)]
+	[TestCase ("FooBar ", false)]
+	[TestCase ("Foo Bar", false)]
+	[TestCase ("1Foo", false)]
+	[TestCase ("\"Foo", false)]
+	public void TestValidIdentifiers (string identifier, bool isValid)
+	{
+		bool actual = MSBuildIdentifier.IsValid (identifier);
+		Assert.AreEqual (isValid, actual);
+	}
+
+	[TestCase ("IsFoo", "Is")]
+	[TestCase ("_IsFoo", "Is")]
+	[TestCase ("_HasFoo", "Has")]
+	[TestCase ("Foo", null)]
+	[TestCase ("_Foo", null)]
+	[TestCase ("__Foo", null)]
+	[TestCase (" Foo", null)]
+	public void TestGetPrefix (string identifier, string prefix)
+	{
+		bool success = MSBuildIdentifier.TryGetPrefix (identifier, out var actual);
+		Assert.AreEqual (prefix is not null, success);
+		Assert.AreEqual (prefix, actual);
+	}
+
+	[TestCase ("FooPath", "Path")]
+	[TestCase ("_FooEnabled", "Enabled")]
+	[TestCase ("_PineappleOn", "On")]
+	[TestCase ("Foo", null)]
+	[TestCase ("Foo_", null)]
+	[TestCase ("_Cat", null)]
+	[TestCase ("__Mouse_", null)]
+	[TestCase ("Walrus ", null)]
+	public void TestGetSuffix (string identifier, string suffix)
+	{
+		bool success = MSBuildIdentifier.TryGetSuffix (identifier, out var actual);
+		Assert.AreEqual (suffix is not null, success);
+		Assert.AreEqual (suffix, actual);
+	}
+
+	[TestCase ("Enable", MSBuildValueKind.Unknown)]
+	[TestCase ("EnableFoo", MSBuildValueKind.Bool)]
+	[TestCase ("_EnableFoo", MSBuildValueKind.Bool)]
+	[TestCase ("DisableFoo", MSBuildValueKind.Bool)]
+	[TestCase ("RequireFoo", MSBuildValueKind.Bool)]
+	[TestCase ("UseFoo", MSBuildValueKind.Bool)]
+	[TestCase ("AllowFoo", MSBuildValueKind.Bool)]
+	[TestCase ("IsFoo", MSBuildValueKind.Bool)]
+	[TestCase ("HasFoo", MSBuildValueKind.Bool)]
+	[TestCase ("FooEnabled", MSBuildValueKind.Bool)]
+	[TestCase ("FooDisabled", MSBuildValueKind.Bool)]
+	[TestCase ("FooRequired", MSBuildValueKind.Bool)]
+	[TestCase ("RequiredFoo", MSBuildValueKind.Unknown)]
+	[TestCase ("FooDependsOn", MSBuildValueKind.TargetName | MSBuildValueKind.ListSemicolon)]
+	[TestCase ("FooPath", MSBuildValueKind.FileOrFolder)]
+	[TestCase ("FooPaths", MSBuildValueKind.FileOrFolder | MSBuildValueKind.ListSemicolon)]
+	[TestCase ("FooDirectory", MSBuildValueKind.Folder)]
+	[TestCase ("FooDir", MSBuildValueKind.Folder)]
+	[TestCase ("FooFile", MSBuildValueKind.File)]
+	[TestCase ("FooFileName", MSBuildValueKind.Filename)]
+	[TestCase ("FooFilename", MSBuildValueKind.Filename)]
+	[TestCase ("FooUrl", MSBuildValueKind.Url)]
+	[TestCase ("FooUri", MSBuildValueKind.Url)]
+	[TestCase ("FooExt", MSBuildValueKind.Extension)]
+	[TestCase ("FooGuid", MSBuildValueKind.Guid)]
+	[TestCase ("FooDirectories", MSBuildValueKind.Folder | MSBuildValueKind.ListSemicolon)]
+	[TestCase ("FooDirs", MSBuildValueKind.Folder | MSBuildValueKind.ListSemicolon)]
+	[TestCase ("FooFiles", MSBuildValueKind.File | MSBuildValueKind.ListSemicolon)]
+	// note: these two are only valid for Property while the rest are valid for Property and Metadata
+	[TestCase ("Configuration", MSBuildValueKind.Configuration)]
+	[TestCase ("Platform", MSBuildValueKind.Platform)]
+	public void TestInferKind (string identifier, MSBuildValueKind kind)
+	{
+		var actual = MSBuildIdentifier.InferValueKind (identifier, MSBuildSyntaxKind.Property);
+		Assert.AreEqual (kind, actual);
+	}
+}

MonoDevelop.MSBuild/Language/Expressions/ExpressionParser.cs

@@ -176,7 +176,7 @@ static ExpressionNode ParseItem (string buffer, ref int offset, int endOffset, i
 
 			ConsumeWhitespace (ref offset);
 
-			string name = ReadMSBuildIdentifier (buffer, ref offset, endOffset);
+			string name = MSBuildIdentifier.TryRead (buffer, ref offset, endOffset);
 			if (name == null) {
 				return new ExpressionError (baseOffset + offset, ExpressionErrorKind.ExpectingItemName, out hasError);
 			}
@@ -349,39 +349,6 @@ static string TryReadNameAsciiLettersOnly (string buffer, ref int offset, int en
 			static bool IsAsciiLetter (char ch) => (ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z');
 		}
 
-		static string ReadMSBuildIdentifier (string buffer, ref int offset, int endOffset)
-		{
-			if (offset > endOffset) {
-				return null;
-			}
-
-			int start = offset;
-			char ch = buffer [offset];
-			if (!char.IsLetter (ch) && ch != '_') {
-				return null;
-			}
-			offset++;
-
-			char lastChar = ch;
-			while (offset <= endOffset) {
-				ch = buffer [offset];
-				if (!char.IsLetterOrDigit (ch) && ch != '_' && ch != '-') {
-					break;
-				}
-				offset++;
-				lastChar = ch;
-			}
-
-			// Although a dash is a valid char for MSBuild identifiers, we will disallow the last char being a dash
-			// as this will conflict with item transform handling.
-			// We could probably allow this if we special cased it but I really can't see a good reason to do so.
-			if (lastChar == '-') {
-				offset--;
-			}
-
-			return buffer.Substring (start, offset - start);
-		}
-
 		// TODO: improve and unify with the CLR identifier validation in the validator
 		static string ReadClrIdentifier (string buffer, ref int offset, int endOffset)
 		{
@@ -720,7 +687,7 @@ static ExpressionNode ParseProperty (string buffer, ref int offset, int endOffse
 					return new ExpressionProperty (baseOffset + start, offset - start, propRef);
 				}
 			} else {
-				string name = ReadMSBuildIdentifier (buffer, ref offset, endOffset);
+				string name = MSBuildIdentifier.TryRead (buffer, ref offset, endOffset);
 				if (name == null) {
 					return new ExpressionError (baseOffset + offset, ExpressionErrorKind.ExpectingPropertyName, out hasError);
 				}
@@ -1037,7 +1004,7 @@ static ExpressionNode ParseMetadata (string buffer, ref int offset, int endOffse
 
 			ConsumeSpace (buffer, ref offset, endOffset);
 
-			string name = ReadMSBuildIdentifier (buffer, ref offset, endOffset);
+			string name = MSBuildIdentifier.TryRead (buffer, ref offset, endOffset);
 			if (name == null) {
 				return new ExpressionError (baseOffset + offset, ExpressionErrorKind.ExpectingMetadataOrItemName, out hasError);
 			}
@@ -1076,7 +1043,7 @@ out hasError
 
 			ConsumeSpace (buffer, ref offset, endOffset);
 
-			string metadataName = ReadMSBuildIdentifier (buffer, ref offset, endOffset);
+			string metadataName = MSBuildIdentifier.TryRead (buffer, ref offset, endOffset);
 			if (metadataName == null) {
 				return new IncompleteExpressionError (
 					baseOffset + offset, offset > endOffset, ExpressionErrorKind.ExpectingMetadataName,