http://d.puremagic.com/issues/show_bug.cgi?id=8909

           Summary: is{File,Dir,SymLink} mix return error code and
                    exception
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Phobos
        AssignedTo: nobody@puremagic.com
        ReportedBy: dimitri.sabadie@gmail.com


--- Comment #0 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-29 07:46:15 PDT ---
I think a call to .isFile / .isDir / .isSymLink shouln’t throw any exception. The problem here is that to use, for instance, isFile, we have to use it this way :

try {
    "path/to/file".isFile;
} catch (const Exception e) {
    /* treat the error here */
}

Such a fonction is excepted — according to its name — to be used by checking it’s returned value :

if (!"path/to/file".isFile) {
    /* treat the error here */
}

The problem is that the current interface mix both those methods, so the above codes is just fucking weird for two reasons :

1. if "path/to/file" is not a file, the if statement won’t even be executed
because of the thrown exception, so an if statement is not good here
2. we don’t catch the exception, so we can’t treat the error. An if we do, we
don’t rely on the returned value.

I propose to remove the exception throw. I’d really love to write this patch, I’m motivated to contribute to D and all projects around it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmx.com


--- Comment #1 from Jonathan M Davis <jmdavisProg@gmx.com> 2012-10-29 09:01:03 PDT ---
I believe that the only reason that they'll throw is if they fail to stat the file, and it makes perfect sense IMHO for them to throw in that case. Neither true nor false would be correct if the file doesn't even exist in the first place. As long as you check that the file exists in the first place, you shouldn't need to worry about them throwing exceptions.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #2 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-29 12:02:26 PDT ---
(In reply to comment #1)
> I believe that the only reason that they'll throw is if they fail to stat the file, and it makes perfect sense IMHO for them to throw in that case. Neither true nor false would be correct if the file doesn't even exist in the first place. As long as you check that the file exists in the first place, you shouldn't need to worry about them throwing exceptions.

I think you’re wrong, because when we use a property or function called is*, we
don’t expect it to throw exception. I personally don’t and I thank it’s insane
to do so. Furthermore, the fact that is{File,Dir,SymLink} « stats » a file is
an implementation detail thus we don’t have to know anything stat relevant.
Then your argument about stat failure is not out of topic IMHO. If the property
actually fails to stat a file, we don’t really care about why. If we do, I
think a set of more specialized function should be used.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #3 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-29 12:04:47 PDT ---
(In reply to comment #2)
> Then your argument about stat failure is not out of topic IMHO.

I meant « is out of topic ».

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |WONTFIX


--- Comment #4 from Jonathan M Davis <jmdavisProg@gmx.com> 2012-10-29 13:16:14 PDT ---
I don't think for a second that there's anything special about isX functions which make it so that they can't throw. It all depends on what they're doing. If they can't throw, then they're marked with nothrow.

And while the stat call may be an implementation detail, regardless of how isFile, isDir, or isSymlink are implemented, they rely on the fact that the file that they're given exists, so _of course_ there's going to be a serious problem if the file doesn't exist. Throwing is the correct behavior in that case.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #5 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-29 13:22:10 PDT ---
(In reply to comment #4)
> And while the stat call may be an implementation detail, regardless of how isFile, isDir, or isSymlink are implemented, they rely on the fact that the file that they're given exists, so _of course_ there's going to be a serious problem if the file doesn't exist. Throwing is the correct behavior in that case.

This case should be test before a call to isFile IMHO. Here isFile has a too high responsibility.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #6 from Jonathan M Davis <jmdavisProg@gmx.com> 2012-10-29 13:53:01 PDT ---
Of course, you test whether the file exists before calling isFile, isDir, isSymlink. That avoids getting the exception when the file doesn't exist. But those functions can still be called without having first checked whether the file existed, and it's perfectly possible for the file to be deleted between the time that you check that it exists and you call isFile/isDir/isSymlink. So, those functions must handle the case where they're given a file which doesn't exist or is otherwise inaccesible (permissions could also make them fail and could make exists fail in the same way). As such, those functions have only 3 options when they are unable to determine whether the file is a file/dir/symlink

1. Throw an exception.
2. Return true.
3. Return false.

#2 is patently wrong, because if the file doesn't exist, claiming that it's a file/dir/symlink would be total lie. And #3 doesn't really make sense either, because that would mean that the program would be being told that the file wasn't a file/dir/symlink when it doesn't actually know what it is. After a call to isDir returned false, it could end up assuming that file operations would work on it (since almost everything is either a file or a directory) and then do who-knows-what based on incorrect information. If isFile/isDir/isSymlink can't determine what the file is, then it really doesn't make sense to return either true or false. So, the correct way to handle it is an exception. Not to mention, that allows for much better error-handling. If an exception is thrown, then the program may be able to respond according to what the error code in the exception was (e.g. it may be able to indicate to the user that they don't have permissions to read the file).

I think that it's quite clear that throwing an exception is the correct thing for isFile/Dir/Symlink to do when they are unable to query the file for whether it's a file/dir/symlink.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #7 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-29 14:38:19 PDT ---
(In reply to comment #6)
> Of course, you test whether the file exists before calling isFile, isDir, isSymlink. That avoids getting the exception when the file doesn't exist. But those functions can still be called without having first checked whether the file existed, and it's perfectly possible for the file to be deleted between the time that you check that it exists and you call isFile/isDir/isSymlink. So, those functions must handle the case where they're given a file which doesn't exist or is otherwise inaccesible (permissions could also make them fail and could make exists fail in the same way). As such, those functions have only 3 options when they are unable to determine whether the file is a file/dir/symlink
> 
> 1. Throw an exception.
> 2. Return true.
> 3. Return false.
> 
> #2 is patently wrong, because if the file doesn't exist, claiming that it's a file/dir/symlink would be total lie. And #3 doesn't really make sense either, because that would mean that the program would be being told that the file wasn't a file/dir/symlink when it doesn't actually know what it is. After a call to isDir returned false, it could end up assuming that file operations would work on it (since almost everything is either a file or a directory) and then do who-knows-what based on incorrect information. If isFile/isDir/isSymlink can't determine what the file is, then it really doesn't make sense to return either true or false. So, the correct way to handle it is an exception. Not to mention, that allows for much better error-handling. If an exception is thrown, then the program may be able to respond according to what the error code in the exception was (e.g. it may be able to indicate to the user that they don't have permissions to read the file).
> 
> I think that it's quite clear that throwing an exception is the correct thing for isFile/Dir/Symlink to do when they are unable to query the file for whether it's a file/dir/symlink.

I well listen to what you said, and I still don’t agree. The problem is in your own solution :

> […] That avoids getting the exception when the file doesn't exist. But

What the point of avoiding a function to throw? You said there’s function to check if the file exists (may you tell me which please?), so we can call it before to call isFile without throwing exception -> it’s a huge design problem, because we will never really catch the exception. And it’s quite logic, because isFile, isDir or isSymLink, by the responsibility they have, shouldn’t check if the file exists. In worst situation, the fact the file exists may appear as a precondition of the property, but an exception…

I’m afraid to figure out how, nowadays, people want to overuse features where simple and more adapted ones would be far away enough… You mark this thread as WONTFIX, it’s a pity, because there’s definitely a problem around those properties.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #8 from Jonathan M Davis <jmdavisProg@gmx.com> 2012-10-29 14:58:46 PDT ---
Checking whether the file exists makes it so that you don't have to take the performance hit when the exception is thrown and allows you to treat the exception as the exceptional case rather than expected. But since it makes no sense to return either true or false when the file can't be checked (due to no longer existing or a lack of permissions or whatever), there's really no choice but to throw an exception unless you want to go the C route of returning an error code and doing the actual return through a pointer or ref argument. But if you want that, just use the C functions. Regardless, you can't assume that the function will work, because it can fail even if you check for the file's existence beforehand.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=8909



--- Comment #9 from Dimitri Sabadie <dimitri.sabadie@gmail.com> 2012-10-30 01:24:12 PDT ---
Ok, got you meant. The thing is I’d more likely use a two-steps approach (if exists then if isFile, for instance). Some guys talk about concurrency, I think it’s not a good argument here because isFile is not atomic either if used with exception. Though I understand why it’s marked as WONTFIX.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------