Improve nullability and XName comparison

Core/Dom/XAttribute.cs

@@ -26,26 +26,38 @@
 
 #nullable enable
 
+using System.Diagnostics.CodeAnalysis;
+
 namespace MonoDevelop.Xml.Dom
 {
 	public class XAttribute : XObject, INamedXObject
 	{
-
-		public XAttribute (int startOffset, XName name, string value) : base (startOffset)
+		public XAttribute (int startOffset, XName name, string value, int valueOffset) : base (startOffset)
 		{
 			Name = name;
 			Value = value;
+			ValueOffset = valueOffset;
 		}
 
 		public XAttribute (int startOffset) : base (startOffset) { }
 
 		public XName Name { get; set; }
 
-		public override bool IsComplete => base.IsComplete && IsNamed;
+		[MemberNotNullWhen (true, nameof (Value), nameof (ValueOffset), nameof (ValueSpan))]
+		public override bool IsComplete => base.IsComplete && IsNamed && HasValue;
 
 		public bool IsNamed => Name.IsValid;
 
-		public string? Value { get; set; }
+		public string? Value { get; private set; }
+
+		public int? ValueOffset { get; private set; }
+
+		[MemberNotNull(nameof(Value), nameof(ValueOffset))]
+		internal void SetValue (int offset, string value)
+		{
+			ValueOffset = offset;
+			Value = value;
+		}
 
 		public XAttribute? NextSibling { get; internal protected set; }
 
@@ -59,6 +71,7 @@ protected override void ShallowCopyFrom (XObject copyFrom)
 			//immutable types
 			Name = copyFromAtt.Name;
 			Value = copyFromAtt.Value;
+			ValueOffset = copyFromAtt.ValueOffset;
 		}
 
 		public override string ToString () => $"[XAttribute Name='{Name.FullName}' Location='{Span}' Value='{Value}']";
@@ -67,10 +80,28 @@ protected override void ShallowCopyFrom (XObject copyFrom)
 
 		public TextSpan NameSpan => new (Span.Start, Name.Length);
 
-		int ValueLength => Value?.Length ?? 0;
+		public TextSpan? ValueSpan => HasValue? new (ValueOffset.Value, Value.Length) : null;
 
-		public int ValueOffset => Span.End - ValueLength - 1;
+		// value nullability helpers
 
-		public TextSpan ValueSpan => new (Span.End - ValueLength - 1, ValueLength);
+		[MemberNotNullWhen (true, nameof (Value), nameof (ValueOffset), nameof (ValueSpan))]
+		public bool HasValue => Value is not null;
+
+		[MemberNotNullWhen (true, nameof (Value), nameof (ValueOffset), nameof (ValueSpan))]
+		public bool HasNonEmptyValue => !string.IsNullOrEmpty (Value);
+
+		[MemberNotNullWhen (true, nameof (Value), nameof (ValueOffset), nameof (ValueSpan))]
+		public bool TryGetValue ([NotNullWhen (true)] out string? value)
+		{
+			value = Value;
+			return HasValue;
+		}
+
+		[MemberNotNullWhen (true, nameof (Value), nameof (ValueOffset), nameof (ValueSpan))]
+		public bool TryGetNonEmptyValue ([NotNullWhen (true)] out string? value)
+		{
+			value = Value;
+			return HasNonEmptyValue;
+		}
 	}
 }

Core/Dom/XAttributeCollection.cs

@@ -82,11 +82,11 @@ public XAttribute? this [int index] {
 			}
 		}
 
-		public XAttribute? Get (XName name, bool ignoreCase)
+		public XAttribute? Get (XName name, bool ignoreCase = false)
 		{
 			XAttribute? current = First;
 			while (current is not null) {
-				if (XName.Equals (current.Name, name, ignoreCase))
+				if (current.Name.Equals (name, ignoreCase))
 					return current;
 				current = current.NextSibling;
 			}

Core/Dom/XName.cs

@@ -31,7 +31,7 @@
 
 namespace MonoDevelop.Xml.Dom
 {
-	public struct XName : IEquatable<XName>
+	public readonly struct XName : IEquatable<XName>, IComparable<XName>
 	{
 		public XName (string prefix, string name)
 		{
@@ -53,38 +53,56 @@ public XName (string name)
 
 		public string? FullName => Prefix == null ? Name : Prefix + ':' + Name;
 
-		[MemberNotNullWhen(true, nameof(Name), nameof (FullName))]
+		[MemberNotNullWhen (true, nameof (Name), nameof (FullName))]
 		public bool IsValid { get { return !string.IsNullOrEmpty (Name); } }
 
-		[MemberNotNullWhen(true, nameof(Prefix))]
+		[MemberNotNullWhen (true, nameof (Prefix))]
 		public bool HasPrefix { get { return !string.IsNullOrEmpty (Prefix); } }
 
 		public int Length => (Name?.Length ?? 0) + (Prefix != null ? Prefix.Length + 1 : 0);
 
-		#region Equality
-
 		public static bool operator == (XName x, XName y) => x.Equals (y);
 
 		public static bool operator != (XName x, XName y) => !x.Equals (y);
 
-		public bool Equals (XName other) => Prefix == other.Prefix && Name == other.Name;
+		public bool Equals (XName other) => Equals (other, false);
+
+		public bool Equals (XName other, bool ignoreCase)
+		{
+			var comparison = ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
+			return string.Equals (Prefix, other.Prefix, comparison) && string.Equals (Name, other.Name, comparison);
+		}
 
 		public override bool Equals (object? obj) => (obj is not XName other) || Equals (other);
 
 		public override int GetHashCode ()
 		{
-			int hash = 0;
-			if (Prefix != null) hash += Prefix.GetHashCode ();
-			if (Name != null) hash += Name.GetHashCode ();
-			return hash;
+			int prefixHash = Prefix is null ? 0 : Prefix.GetHashCode ();
+			int nameHash = Name is null ? 0 : Name.GetHashCode ();
+			return HashCode.Combine (prefixHash, nameHash);
+		}
+
+		// exposed via XNameComparer but implemented here so all GetHashCode impls are in one place
+		internal int GetHashCode (bool ignoreCase) => ignoreCase ? GetHashCodeIgnoreCase () : GetHashCode ();
+
+		readonly int GetHashCodeIgnoreCase ()
+		{
+			var comparer = StringComparer.OrdinalIgnoreCase;
+			int prefixHash = Prefix is null ? 0 : comparer.GetHashCode (Prefix);
+			int nameHash = Name is null ? 0 : comparer.GetHashCode (Name);
+			return HashCode.Combine (prefixHash, nameHash);
 		}
 
-		#endregion
+		public int CompareTo (XName other) => CompareTo (other, false);
 
-		public static bool Equals (XName a, XName b, bool ignoreCase)
+		public int CompareTo (XName other, bool ignoreCase)
 		{
-			var comp = ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
-			return string.Equals (a.Prefix, b.Prefix, comp) && string.Equals (a.Name, b.Name, comp);
+			var comparison = ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
+			var prefix = string.Compare (Prefix, other.Prefix, comparison);
+			if (prefix != 0) {
+				return prefix;
+			}
+			return string.Compare (Name, other.Name, comparison);
 		}
 
 		public override string ToString ()
@@ -94,6 +112,6 @@ public override string ToString ()
 			return Prefix + ":" + Name;
 		}
 
-		public static implicit operator XName (string name) => new(name);
+		public static implicit operator XName (string name) => new (name);
 	}
 }

Core/Dom/XNameComparer.cs

@@ -0,0 +1,28 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+#nullable enable
+
+using System.Collections.Generic;
+
+namespace MonoDevelop.Xml.Dom;
+
+public class XNameComparer : IComparer<XName>, IEqualityComparer<XName>
+{
+	readonly bool ignoreCase;
+
+	XNameComparer (bool ignoreCase)
+	{
+		this.ignoreCase = ignoreCase;
+	}
+
+	public static XNameComparer Ordinal { get; } = new XNameComparer (false);
+
+	public static XNameComparer OrdinalIgnoreCase { get; } = new XNameComparer (true);
+
+	public int Compare (XName x, XName y) => x.CompareTo (y, ignoreCase);
+
+	public bool Equals (XName x, XName y) => x.Equals (y, ignoreCase);
+
+	public int GetHashCode (XName obj) => obj.GetHashCode (ignoreCase);
+}

Core/Dom/XmlDomExtensions.cs

@@ -16,12 +16,6 @@ public static TextSpan GetSquiggleSpan (this XNode node)
 			return node is XElement el ? el.NameSpan : node.NextSibling?.Span ?? node.Span;
 		}
 
-		public static bool NameEquals (this INamedXObject obj, string name, bool ignoreCase)
-		{
-			var comparison = ignoreCase ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
-			return !obj.Name.HasPrefix && string.Equals (obj.Name.Name, name, comparison);
-		}
-
 		public static bool IsTrue (this XAttributeCollection attributes, string name)
 		{
 			var att = attributes.Get (name, true);

Core/Logging/UserIdentifiable.cs

@@ -42,12 +42,12 @@ public interface IUserIdentifiable<TValue> : IUserIdentifiableValue
 /// Loggers that strip PII must hash this value unless it can be reliably determined
 /// to be a non-identifiable value such a filename that is part of the app itself.
 /// </summary>
-public readonly struct UserIdentifiableFileName : IUserIdentifiable<string>
+public readonly struct UserIdentifiableFileName : IUserIdentifiable<string?>
 {
-	public string Value { get; }
-	public UserIdentifiableFileName (string filename) { Value = filename; }
-	public static implicit operator UserIdentifiableFileName (string filename) => new (filename);
-	public override readonly string ToString () => Value;
+	public string? Value { get; }
+	public UserIdentifiableFileName (string? filename) { Value = filename; }
+	public static implicit operator UserIdentifiableFileName (string? filename) => new (filename);
+	public override readonly string ToString () => Value ?? "[null]";
 
 	object? IUserIdentifiableValue.Value => Value;
 }

Core/MonoDevelop.Xml.Core.csproj

@@ -17,5 +17,6 @@
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="7.0.1" />
     <PackageReference Include="System.Collections.Immutable" Version="7.0.0" />
+    <PackageReference Include="Microsoft.Bcl.HashCode" Version="1.1.1" Condition="'$(TargetFramework)'=='net48'" />
   </ItemGroup>
 </Project>

Core/Parser/XmlAttributeValueState.cs

@@ -50,7 +50,7 @@ public class XmlAttributeValueState : XmlParserState
 			if (c == '<') {
 				//the parent state should report the error
 				var att = (XAttribute)context.Nodes.Peek ();
-				att.Value = context.KeywordBuilder.ToString ();
+				att.SetValue (context.Position - context.CurrentStateLength, context.KeywordBuilder.ToString ());
 				replayCharacter = true;
 				return Parent;
 			}
@@ -59,7 +59,7 @@ public class XmlAttributeValueState : XmlParserState
 				//the parent state should report the error
 				context.Diagnostics?.Add (XmlCoreDiagnostics.IncompleteAttributeEof, context.PositionBeforeCurrentChar);
 				var att = (XAttribute)context.Nodes.Peek ();
-				att.Value = context.KeywordBuilder.ToString ();
+				att.SetValue (context.Position - context.CurrentStateLength, context.KeywordBuilder.ToString ());
 				return Parent;
 			}
 
@@ -84,7 +84,7 @@ public class XmlAttributeValueState : XmlParserState
 			if ((c == '"' && maskedTag == DOUBLEQUOTE) || c == '\'' && maskedTag == SINGLEQUOTE) {
 				//ending the value
 				var att = (XAttribute) context.Nodes.Peek ();
-				att.Value = context.KeywordBuilder.ToString ();
+				att.SetValue (context.Position - context.KeywordBuilder.Length, context.KeywordBuilder.ToString ());
 				return Parent;
 			}
 
@@ -112,7 +112,7 @@ public class XmlAttributeValueState : XmlParserState
 			}
 
 			var att = (XAttribute)context.Nodes.Peek ();
-			att.Value = context.KeywordBuilder.ToString ();
+			att.SetValue (context.Position - context.CurrentStateLength, context.KeywordBuilder.ToString ());
 
 			if (context.Diagnostics is not null && att.Name.IsValid) {
 				context.Diagnostics.Add (XmlCoreDiagnostics.UnquotedAttributeValue, new TextSpan (context.Position - att.Value.Length, att.Value.Length), att.Name.FullName);

Editor.Tests/Completion/XmlCompletionTestSource.cs

@@ -59,13 +59,15 @@ public XmlCompletionTestSource (ITextView textView, ILogger logger, XmlParserPro
 			var item = new CompletionItem (includeBracket? "<Hello" : "Hello", this)
 				.AddKind (XmlCompletionItemKind.Element);
 			var items = new List<CompletionItem> () { item };
-			items.AddRange (GetMiscellaneousTags (context.TriggerLocation, context.NodePath, includeBracket));
+			if (context.NodePath is not null) {
+				items.AddRange (GetMiscellaneousTags (context.TriggerLocation, context.NodePath, includeBracket));
+			}
 			return Task.FromResult<IList<CompletionItem>?> (items);
 		}
 
 		protected override Task<IList<CompletionItem>?> GetAttributeCompletionsAsync (XmlCompletionTriggerContext context, IAttributedXObject attributedObject, Dictionary<string, string> existingAtts, CancellationToken token)
 		{
-			if (context.NodePath.LastOrDefault () is XElement xel && xel.NameEquals ("Hello", true)) {
+			if (context.NodePath?.LastOrDefault () is XElement xel && xel.Name.Equals ("Hello", true)) {
 				var item = new CompletionItem ("There", this)
 					.AddKind (XmlCompletionItemKind.Attribute);
 				var items = new List<CompletionItem> () {  item };

Editor.Tests/Extensions/CompletionTestExtensions.cs

@@ -18,8 +18,7 @@ public static Task<CompletionContext> GetCompletionContext (
 			this EditorTest test,
 			string documentText, CompletionTriggerReason reason = default, char triggerChar = '\0', char caretMarker = '$', string filename = default, CancellationToken cancellationToken = default)
 		{
-			int caretOffset;
-			(documentText, caretOffset) = SelectionHelper.ExtractCaret (documentText, caretMarker);
+			(documentText, var caretOffset) = TextWithMarkers.ExtractSinglePosition (documentText, caretMarker);
 
 			var textView = test.CreateTextView (documentText, filename);
 

Editor.Tests/Extensions/EditorCommandExtensions.cs

@@ -44,8 +44,9 @@ public static Task TestCommands (
 			Func<ITextView, Task> initialize = null,
 			CancellationToken cancellationToken = default)
 		{
-			(beforeDocumentText, int beforeCaretOffset) = SelectionHelper.ExtractCaret (beforeDocumentText, caretMarkerChar);
-			(afterDocumentText, int afterCaretOffset) = SelectionHelper.ExtractCaret (afterDocumentText, caretMarkerChar);
+			(beforeDocumentText, int beforeCaretOffset) = TextWithMarkers.ExtractSinglePosition (beforeDocumentText, caretMarkerChar);
+			(afterDocumentText, int afterCaretOffset) = TextWithMarkers.ExtractSinglePosition (afterDocumentText, caretMarkerChar);
+
 			return test.TestCommands (
 				beforeDocumentText, beforeCaretOffset,
 				afterDocumentText, afterCaretOffset,

Editor/Completion/AbstractCompletionCommitManager.cs

@@ -90,7 +90,7 @@ CommitResult TryCommitInternal (IAsyncCompletionSession session, ITextBuffer buf
 				if (!IsCommitCharForTriggerKind (trigger, session, buffer.CurrentSnapshot, typedChar)) {
 					return CommitCancel;
 				}
-			} else if (item.CommitCharacters.Contains (typedChar)) {
+			} else if (!item.CommitCharacters.Contains (typedChar)) {
 				return CommitCancel;
 			}
 		}

Editor/Completion/XmlCompletionCommitManager.cs

@@ -73,7 +73,7 @@ protected override CommitResult TryCommitItemKind (XmlCompletionItemKind itemKin
 
 		switch (itemKind) {
 		case XmlCompletionItemKind.SelfClosingElement: {
-				//comitting self-closing element with > makes it non-self-closing
+				//committing self-closing element with > makes it non-self-closing
 				if (typedChar == '>') {
 					goto case XmlCompletionItemKind.Element;
 				}

Editor/Completion/XmlCompletionSource.cs

@@ -193,7 +193,7 @@ protected virtual CompletionStartData InitializeCompletion (CompletionTrigger tr
 
 		protected virtual Task<IList<CompletionItem>?> GetDeclarationCompletionsAsync (TCompletionTriggerContext context, CancellationToken token)
 			=> TaskCompleted (
-				context.NodePath.Any (n => n is XElement)
+				context.NodePath is not null && context.NodePath.Any (n => n is XElement)
 					? new [] { cdataItemWithBracket, commentItemWithBracket }
 					: new [] { commentItemWithBracket }
 				);

Editor/Completion/XmlCompletionTriggerContext.cs

@@ -51,7 +51,7 @@ public virtual Task InitializeNodePath (ILogger logger, CancellationToken cancel
 
 		// if we're completing an existing element, remove it from the path
 		// so we don't get completions for its children instead
-		if ((XmlTriggerKind == XmlCompletionTrigger.ElementName || XmlTriggerKind == XmlCompletionTrigger.Tag) && nodePath.Count > 0) {
+		if ((XmlTriggerKind == XmlCompletionTrigger.ElementName || XmlTriggerKind == XmlCompletionTrigger.Tag) && nodePath?.Count > 0) {
 			if (nodePath[nodePath.Count - 1] is XElement leaf && leaf.Name.Length == ApplicableToSpan.Length) {
 				nodePath.RemoveAt (nodePath.Count - 1);
 			}
@@ -74,7 +74,7 @@ public virtual Task InitializeNodePath (ILogger logger, CancellationToken cancel
 	internal XmlCompletionTrigger XmlTriggerKind { get; }
 	internal XmlTriggerReason XmlTriggerReason { get; }
 
-	public List<XObject> NodePath { get; private set; }
+	public List<XObject>? NodePath { get; private set; }
 
 	internal static XmlTriggerReason ConvertReason (CompletionTriggerReason reason, char typedChar)
 	{