Skip to content

Conversation

@ShikiSuen
Copy link

@ShikiSuen ShikiSuen commented Dec 2, 2025

This PR targets swiftlang:release/6.3.

  • On Windows, it's not guaranteed that USERPROFILE is equivalent to %SYSTEMDRIVE%\Users\%USERNAME%. It is therefore possible that UserDefaults tries to write to a non-existent location, in which case it will never work. When this happens, all writes silently no-op.

  • This PR uses the Shell API SHGetKnownFolderPath to programmatically get the location of %LOCALAPPDATA% instead of manually building the path.


  • Explanation:

    • On Windows, it is not guaranteed that the environment variable USERPROFILE is equivalent to %SYSTEMDRIVE%\Users\%USERNAME%. Because of this, building the LocalAppData path using environment variables or string concatenation can result in a path that does not exist, and writes may silently no-op.
    • This change replaces hand-built LocalAppData path construction with a proper Windows Shell API call: SHGetKnownFolderPath(&FOLDERID_LocalAppData, ...) to get the correct per-user LocalAppData path.
    • Additional cleanup: include <shlobj_core.h> and use CoTaskMemFree to free the pointer from SHGetKnownFolderPath. The change also tightens a fallthrough control flow for _kCFKnownLocationUserCurrent so we do not accidentally fall through when the username lookup succeeded.
  • Scope:

    • Platform-specific change: only affects Windows (TARGET_OS_WIN32).
    • File(s) changed: Sources/CoreFoundation/CFKnownLocations.c.
    • Behavior change: CFPreferences path lookup for the current user is now retrieved through the Windows shell API rather than manually constructing a path. This should make LocalAppData detection more reliable.
    • No public API surface changes; behavior for other platforms is unaffected.
    • Potential behavior change: logic for "ByName" (i.e., CFKnownLocationUserByName) may no longer compute the LocalAppData for a specified user other than the current user; instead it uses the OS API to get the path for the current account. If the intent is to support accessing preferences for other users on Windows, this must be assessed (see Risk section).
  • Issues:

    • Fixes a scenario where writing preferences silently no-ops because the computed LocalAppData path does not exist on Windows due to assumptions about USERPROFILE being equivalent to %SYSTEMDRIVE%\Users\%USERNAME%.
    • Related behavior: If the code previously relied on manually-built path logic to support non-current user "ByName" paths on Windows, that functionality may require re-evaluation.
  • Original PRs:
    Derived from mainline work: "use shgetknownfolderpath instead of hand building the path" and follow-up Windows fixes.

  • Risk:

    • Risk severity: Low to Moderate (Windows-only).
    • Rationale:
      • Low: No API changes; logic cleanup reduces certain failure modes and uses a documented OS API to find the current user's LocalAppData.
      • Moderate: The previous code path that attempted to build a per-user path based on profiles may have been able to derive a LocalAppData path for a user other than the current user (e.g., by combining GetProfilesDirectoryW + username); the new approach uses SHGetKnownFolderPath which returns the path for the effective user context. If the project intends to support preference paths for other users (ByName) on Windows, then that feature may need an explicit approach (e.g., obtaining a path via SID impersonation or GetProfilesDirectoryW for other users).
    • Build/link risk:
      • Uses SHGetKnownFolderPath and FOLDERID_LocalAppData which requires appropriate headers (shlobj_core.h) and linking to Windows libraries (ole32 or shell32 may be required). Verify that the build system provides these for Windows targets.
    • Runtime risk:
      • SHGetKnownFolderPath may fail on older Windows versions; ensure fallback is appropriate if needed (i.e., graceful fallback to prior methods or to a sensible default).
      • Memory management must handle the pointer returned by SHGetKnownFolderPath correctly (using CoTaskMemFree) — the commit does this.
  • Testing:

    • TO BE FILLED-IN.
  • Reviewers:

    • TO BE FILLED-IN.

@ShikiSuen ShikiSuen changed the title Windows: Use SHGetKnownFolderPath to compute LocalAppData. Windows: Use SHGetKnownFolderPath to compute LocalAppData. (targeting release/6.3). Dec 2, 2025
@compnerd
Copy link
Member

compnerd commented Dec 2, 2025

@swift-ci please test Linux platform

@compnerd
Copy link
Member

compnerd commented Dec 2, 2025

@swift-ci please test Windows platform

@compnerd
Copy link
Member

compnerd commented Dec 2, 2025

Could you please include the filled out CCC template from https://github.com/swiftlang/.github/blob/main/PULL_REQUEST_TEMPLATE/release.md?plain=1

@ShikiSuen
Copy link
Author

@compnerd I added the filled template to the main post. There are still fields pending finish depending on the CI results.

@compnerd compnerd changed the title Windows: Use SHGetKnownFolderPath to compute LocalAppData. (targeting release/6.3). [6.3] Windows: Use SHGetKnownFolderPath to compute LocalAppData Dec 2, 2025
@compnerd
Copy link
Member

compnerd commented Dec 2, 2025

@compnerd I added the filled template to the main post. There are still fields pending finish depending on the CI results.

Thanks, yes, that seems fine. I will kick off the windows tests once more, but it might be related tot his change.

@compnerd
Copy link
Member

compnerd commented Dec 2, 2025

@swift-ci please test Windows platform

@ShikiSuen
Copy link
Author

@swift-ci Please test Windows platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants