Skip to content

goutil: handle Windows paths in ResolvePackage and ResolveImport - #91

Open
shane-tw wants to merge 2 commits into
masterfrom
fix/goutil-windows-paths
Open

goutil: handle Windows paths in ResolvePackage and ResolveImport#91
shane-tw wants to merge 2 commits into
masterfrom
fix/goutil-windows-paths

Conversation

@shane-tw

@shane-tw shane-tw commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Why

Two goutil path helpers assume Unix conventions. Found while investigating why kommentaar fails to resolve types for a colleague running a native Windows checkout (no WSL) — it calls both helpers for every type reference it looks up.

What's wrong

ResolvePackage switched on path[0] and only treated a leading / as a directory:

switch path[0] {
case '/':   pkg, err = build.ImportDir(path, mode)   // directory
case '.':   ...
default:    pkg, err = build.Import(path, cwd, mode) // import path
}

A Windows absolute path starts with a drive letter, so C:\src\proj\api fell through to default and was resolved as if it were an import path — which can never succeed. The doc comment already promises full paths are supported.

ResolveImport's same-package fallback derived the current package name from a filesystem path with the slash-only path package:

currentPkg := path.Base(path.Dir(file))

For C:\src\pkg\file.go there is no /, so path.Dir returns . and currentPkg becomes .. The pkgName == currentPkg comparison then never matches, so same-package references never resolve to "." on Windows.

Both are demonstrable without a Windows machine, since the mishandling is in the parsing rather than the host: path.Dir returns . for a backslash path on every OS, and the path[0] branch picks the wrong route regardless of where it runs.

Fix

Two small helpers that accept either convention, so they are unit-testable on any host instead of only on a Windows runner:

  • isAbsPath — Unix /, Windows drive (C:\, C:/) and UNC (\\host\share).
  • dirName — directory name of a path, treating / and \ as separators.

No behaviour change for Unix paths: isAbsPath still routes /dir to build.ImportDir, and dirName reduces to the previous expression when the path contains no backslash. A literal backslash in a Unix file name would now be read as a separator, which Go source paths never rely on (noted in the doc comment).

Testing

go test ./goutil/ passes, including new table tests for both helpers covering Unix, drive-letter, UNC and relative inputs. TestDirName's Windows cases fail against the old expression, so they'd have caught this.

GOOS=windows go build ./... and GOOS=windows go vet ./goutil/ are clean. Full go test ./... is green apart from httputilx.TestFetch, which hits httpbin.org and fails identically on master (503 from the external service).

Not verified end-to-end on a Windows host — I don't have one. The unit tests pin the behaviour that was wrong; a sanity check from someone on Windows would be welcome.

Also in this PR

httputilx.TestFetch drove its status-code cases off httpbin.org, which
returned a 503 and failed the build on this branch while the same commit passed
on a re-run. A second commit serves those fixtures from an httptest server, and
points the connection-failure case at a closed local port instead of relying on a
domain never resolving (a wildcard DNS resolver defeats that). Unrelated to the
goutil change, but it was the only thing red on CI — happy to split it out if
you'd rather.

🤖 Generated with Claude Code

Two path helpers assumed Unix conventions, so both misbehave on a Windows
checkout.

ResolvePackage switched on path[0] and only treated a leading '/' as a
directory. A Windows absolute path starts with a drive letter, so it fell
through to build.Import and was resolved as though it were an import path,
which always fails.

ResolveImport's same-package fallback derived the current package name with
path.Base(path.Dir(file)). The path package is slash-only, so for
C:\src\pkg\file.go it evaluates to "." and the fallback never fires,
leaving same-package type references unresolvable.

Both are fixed with helpers that accept either convention, so they are
unit-testable on any host rather than only on Windows. No behaviour change
for Unix paths.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coveralls

coveralls commented Aug 13, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 31686645085

Coverage increased (+0.2%) to 76.868%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: 2 uncovered changes across 1 file (14 of 16 lines covered, 87.5%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
goutil/goutil.go 16 14 87.5%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 1686
Covered Lines: 1296
Line Coverage: 76.87%
Coverage Strength: 39.22 hits per line

💛 - Coveralls

TestFetch drove its status-code cases off httpbin.org, which returns 503
often enough to fail unrelated builds — it failed on this branch while
passing on a re-run of the same commit.

An httptest server now serves the 400/500/418 responses, and the
connection-failure case dials a closed local port rather than relying on a
domain never resolving, which a wildcard DNS resolver can defeat.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants