Path casts #2077

nathanhammond · 2022-09-26T07:48:23Z

@gsoltis (correctly! as proven by this PR) has a vendetta against casts.

This PR removes most casts to turbopath.* in the repository with explicit exceptions for strings and t.TempDir().

The regex used to identify casts, of which, 100% have been manually reviewed:
turbopath\.(Absolute|Anchored|Relative)(Unix|System)Path\(

Of the remaining 13 that do not fall into the above "string" or t.TempDir() exceptions, 7 fall outside of the fs package:
turbopath\.(Absolute|Anchored|Relative)(Unix|System)Path\((?!("|t\.TempDir))

7 results - 4 files

cli/internal/cache/cache_http.go:
  280  		// FIXME: THIS IS A BUG.
  281: 		restoredName := turbopath.AnchoredUnixPath(hdr.Name)
  282  		files = append(files, restoredName.ToSystemPath())

cli/internal/hashing/package_deps_hash_test.go:
  20  	cwd, _ := os.Getwd()
  21: 	root := turbopath.AbsoluteSystemPath(filepath.VolumeName(cwd) + string(os.PathSeparator))
  22: 	checking := turbopath.AbsoluteSystemPath(cwd)
  23  

  32  			for _, file := range files {
  33: 				fileName := turbopath.RelativeSystemPath(file.Name())
  34  				if strings.Index(fileName.ToString(), fmt.Sprintf("%02d-", id)) == 0 {
  35: 					return turbopath.AbsoluteSystemPath(fixtureDirectory.Join(fileName))
  36  				}

cli/internal/lockfile/berry_lockfile.go:
  305  		if isPatch && !strings.HasPrefix(patchPath, "~") && !_builtinRegexp.MatchString(patchPath) {
  306: 			patches = append(patches, turbopath.AnchoredUnixPath(patchPath))
  307  		}

cli/internal/lockfile/pnpm_lockfile.go:
  251  	for _, patch := range p.PatchedDependencies {
  252: 		patches[i] = turbopath.AnchoredUnixPath(patch.Path)
  253  		i++

Of these 7:

One is a bug resolved by cache-to-tar.
Four are in a single test utility function. That should get cleaned up but is very low priority.
Two are in application code for berry and pnpm patches. I'll pass that off to @chris-olszewski since I'm not 100% certain that I can safely slap turbopath.AnchoredUnixPathFromUpstream() on them.

vercel · 2022-09-26T07:48:28Z

@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

nathanhammond

Notes to reviewers.

nathanhammond · 2022-09-26T07:52:21Z

cli/internal/hashing/package_deps_hash_test.go

-				turbopath.AnchoredSystemPath(filepath.Join("..", "root.json")),
-				turbopath.AnchoredSystemPath("child.json"),
-				turbopath.AnchoredSystemPath(filepath.Join("grandchild", "grandchild.json")),
+				turbopath.AnchoredUnixPath("../root.json").ToSystemPath(),


This actually breaks one of the rules intended to exist for Anchored(System|Unix)Paths: that they're io/fs safe. I'm not going to address this now because it's an issue with our entire hashing story.

cli/internal/lockfile/berry_lockfile_test.go

nathanhammond · 2022-09-26T07:56:15Z

cli/internal/scope/filter/filter_test.go

 	packageJSONs := make(map[interface{}]*fs.PackageJSON)
 	graph := &dag.AcyclicGraph{}
 	graph.Add("@foo/bar")
 	packageJSONs["@foo/bar"] = &fs.PackageJSON{
 		Name: "@foo/bar",
-		Dir:  turbopath.AnchoredSystemPath(filepath.Join(root, "packages", "bar")),
+		Dir:  rootPath.UntypedJoin("packages", "bar"),


This is actually a problem; we're alternately supplying this with Anchored and Absolute paths. We still need to address this (which is why this PR presently is in Draft).

Okay, I've confirmed that this is definitely supposed to be a Anchored 100% of the time: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L906

I'm changing all instances of this to match.

nathanhammond · 2022-09-26T07:58:07Z

cli/internal/scope/scope_test.go

@@ -57,28 +57,28 @@ func TestResolvePackages(t *testing.T) {
 	graph.Connect(dag.BasicEdge("app2-a", "libC"))
 	packagesInfos := map[interface{}]*fs.PackageJSON{
 		"app0": {
-			Dir: turbopath.AnchoredSystemPath(filepath.FromSlash("app/app0")),
+			Dir: turbopath.AnchoredUnixPath("app/app0").ToSystemPath(),


This should be our preferred pattern for creating AnchoredSystemPaths in tests: use AnchoredUnixPath and then convert it to an AnchoredSystemPath.

Do we want a SystemPathFromUnixString to avoid forgetful devs (me) from trying to directly create a system path from a string with unix separators?

I want to write a linter that checks for \ in strings passed to Unix, and either of \ or / in strings passed to System. Creating a function will not prevent us from making mistakes as we have seen multiple clever ways of accidentally failing this in just my last two PRs.

Problem: low priority.

TURBO-336

(On numerous occasions we have failed to satisfy the XTypeFromYSource constraint, so we should stop relying on fallible humans to do the right thing.)

nathanhammond · 2022-09-26T08:00:34Z

cli/internal/taskhash/taskhash_test.go

-	prefix := pkgName + "/"
-	prefixLen := len(prefix)


This test was accidentally platform dependent, but the upstream code appears to have gracefully handled the garbage in.

nathanhammond · 2022-09-26T08:02:25Z

cli/internal/taskhash/taskhash_test.go

 	if err != nil {
 		t.Fatalf("failed to calculate manual hashes: %v", err)
 	}
 	for path, spec := range files {
-		if strings.HasPrefix(path.ToString(), prefix.ToString()) {
+		if path.ToSystemPath().HasPrefix(pkgName) {


This was previously comparing an AnchoredUnixPath and an AnchoredSystemPath. 🤦‍♂️

So now we have a utility function to stop doing that.

vercel · 2022-09-26T08:18:48Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
turbo-site	✅ Ready (Inspect)	Visit Preview	Sep 27, 2022 at 7:12AM (UTC)

chris-olszewski · 2022-09-26T17:10:36Z

Both of the exceptions in the lockfile files will be fixed up when I properly add patch support to Yarn 2+ and revamp the pnpm patch support. I'm pretty sure AnchoredUnixPathFromUpstream will be the correct answer, but I'll go through the each package manager and verify.

chris-olszewski · 2022-09-26T18:03:18Z

cli/internal/turbopath/anchored_system_path.go

+
+	// otherPath is definitely shorter than p.
+	// We need to confirm that p[len(otherPath)] is a system separator.
+	return os.IsPathSeparator(p[prefixLen])


Won't this return true for "foo/bar".HasPrefix("baz/") or for any prefix that has a length that happens to line up with a path separator?

Yup. Totally missed a step here.

chris-olszewski

Only thing I think needs to get fixed is the implementation of HasPrefix

gsoltis · 2022-09-26T21:16:37Z

cli/internal/turbopath/anchored_unix_path.go

@@ -25,9 +25,9 @@ func (p AnchoredUnixPath) ToUnixPath() AnchoredUnixPath {
 }

 // RelativeTo calculates the relative path between two `AnchoredUnixPath`s.
-func (p AnchoredUnixPath) RelativeTo(basePath AnchoredUnixPath) (AnchoredUnixPath, error) {
+func (p AnchoredUnixPath) RelativeTo(basePath AnchoredUnixPath) (AnchoredSystemPath, error) {


It looks like the callsites immediately convert to AnchoredUnixPath. Maybe this function should do that, rather than change types?

The correct answer here is actually to avoid triggering FromSlash or ToSlash as those calls are cross-platform unsafe. It's usually not a problem because most people on POSIX systems don't create file names like this\is\going\to\go\badly.txt but it's an unfortunate edge case.

I want to actually fix this, but it's not something I can justify doing in our current world.

In the mean time I'm going to drop this function and address the call sites with string manipulation.

nathanhammond mentioned this pull request Sep 26, 2022

Add path types for cache. #2059

Merged

nathanhammond commented Sep 26, 2022

View reviewed changes

nathanhammond requested review from gsoltis and chris-olszewski September 26, 2022 08:18

vercel bot deployed to Preview September 26, 2022 08:19 View deployment

nathanhammond marked this pull request as ready for review September 26, 2022 08:22

nathanhammond requested a review from a team as a code owner September 26, 2022 08:22

vercel bot deployed to Preview September 26, 2022 10:26 View deployment

nathanhammond force-pushed the path-casts branch from 7a88dd4 to 06cfeb9 Compare September 26, 2022 17:00

vercel bot deployed to Preview September 26, 2022 17:01 View deployment

chris-olszewski reviewed Sep 26, 2022

View reviewed changes

chris-olszewski added the pr: fixship A PR which is approved with notes and does not require re-review. label Sep 26, 2022

chris-olszewski approved these changes Sep 26, 2022

View reviewed changes

gsoltis reviewed Sep 26, 2022

View reviewed changes

nathanhammond added 6 commits September 27, 2022 13:17

Correct path casts.

3e42d49

Repair issues in taskhash_test.go

e2f7d0a

Fix creation in tests.

8440191

Since filepath.Rel calls Clean, it returns a system path.

e8178cb

Make Windows happy?

fb1bbcd

Correct HasPrefix and remove unsound method.wq

07927ed

nathanhammond force-pushed the path-casts branch from 2291653 to 07927ed Compare September 27, 2022 05:56

vercel bot deployed to Preview September 27, 2022 07:12 View deployment

nathanhammond added pr: automerge Kodiak will merge these automatically after checks pass and removed pr: fixship A PR which is approved with notes and does not require re-review. labels Sep 27, 2022

kodiakhq bot merged commit f3f6aee into vercel:main Sep 27, 2022

Path casts #2077

Path casts #2077

Uh oh!

Conversation

nathanhammond commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 26, 2022

Uh oh!

nathanhammond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vercel bot commented Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chris-olszewski commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chris-olszewski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nathanhammond commented Sep 26, 2022 •

edited

Loading

vercel bot commented Sep 26, 2022 •

edited

Loading