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

Make FilePathNormalizer.GetHashCode case insensitive #10050

Merged
merged 6 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public CohostProjectSnapshot(Project project, DocumentSnapshotFactory documentSn

_importsToRelatedDocumentsLazy = new Lazy<ImmutableDictionary<string, ImmutableArray<string>>>(() =>
{
var importsToRelatedDocuments = ImmutableDictionary.Create<string, ImmutableArray<string>>(FilePathNormalizer.Comparer);
var importsToRelatedDocuments = ImmutableDictionary.Create<string, ImmutableArray<string>>(FilePathNormalizingComparer.Instance);
foreach (var document in DocumentFilePaths)
{
var importTargetPaths = ProjectState.GetImportDocumentTargetPaths(document, FileKinds.GetFileKindFromFilePath(document), _lazyProjectEngine.Value);
Expand Down
davidwengier marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,17 @@

using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using Microsoft.CodeAnalysis.Razor;
#if !NET
using Microsoft.Extensions.Internal;
#endif

namespace Microsoft.AspNetCore.Razor.Utilities;

internal static class FilePathNormalizer
{
private static Lazy<IEqualityComparer<string>> _lazyComparer = new Lazy<IEqualityComparer<string>>(() => new FilePathNormalizingComparer());
public static IEqualityComparer<string> Comparer => _lazyComparer.Value;

private class FilePathNormalizingComparer : IEqualityComparer<string>
{
public bool Equals(string? x, string? y) => FilePathNormalizer.FilePathsEquivalent(x, y);

public int GetHashCode([DisallowNull] string obj) => FilePathNormalizer.GetHashCode(obj);
}

public static string NormalizeDirectory(string? directoryFilePath)
{
if (directoryFilePath.IsNullOrEmpty())
Expand Down Expand Up @@ -131,7 +121,7 @@ public static bool FilePathsEquivalent(string? filePath1, string? filePath2)
return normalizedSpan1.Equals(normalizedSpan2, FilePathComparison.Instance);
}

public static int GetHashCode(string filePath)
public static int GetHashCode(string filePath, Func<char, char> charConverter)
{
if (filePath.Length == 0)
{
Expand All @@ -143,14 +133,18 @@ public static int GetHashCode(string filePath)
using var _ = ArrayPool<char>.Shared.GetPooledArray(filePathSpan.Length, out var array1);
var normalizedSpan = NormalizeCoreAndGetSpan(filePathSpan, array1);

#if NET
return string.GetHashCode(normalizedSpan, FilePathComparison.Instance);
#else
var hashCombiner = HashCodeCombiner.Start();

foreach (var ch in normalizedSpan)
{
hashCombiner.Add(ch);
hashCombiner.Add(charConverter(ch));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of suggestions:

  1. On .NET Core, consider using the StringComparer.OrdinalIgnoreCase.GetHashCode(ReadOnlySpan<char>) or string.GetHashCode(ReadOnlySpan<char>, StringComparison.OrdinalIgnoreCase) overloads.
  2. If converting the span to lower invariant is the way to go, consider calling ReadOnlySpan<char>.ToLowerInvariant(...) rather than calling into a delegate for every char.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. I looked for a method on a comparer that would be useful, but didn't find one. string.GetHashCode will be very helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like on .NET Framework, ReadOnlySpan<char>.ToLower..() just does a ToString() then calls ToLower() on it. Since we have to loop anyway, I'm leaning towards leaving this for that TFM. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird because I think CultureInvariant is different from an ordinal comparison. It still does some modifications. I think it still works for our needs though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's definitely different, but for hash code I don't think we really care much, because there will still be an Equals check once the right bucket is found, and check does use ordinal comparison. In the case of hash code in a dictionary, I think its actually better to err on the side of more collisions, because we can trust Equals.

}

return hashCombiner.CombinedHash;
#endif
}

private static ReadOnlySpan<char> NormalizeCoreAndGetSpan(ReadOnlySpan<char> source, Span<char> destination)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Runtime.InteropServices;

namespace Microsoft.AspNetCore.Razor.Utilities;

internal sealed class FilePathNormalizingComparer : IEqualityComparer<string>
{
public static readonly FilePathNormalizingComparer Instance = new FilePathNormalizingComparer();

private static readonly Func<char, char> _charConverter = RuntimeInformation.IsOSPlatform(OSPlatform.Linux)
? c => c
: char.ToLowerInvariant;

private FilePathNormalizingComparer()
{
}

public bool Equals(string? x, string? y) => FilePathNormalizer.FilePathsEquivalent(x, y);

public int GetHashCode(string obj) => FilePathNormalizer.GetHashCode(obj, _charConverter);
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public ProjectSnapshot(ProjectState state)
State = state ?? throw new ArgumentNullException(nameof(state));

_lock = new object();
_documents = new Dictionary<string, DocumentSnapshot>(FilePathNormalizer.Comparer);
_documents = new Dictionary<string, DocumentSnapshot>(FilePathNormalizingComparer.Instance);
}

public ProjectKey Key => State.HostProject.Key;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ internal class ProjectState
ProjectDifference.DocumentAdded |
ProjectDifference.DocumentRemoved;

private static readonly ImmutableDictionary<string, DocumentState> s_emptyDocuments = ImmutableDictionary.Create<string, DocumentState>(FilePathNormalizer.Comparer);
private static readonly ImmutableDictionary<string, ImmutableArray<string>> s_emptyImportsToRelatedDocuments = ImmutableDictionary.Create<string, ImmutableArray<string>>(FilePathNormalizer.Comparer);
private static readonly ImmutableDictionary<string, DocumentState> s_emptyDocuments = ImmutableDictionary.Create<string, DocumentState>(FilePathNormalizingComparer.Instance);
private static readonly ImmutableDictionary<string, ImmutableArray<string>> s_emptyImportsToRelatedDocuments = ImmutableDictionary.Create<string, ImmutableArray<string>>(FilePathNormalizingComparer.Instance);
private readonly object _lock;

private readonly IProjectEngineFactoryProvider _projectEngineFactoryProvider;
Expand Down Expand Up @@ -360,7 +360,7 @@ public ProjectState WithHostProject(HostProject hostProject)
return this;
}

var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithConfigurationChange(), FilePathNormalizer.Comparer);
var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithConfigurationChange(), FilePathNormalizingComparer.Instance);

// If the host project has changed then we need to recompute the imports map
var importsToRelatedDocuments = s_emptyImportsToRelatedDocuments;
Expand Down Expand Up @@ -388,7 +388,7 @@ public ProjectState WithProjectWorkspaceState(ProjectWorkspaceState projectWorks
}

var difference = ProjectDifference.ProjectWorkspaceStateChanged;
var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithProjectWorkspaceStateChange(), FilePathNormalizer.Comparer);
var documents = Documents.ToImmutableDictionary(kvp => kvp.Key, kvp => kvp.Value.WithProjectWorkspaceStateChange(), FilePathNormalizingComparer.Instance);
var state = new ProjectState(this, difference, HostProject, projectWorkspaceState, documents, ImportsToRelatedDocuments);
return state;
}
Expand Down Expand Up @@ -448,7 +448,7 @@ internal static List<string> GetImportDocumentTargetPaths(string targetPath, str
{
var itemTargetPath = importItem.FilePath.Replace('/', '\\').TrimStart('\\');

if (FilePathNormalizer.Comparer.Equals(itemTargetPath, targetPath))
if (FilePathNormalizingComparer.Instance.Equals(itemTargetPath, targetPath))
{
// We've normalized the original importItem.FilePath into the HostDocument.TargetPath. For instance, if the HostDocument.TargetPath
// was '/_Imports.razor' it'd be normalized down into '_Imports.razor'. The purpose of this method is to get the associated document
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1228,12 +1228,15 @@ await RunOnDispatcherAsync(() =>
});

// Assert
listener.AssertNotifications(

// AddProject iterates through a dictionary to migrate documents, so the order of the documents is not deterministic. In the real world
// the adds and removes are interleaved per document
listener.OrderBy(e => e.Kind).ThenBy(e => e.DocumentFilePath).AssertNotifications(
x => x.ProjectAdded(ProjectFilePath, newProjectKey),
x => x.DocumentRemoved(DocumentFilePath2, miscProject.Key),
x => x.DocumentAdded(DocumentFilePath1, newProjectKey),
x => x.DocumentAdded(DocumentFilePath2, newProjectKey),
x => x.DocumentRemoved(DocumentFilePath1, miscProject.Key),
x => x.DocumentAdded(DocumentFilePath1, newProjectKey));
x => x.DocumentRemoved(DocumentFilePath2, miscProject.Key));
}

private static TextLoader CreateEmptyTextLoader()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Generic;
using Microsoft.AspNetCore.Razor.Test.Common;
using Microsoft.AspNetCore.Razor.Utilities;
using Xunit;
Expand Down Expand Up @@ -240,4 +241,22 @@ public void Normalize_ReplacesBackSlashesWithForwardSlashes()
// Assert
Assert.Equal("C:/path/to/document.cshtml", normalized);
}

[OSSkipConditionTheory(["OSX", "Linux"])]
[InlineData(@"C:\path\to\document.cshtml")]
[InlineData(@"c:\path\to\document.cshtml")]
[InlineData("C:/path/to/document.cshtml")]
[InlineData("c:/path/to/document.cshtml")]
public void Comparer_CaseInsensitiveDictionary(string fileName)
{
var dictionary = new Dictionary<string, bool>(FilePathNormalizingComparer.Instance)
{
{ "C:/path/to/document.cshtml", true },
{ "C:/path/to/document1.cshtml", true },
{ "C:/path/to/document2.cshtml", true }
};

Assert.True(dictionary.ContainsKey(fileName));
Assert.True(dictionary.TryGetValue(fileName, out _));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Xunit;
using static Microsoft.AspNetCore.Razor.Test.Common.ProjectSystem.TestProjectSnapshotManager.Listener;

namespace Microsoft.AspNetCore.Razor.Test.Common.ProjectSystem;

internal static class NotificationAssertions
{
public static void AssertNoNotifications(this IEnumerable<ProjectChangeEventArgs> notifications)
{
Assert.Empty(notifications);
}

public static void AssertNotifications(this IEnumerable<ProjectChangeEventArgs> notifications, params Action<Inspector>[] inspectors)
{
Assert.Equal(notifications.Count(), inspectors.Length);

var i = 0;
foreach (var notification in notifications)
{
var inspector = inspectors[i];
inspector(new(notification));

i++;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Collections;
using System.Collections.Generic;
using Microsoft.CodeAnalysis.Razor.ProjectSystem;
using Xunit;

namespace Microsoft.AspNetCore.Razor.Test.Common.ProjectSystem;

Expand Down Expand Up @@ -46,21 +45,5 @@ private void ProjectManager_Changed(object? sender, ProjectChangeEventArgs e)
{
_notifications.Add(e);
}

public void AssertNoNotifications()
{
Assert.Empty(_notifications);
}

public void AssertNotifications(params Action<Inspector>[] inspectors)
{
Assert.Equal(_notifications.Count, inspectors.Length);

for (var i = 0; i < _notifications.Count; i++)
{
var inspector = inspectors[i];
inspector(new(_notifications[i]));
}
}
}
}