Ensure we match equivalent paths when retrieving analyzer options in global configs#56588
Conversation
|
@dotnet/roslyn-compiler for review please :) |
src/Compilers/Core/CodeAnalysisTest/Analyzers/AnalyzerConfigTests.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
1059699 to
96d4078
Compare
|
Rebased onto rel/dev17.0 |
96d4078 to
e1e4212
Compare
|
@RikkiGibson @dotnet/roslyn-compiler. I've changed the approach for this PR. At this point we don't treat analyzer config paths as their system representations, so its incorrect to use system functions on them. Instead, we now just extract out the '..' and '.' ourselves, which is simple because we know the path has to be in a certain format. Appreciate a couple of re-reviews on this. |
Correctly handle windows paths Add extra comments
| var parts = GetPathParts(p); | ||
|
|
||
| // On windows we need to skip the volume specifier, but remember it for re-joining later | ||
| var volumeSpecifier = (!IsUnixLikePlatform) ? string.Empty : p.Substring(0, 2); |
| // GetPathParts also removes any instances of '.' | ||
| var parts = GetPathParts(p); | ||
|
|
||
| // On windows we need to skip the volume specifier, but remember it for re-joining later |
There was a problem hiding this comment.
Yes, the test explicitly adds the volume for windows. Makes sense to add a direct test too though.
| var volumeSpecifier = (!IsUnixLikePlatform) ? string.Empty : p.Substring(0, 2); | ||
|
|
||
| // Skip the root directory | ||
| var toSkip = IsUnixLikePlatform ? 1 : 2; |
There was a problem hiding this comment.
It actually would work with UNC paths, but not entirely correctly. Instead i've just made it bail if we see them, as this is supposed to be best effort.
| [InlineData("././../.././dir1/dir2/file.cs", false)] | ||
| [InlineData("./dir1/../file.cs", false)] | ||
| [InlineData("../dir1/dir2.cs", false)] | ||
| public void EquivalentSourcePathNames(string sourcePath, bool shouldMatch) |
There was a problem hiding this comment.
Done. Explicitly excluding UNC paths. Its essentially a best effort, so if we can't expand it we'll just return the original string. Added tests for relative, device specified and UNC and called out in the remarks what we do.
| [InlineData("./dir1/dir2/file.cs", false)] | ||
| [InlineData("././../.././dir1/dir2/file.cs", false)] | ||
| [InlineData("./dir1/../file.cs", false)] | ||
| [InlineData("../dir1/dir2.cs", false)] |
There was a problem hiding this comment.
Instead of 'shouldMatch', should we just include a 'string expected' in this data? For example, the expected result of normalizing ./dir1/../file.cs is file.cs (I hope--otherwise I misread the implementation or didn't look closely enough at GetParts, etc.)
There was a problem hiding this comment.
I ended up doing that on the direct test, so we can get decent coverage.
Add tests for method directly.
| } | ||
|
|
||
| [ConditionalTheory(typeof(WindowsOnly))] | ||
| [InlineData("x:\\a\\b\\file.cs", "x:/a/b/file.cs")] |
| [InlineData("\\\\.\\C:\\a\\..\\b.cs", "\\\\.\\C:\\a\\..\\b.cs")] | ||
| [InlineData("\\\\?\\C:\\a\\b.cs", "\\\\?\\C:\\a\\b.cs")] | ||
| [InlineData("\\\\.\\Volume{00000000-0000-0000-0000-000000000000}\\a\\b.cs", "\\\\.\\Volume{00000000-0000-0000-0000-000000000000}\\a\\b.cs")] | ||
| public void TestExpandAbsolutePathWithRelativeParts_Windows(string input, string expected) => TestExpandAbsolutePathWithRelativeParts(input, expected); |
| [InlineData("../a/../b/c", "../a/../b/c")] | ||
| [InlineData("foo", "foo")] | ||
| [InlineData("", "")] | ||
| public void TestExpandAbsolutePathWithRelativeParts(string input, string expected) |
6efec13 to
ff8f1d5
Compare
Fixes #47786