Skip to content

refactor: migrate FilePath/NormalizedFilePath to OsPath/NormalizedOsPath #3046 - #5118

Open
blackheaven wants to merge 8 commits into
haskell:masterfrom
blackheaven:refactor/ospath-migration
Open

refactor: migrate FilePath/NormalizedFilePath to OsPath/NormalizedOsPath #3046#5118
blackheaven wants to merge 8 commits into
haskell:masterfrom
blackheaven:refactor/ospath-migration

Conversation

@blackheaven

@blackheaven blackheaven commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

My MuniHac contribution to close #3046

Note that there is still a legacy callsite I'll handle when haskell/lsp#646 is merged.

@blackheaven

blackheaven commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Not sure why everyone is involved, sorry

@blackheaven blackheaven changed the title WIP: migrate FilePath/NormalizedFilePath to OsPath/NormalizedOsPath #3046 refactor: migrate FilePath/NormalizedFilePath to OsPath/NormalizedOsPath #3046 Oct 9, 2026
@blackheaven
blackheaven force-pushed the refactor/ospath-migration branch 2 times, most recently from ef0dd8d to 9e7da21 Compare October 10, 2026 05:32
@blackheaven
WIP: migrate FilePath/NormalizedFilePath to OsPath/NormalizedOsPath h…
17e896a 
…askell#3046

- New type NormalizedOsPath (OsPath-backed, cached NormalizedUri) in
  hls-plugin-api/src/Ide/Types/Location.hs; ghcide's
  Development.IDE.Types.Location re-exports it.
- Same function names, new types: toNormalizedFilePath',
  fromNormalizedFilePath, uriToFilePath' (-> Maybe OsPath),
  uriToNormalizedOsPath (total on client input), filePathToUri' (O(1)),
  fromUri, encodeOsPath/decodeOsPath (filesystem encoding; CPP for the
  filepath 1.4.3xx double-encoding API on GHC <= 9.8).
- lsp NormalizedFilePath re-exports removed; lsp wire stays Text.
- GHC API / hiedb / hie-bios / Process / Temp boundaries keep FilePath
  via fromNormalizedFilePath.
- ghcide core, session-loader, plugin-api, test-utils, all plugins,
  tests migrated. cabal bounds: filepath >=1.4.100.

Verified: cabal build all (GHC 9.8.4); ghcide-tests, func-test,
refactor/hlint/cabal/eval/rename plugin suites; bench edit within
noise; peak RSS unchanged; raw-byte path behavior identical to
baseline.
@blackheaven
blackheaven force-pushed the refactor/ospath-migration branch from 9e7da21 to 17e896a Compare October 10, 2026 05:58
@blackheaven
Allow unsafePerformIO in Ide.Types.Location
bd5a542 
systemFsEncoding and decodeOsPath in Ide.Types.Location need
unsafePerformIO to cache the file-system encoding, same rationale
as the existing Ide.Types allowlist entry.
@blackheaven
Improve failure message of import-path-order test; enable server logs…
79b67fd 
… on CI
@blackheaven
Re-trigger CI (transient haskell-lexer install failure on windows-9.12)
5063610
@blackheaven
Remove temporary HLS_TEST_LOG_STDERR from Test ghcide step
4a60d2b
@blackheaven

blackheaven commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

CI failure triage for windows-9.12 import path order determines module file:

  • Run 38036490411: job failed before any test ran — a transient cabal install failure for haskell-lexer-1.2.1 on the runner (Cabal-7125, install step file-copy error), 28s into cabal build.
  • Run 38038428285 (with HLS_TEST_LOG_STDERR=1 server debug logging temporarily enabled in the Test ghcide step): test (9.12, windows-latest) passed, including the import-path-order test. The enriched failure assertion (prints current diagnostics on failure) is kept permanently as a strict improvement; the temporary env var was reverted.
  • No product-code change was needed; the earlier deterministic-looking failure did not reproduce under logging and is attributed to runner-side flakiness.

@blackheaven

blackheaven commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Latest errors:

Run cabal update
  Downloading the latest package list from hackage.haskell.org
  Unexpected response 403 for http://objects-us-east-1.dream.io/hackage-mirror/timestamp.json
  Error: Process completed with exit code 1.

I'll trigger it again when I get feedback or it's approved.

@fendor

fendor commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

Should we perhaps avoid many of the changes and rather change the Internals of NormalizedFilePath to use OsPath rather than introducing a new type? I feel like the changes would be much more isolated if we changed the internals of NormalizedFilePath instead?

blackheaven added a commit to blackheaven/haskell-language-server that referenced this pull request Oct 10, 2026
@blackheaven
refactor: keep NormalizedFilePath name with OsPath internals
c38ff8d 
Address fendor's review on haskell#5118: avoid introducing a new exported type
name; keep NormalizedFilePath and only change its internals to OsPath.
Mechanical rename back from NormalizedOsPath, plus hiding lists for
lsp-types' own NormalizedFilePath/uriToNormalizedFilePath.

Also commit test/testdata/Property golden data required by
hls-plugin-api tests (previously untracked).
@blackheaven
refactor: keep NormalizedFilePath name with OsPath internals
305f620 
Address fendor's review on haskell#5118: avoid introducing a new exported type
name; keep NormalizedFilePath and only change its internals to OsPath.
Mechanical rename back from NormalizedOsPath, plus hiding lists for
lsp-types' own NormalizedFilePath/uriToNormalizedFilePath.

Also commit test/testdata/Property golden data required by
hls-plugin-api tests (previously untracked).
@blackheaven
fix: format
7f305c5
@blackheaven
blackheaven force-pushed the refactor/ospath-migration branch from c38ff8d to 7f305c5 Compare October 10, 2026 14:46
@blackheaven

blackheaven commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Should we perhaps avoid many of the changes and rather change the Internals of NormalizedFilePath to use OsPath rather than introducing a new type? I feel like the changes would be much more isolated if we changed the internals of NormalizedFilePath instead?

Done, though it did not impact much the final diff.

Please, let me know what I should improve next.

This branch has not been deployed

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

Reviewers

@michaelpj michaelpj Awaiting requested review from michaelpj michaelpj is a code owner
@0rphee 0rphee Awaiting requested review from 0rphee
@konn konn Awaiting requested review from konn konn is a code owner
@soulomoon soulomoon Awaiting requested review from soulomoon soulomoon is a code owner
@joyfulmantis joyfulmantis Awaiting requested review from joyfulmantis
@georgefst georgefst Awaiting requested review from georgefst georgefst is a code owner
@jvanbruegge jvanbruegge Awaiting requested review from jvanbruegge jvanbruegge is a code owner
@eddiemundo eddiemundo Awaiting requested review from eddiemundo eddiemundo is a code owner
@July541 July541 Awaiting requested review from July541 July541 is a code owner
@crtschin crtschin Awaiting requested review from crtschin crtschin is a code owner
@kokobd kokobd Awaiting requested review from kokobd kokobd is a code owner
@Aster89 Aster89 Awaiting requested review from Aster89 Aster89 is a code owner
@fendor fendor Awaiting requested review from fendor fendor is a code owner
@VeryMilkyJoe VeryMilkyJoe Awaiting requested review from VeryMilkyJoe VeryMilkyJoe is a code owner
@drsooch drsooch Awaiting requested review from drsooch drsooch is a code owner
@wz1000 wz1000 Awaiting requested review from wz1000 wz1000 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Migrate to OsPath

2 participants

@blackheaven @fendor