fix panic in Path::strip_prefix

[name1e5s]

close #93586

[rust-highfive]

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

This looks like a correct fix, I think, but I think it would be worthwhile to remove prefix_len as a distinct thing from prefix_remaining entirely -- it seems like it's essentially not a good idea to use that function (unless you know the state is Prefix, but then they have identical behavior).

Would you be up for making that refactoring? It looks like there are not that many users that would need to change and all have a trivially known state, if I look locally.

[name1e5s]

This looks like a correct fix, I think, but I think it would be worthwhile to remove prefix_len as a distinct thing from prefix_remaining entirely -- it seems like it's essentially not a good idea to use that function (unless you know the state is Prefix, but then they have identical behavior).

Would you be up for making that refactoring? It looks like there are not that many users that would need to change and all have a trivially known state, if I look locally.

done

[Mark-Simulacrum]

Thanks, that looks great!

@bors r+ rollup=never (potentially perf-sensitive)

[bors]

📌 Commit 8ef9477eac8ccad15078b21d2004813c3f084f4c has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit 8ef9477eac8ccad15078b21d2004813c3f084f4c with merge eb40a89684843ed08dc5bf766825533517238e56...

[bors]

💔 Test failed - checks-actions

[matthiaskrgr]

@bors retry x86_64 msvc (probably) spurious failure:

[RUSTC-TIMING] unicode_width test:false 0.100
[RUSTC-TIMING] getopts test:false 3.213
[RUSTC-TIMING] proc_macro test:false 15.787
   Compiling test v0.0.0 (D:\a\rust\rust\library\test)
error: linking with `link.exe` failed: exit code: 1120
  |
  = note: "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Enterprise\\VC\\Tools\\MSVC\\14.29.30133\\bin\\HostX64\\x64\\link.exe" "/DEF:C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\rustcyYH2W9\\lib.def" "/NOLOGO" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\test-6464307edd00076d.test.06275a10-cgu.0.rcgu.o" "D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\test-6464307edd00076d.47fpacyqlqntgftt.rcgu.rmeta" "/LIBPATH:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps" "/LIBPATH:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\release\\deps" "/LIBPATH:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\build\\compiler_builtins-d23c84560eca12a0\\out" "/LIBPATH:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "kernel32.lib" "/WHOLEARCHIVE:\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\libgetopts-04f14876bdd48cb7.rlib" "/WHOLEARCHIVE:\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\libunicode_width-c74fe7d359ba4245.rlib" "/WHOLEARCHIVE:\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\librustc_std_workspace_std-ac92d3dd0da3e4c2.rlib" "/LIBPATH:\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps" "\\\\?\\D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\libcompiler_builtins-033009b95f350faf.rlib" "kernel32.lib" "ws2_32.lib" "bcrypt.lib" "advapi32.lib" "userenv.lib" "kernel32.lib" "libcmt.lib" "/NXCOMPAT" "/LIBPATH:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "/OUT:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\test-6464307edd00076d.dll" "/OPT:REF,ICF" "/DLL" "/IMPLIB:D:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\deps\\test-6464307edd00076d.dll.lib" "/DEBUG"
  = note: lib.def : error LNK2001: unresolved external symbol __rust_alloc

          lib.def : error LNK2001: unresolved external symbol __rust_alloc_zeroed

          lib.def : error LNK2001: unresolved external symbol __rust_dealloc

          lib.def : error LNK2001: unresolved external symbol __rust_realloc

          D:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-std\x86_64-pc-windows-msvc\release\deps\test-6464307edd00076d.dll.lib : fatal error LNK1120: 4 unresolved externals
          

[RUSTC-TIMING] test test:false 17.556
error: could not compile `test` due to previous error

[bors]

⌛ Testing commit 8ef9477eac8ccad15078b21d2004813c3f084f4c with merge 25b8514be67192e2651807fc34bcbd353c3e31d3...

[bors]

💔 Test failed - checks-actions

[matthiaskrgr]

ok might not be spurious after all 🤔

[Mark-Simulacrum]

I have no sense of why this might be happening -- this particular patch seems like it should be near-identical in terms of functionality. Maybe we're no longer including some prefix due to the change in include_cur_dir, which is causing us to reference a different path in the link invocation somehow? That would be surprising to me, the behavior looks right I think with the new impl.

cc @the8472 since you've touched the path-related code recently and maybe can spot what I'm presumably overlooking here

[the8472]

I see two potential problems

a) the new prefix_remaining() method only checks self.front == State::Prefix but not self.back which changes during backwards iteration and can also reach the prefix
b) len_before_body() wants to know the prefix length even when self.front != Prefix (or is this part of the fix? The PR doesn't explain)

#93586 (comment) mentions as_path panicking which calls trim_right() which in turn calls len_before_body so that may be worth looking into. Maybe try to reproduce it first with a stacktrace to check it's the right callsite.

[the8472]

Btw, did you run stage0 std tests under windows locally? I guess CI didn't even get that far, but if the testsuite doesn't catch whatever is failing here then we need more coverge.

[ChrisDenton]

I'm seeing these failures locally: x.py test library/std --test-args path::tests

---- path::tests::test_decompositions_windows stdout ----
thread 'path::tests::test_decompositions_windows' panicked at 'iter: Expected ["c:", "\\"], found ["\\", ":"]', library\std\src\path\tests.rs:523:5

---- path::tests::test_push stdout ----
thread 'path::tests::test_push' panicked at 'pushing "baz" onto "\\\\?\\foo\\bar": Expected "\\\\?\\foo\\bar\\baz", got "\\?\\foo\\bar\\baz"', library\std\src\path\tests.rs:1252:9

[bors]

💔 Test failed - checks-actions

[name1e5s]

After testing on my new device with windows 11 installed, I found the problem may belongs to the refactoring after the small fix. The fix itself is okay. I'll find out why the refactoring breaks 2 other tests.
Screenshot for testing on my branch:

The failing tests on my branch also failed on master, still investgating the reason.
Screenshot for testing on master:

[ChrisDenton]

test_windows_absolute should be fixed by #94650. I'd guess windows_exe_resolver is failing for you because you don't have developer mode enabled in the Windows settings? In either case they shouldn't block this PR. Btw, you can use the --test-args parameter to only run relevant tests.

[bors]

☔ The latest upstream changes (presumably #94887) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage:
@name1e5s #94650 has been merged - can you please post your status on this PR?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[name1e5s]

@rustbot ready

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit b87dd75 has been approved by Mark-Simulacrum

[bors]

⌛ Testing commit b87dd75 with merge 83322c5...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 83322c5 to master...

[rust-timer]

Finished benchmarking commit (83322c5): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression