A builtin_islink() implementation would open the path using the flagsFILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS and get theFileAttributeTagInfo. It's a symlink if the query succeeds andReparseTag isIO_REPARSE_TAG_SYMLINK. Note thatIO_REPARSE_TAG_MOUNT_POINT (i.e. a junction) is intentionally excluded fromislink().

A few cases that are handled bywin32_xstat_impl() should be supported. Perhaps a common function could be implemented that factors out the initial open fromwin32_xstat_impl() andattributes_from_dir() -- e.g.win32_stat_open(const wchar_t *path, BOOL traverse, WIN32_FIND_DATAW *find_data, BOOL *find_data_updated). Or perhaps simply fall back onwin32_xstat_impl() ifCreateFileW() fails with one of the error codes discussed below.

• ERROR_ACCESS_DENIED andERROR_SHARING_VIOLATION: Fall back on usingFindFirstFileW(). Internally, this opens the parent directory viaNtOpenFile() and queries a directory entry for the name usingNtQueryDirectoryFileEx(). The query must fail or return false if the directory entry is a reparse point, unless it's a name-surrogate reparse point (e.g. a symlink or junction) that was supposed to be opened. Here are some cases where this can occur:
  • The file security may grant no access to the caller. Microsoft's filesystems implement a policy that always grantsFILE_READ_ATTRIBUTES access if the caller is grantedFILE_READ_DATA access to the parent directory. However,CreateFileW() also requestsSYNCHRONIZE access.
  • No access is granted to a file that's mapped as a system paging file.
  • No access is granted to a file or directory that has been deleted but is not unlinked, which is the case if there are existing opens and POSIX delete isn't supported by
    • the OS version (e.g. Windows 8),
    • the API function (e.g.RemoveDirectoryW),
    • or the filesystem driver (e.g. exFAT and FAT32).
• ERROR_CANT_ACCESS_FILE: Traversing a reparse point in a path may not be supported because it was created by software on another system (e.g. on a portable drive), or the driver/service that supports the reparse point is no longer installed or running, or it was simply never intended for automatic traversal (e.g.IO_REPARSE_TAG_APPEXECLINK). In this case, the open should be retried with the flagFILE_FLAG_OPEN_REPARSE_POINT. If it turns out to be a name-surrogate reparse point, the query should be failed with the original error code,ERROR_CANT_ACCESS_FILE, as a broken symlink(ish) file.
• ERROR_INVALID_PARAMETER: A device open may requireGENERIC_READ access. For example, opening "\\.\CON" (i.e. "\Device\ConDrv\Console") requires eitherGENERIC_READ orGENERIC_WRITE access. The open has to be retried with the requiredGENERIC_READ access.

A few cases that are handled by win32_xstat_impl() should be supported.

I guess this means we don't have test coverage for them...?

In any event, I agree we should probably try to cover these cases. I'll see what can be done.

Or perhaps simply fall back on win32_xstat_impl() if CreateFileW() fails with one of the error codes discussed below.

My initial implementation from this morning did that, which I like because it doesn't repeat what's clearly battle-tested code, but it made thingsslower in the event of symlinks. I was almost ok with that given that that isn't the common case, but your solution seemed to make everything a little faster (ignoring these new corner cases).

Or perhaps simply fall back on win32_xstat_impl() if CreateFileW() fails with one of the error codes discussed below.

My initial implementation from this morning did that, which I like because it doesn't repeat what's clearly battle-tested code, but it made thingsslower in the event of symlinks.

There shouldn't be an issue with speed for common cases as long as you only fall back onwin32_xstat_impl() for the discussed error codes:

• ERROR_ACCESS_DENIED
• ERROR_SHARING_VIOLATION
• ERROR_CANT_ACCESS_FILE
• ERROR_INVALID_PARAMETER

@eryksun: Thanks for all of your help and pointers.

I've implemented support for these corner cases by callingwin32_stat under the hood, which is not the most efficient but avoids duplicating the complex logic inwin32_xstat_impl.

I'm a little stuck on writing unit tests for them. I was able to coverERROR_ACCESS_DENIED by extending an existing test forstat to also ensure thatisfile matches it.

I'm not sure how to recreate the case you describe inERROR_CANT_ACCESS_FILE.

ForERROR_INVALID_PARAMETER, I triedisfile(r"\\.\CON") andisdir(r"\\.\CON"), but I get the same result (False) with or without this error handling as well as onmain. So maybe I need to somehow create or haveCON?

Looks good to me, but I'll add that there's a lot of code duplication in here; it might be worth it to refactor the code shortly after landing this PR, so the added code is more maintainable and readable.

Looks good to me, but I'll add that there's a lot of code duplication in here; it might be worth it to refactor the code shortly after landing this PR, so the added code is more maintainable and readable.

I'll take a first pass at reducing some of this duplication.

My read of the duplication is that there's just enough difference that it's probably not worth it. The main change I'd like to see (but wasn't requiring) is to add an option topath_t for Argument Clinic to let it fall through in the case of errors, rather than raising.

If all the additional functions were using the same OS call, it would make sense to merge them all. But they're subtly different each time, and since this is for perf, we don't want to introduce more switches.

The main change I'd like to see (but wasn't requiring) is to add an option topath_t for Argument Clinic to let it fall through in the case of errors, rather than raising.

+1 (I suggest adding that in a follow-up PR; let's land this.)

If all the additional functions were using the same OS call, it would make sense to merge them all. But they're subtly different each time, and since this is for perf, we don't want to introduce more switches.

True that.

Michael, are you still working on refactoring the code? Steve and Erlend seem to be in agreement that it's ready to merge as is. I think it's ready.

Just heard that it's good to go as it is.

Thanks@mdboom for the PR, and@zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

Thanks@mdboom for the PR, and@zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Sorry,@mdboom and@zooba, I could not cleanly backport this to3.10 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.10

Sorry@mdboom and@zooba, I had trouble checking out the3.11 backport branch.
Please retry by removing and re-adding the "needs backport to 3.11" label.
Alternatively, you can backport usingcherry_picker on the command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.11

This ought to be safe to backport from a public API perspective, but I'll let the bot figure out how easy it'll be.

Thanks@mdboom for the PR, and@zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Sorry,@mdboom and@zooba, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 86ebd5c3fa9ac0fba3b651f1d4abfca79614af5f 3.11

Okay, it's not going to backport trivially :) If anyone's up for it, go ahead, otherwise I'll take a look when I get time.

(It's relatively rare that we backport performance improvements, so if it ends up being complicated maybe we shouldn't)