Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

joinpath should not throw. period. #20912

Closed
StefanKarpinski opened this issue Mar 6, 2017 · 11 comments
Closed

joinpath should not throw. period. #20912

StefanKarpinski opened this issue Mar 6, 2017 · 11 comments
Labels
domain:error handling Handling of exceptions by Julia or the user domain:filesystem Underlying file system and functions that use it good first issue Indicates a good issue for first-time contributors to Julia kind:breaking This change will break code status:help wanted Indicates that a maintainer wants help on an issue or pull request
Milestone

Comments

@StefanKarpinski
Copy link
Sponsor Member

As observed in #20695, joinpath causes an error on Windows in situations where a git URL looks like a relative path with a drive specification – since the drives of the two arguments don't match, an error is thrown. That specific problem can be worked around, but the deeper problem is that joinpath throws in the first place. It's fundamentally just a string manipulation function and shouldn't throw errors. However, that leaves the question of what should happen when the second argument to joinpath is a relative with a drive that doesn't match that of the first argument.

@StefanKarpinski
Copy link
Sponsor Member Author

More generally, no path manipulation functions that treat paths as strings detached from the file system should throw errors.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

it's a function for manipulating paths, that happens to operate on strings. if the inputs aren't valid paths, then shouldn't be calling joinpath.

@StefanKarpinski
Copy link
Sponsor Member Author

StefanKarpinski commented Mar 6, 2017

Yes, leaving surprising booby traps for programmers in seemingly innocuous functions – especially ones that only explode on some platforms – is exactly how we like to design our APIs.

@StefanKarpinski
Copy link
Sponsor Member Author

There are two very distinct classes of file/path manipulation functions:

  1. Those that never look at the file system and only operate on their arguments as data.
  2. Those that look at the file system and require valid paths to file systems objects.

The former should not throw errors – they are fundamentally just data manipulation functions. We already have the example why – an unexpected bug in a function that assumed that calling joinpath on two strings to see if they made a valid path together was a safe operation. It was not.

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2017

It can't give a correct answer in this corner case. The only correct answer is that your inputs are erroneous.

@nalimilan
Copy link
Member

It can return an incorrect path which will trigger an error when trying to open the file. That way you only need to catch exceptions in one place rather than two. Anyway a path is generally composed just before being used to access a file.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 6, 2017

It can probably just return the second object. Answering the question: "relative to a relative path B on drive A, what is the full path to the relative path C on drive D", is probably most sensibly "the relative path D on drive C" (with the first clause adding no useful information). So joinpath("A:B", "C:D") would reasonably be "C:D". The downside is that the commutativity is wrong, if you treat joinpath as equivalent to a sequence of cd operations, but I think we can live with that. Also, fwiw, that's python's interpretation:

> "C:\Program Files\Python36\python.exe"
Python 3.6.0 (v3.6.0:41df79263a11, Dec 23 2016, 08:06:12) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.join("A:B", "C:D")
'C:D'

also, ref #5123

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 6, 2017

if you treat joinpath as equivalent to a sequence of cd operations

Also, fwiw, this interpretation is valid generally on *nix systems (calling normpath is not information-preserving, calling cd is). But this interpretation is not valid generally on windows systems (calling normpath is information-preserving, calling cd is not).

@StefanKarpinski
Copy link
Sponsor Member Author

We should follow Python here since we've followed them on basically everything else path-related. As @nalimilan points out, if it's not a sane answer then opening the file or checking for it will give the appropriate result (error/does not exist).

@ararslan ararslan added domain:error handling Handling of exceptions by Julia or the user domain:filesystem Underlying file system and functions that use it labels Mar 6, 2017
@JeffBezanson JeffBezanson added the kind:breaking This change will break code label Mar 9, 2017
@JeffBezanson JeffBezanson added this to the 1.0 milestone Mar 9, 2017
@StefanKarpinski StefanKarpinski added good first issue Indicates a good issue for first-time contributors to Julia status:help wanted Indicates that a maintainer wants help on an issue or pull request labels Jul 13, 2017
@StefanKarpinski
Copy link
Sponsor Member Author

Resolved: in the case where joinpath currently throws an error, we should return the second path.

@StefanKarpinski
Copy link
Sponsor Member Author

Should anyone with a Windows machine want to do this, the functional change is this:

diff --git a/base/path.jl b/base/path.jl
index c042bef833..bcb04129ee 100644
--- a/base/path.jl
+++ b/base/path.jl
@@ -208,7 +208,7 @@ function joinpath(a::String, b::String)
     isabspath(b) && return b
     A, a = splitdrive(a)
     B, b = splitdrive(b)
-    !isempty(B) && A != B && throw(ArgumentError("drive mismatch: $A$a $B$b"))
+    !isempty(B) && A != B && return B
     C = isempty(B) ? A : B
     isempty(a)                             ? string(C,b) :
     ismatch(path_separator_re, a[end:end]) ? string(C,a,b) :

In addition to that, however, it should have tests for different cases in joinpath, including the one that is changed here, and a NEWS.md entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:error handling Handling of exceptions by Julia or the user domain:filesystem Underlying file system and functions that use it good first issue Indicates a good issue for first-time contributors to Julia kind:breaking This change will break code status:help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

6 participants