Make File.expand_path mostly spec-compliant

There is one test about leading slashes that I think is stupid and don't
care to support.

include/natalie/file_object.hpp

@@ -23,6 +23,7 @@ class FileObject : public IoObject {
 
     static Value absolute_path(Env *env, Value path, Value dir = nullptr);
     static Value expand_path(Env *env, Value path, Value root);
+    static Value expand_path_old(Env *env, Value path, Value root);
     static void unlink(Env *env, Value path);
     static Value unlink(Env *env, Args &&args);
 

lib/natalie/compiler/binding_gen.rb

@@ -594,6 +594,7 @@ def generate_name
 gen.static_binding('File', 'executable_real?', 'FileObject', 'is_executable_real', argc: 1, pass_env: true, pass_block: false, return_type: :bool)
 gen.static_binding('File', 'exist?', 'FileObject', 'exist', argc: 1, pass_env: true, pass_block: false, return_type: :bool)
 gen.static_binding('File', 'expand_path', 'FileObject', 'expand_path', argc: 1..2, pass_env: true, pass_block: false, return_type: :Object)
+gen.static_binding('File', 'expand_path_old', 'FileObject', 'expand_path_old', argc: 1..2, pass_env: true, pass_block: false, return_type: :Object)
 gen.static_binding('File', 'file?', 'FileObject', 'is_file', argc: 1, pass_env: true, pass_block: false, return_type: :bool)
 gen.static_binding('File', 'ftype', 'FileObject', 'ftype', argc: 1, pass_env: true, pass_block: false, return_type: :Object)
 gen.static_binding('File', 'grpowned?', 'FileObject', 'is_grpowned', argc: 1, pass_env: true, pass_block: false, return_type: :bool)

spec/core/dir/shared/exist.rb

@@ -35,9 +35,7 @@
 
   it "doesn't expand paths" do
     skip "$HOME not valid directory" unless ENV['HOME'] && File.directory?(ENV['HOME'])
-    NATFIXME "File.expand_path incorrect output", exception: SpecFailedException do
-      Dir.send(@method, File.expand_path('~')).should be_true
-    end
+    Dir.send(@method, File.expand_path('~')).should be_true
     Dir.send(@method, '~').should be_false
   end
 

spec/core/file/expand_path_spec.rb

@@ -64,69 +64,53 @@
     # FIXME: these are insane!
     it "expand path with" do
       File.expand_path("../../bin", "/tmp/x").should == "/bin"
-      NATFIXME 'expand path with', exception: SpecFailedException do
-        File.expand_path("../../bin", "/tmp").should == "/bin"
-        File.expand_path("../../bin", "/").should == "/bin"
-      end
+      File.expand_path("../../bin", "/tmp").should == "/bin"
+      File.expand_path("../../bin", "/").should == "/bin"
       File.expand_path("../bin", "tmp/x").should == File.join(@base, 'tmp', 'bin')
       File.expand_path("../bin", "x/../tmp").should == File.join(@base, 'bin')
     end
 
     it "expand_path for common unix path gives a full path" do
       File.expand_path('/tmp/').should =='/tmp'
-      NATFIXME 'expand_path for common unix path gives a full path', exception: SpecFailedException do
-        File.expand_path('/tmp/../../../tmp').should == '/tmp'
-      end
+      File.expand_path('/tmp/../../../tmp').should == '/tmp'
       File.expand_path('').should == Dir.pwd
-      NATFIXME 'expand_path for common unix path gives a full path', exception: SpecFailedException do
-        File.expand_path('./////').should == Dir.pwd
-      end
+      File.expand_path('./////').should == Dir.pwd
       File.expand_path('.').should == Dir.pwd
       File.expand_path(Dir.pwd).should == Dir.pwd
-      NATFIXME 'expand_path for common unix path gives a full path', exception: SpecFailedException do
-        File.expand_path('~/').should == @var_home
-        File.expand_path('~/..badfilename').should == "#{@var_home}/..badfilename"
-        File.expand_path('~/a','~/b').should == "#{@var_home}/a"
-      end
+      File.expand_path('~/').should == @var_home
+      File.expand_path('~/..badfilename').should == "#{@var_home}/..badfilename"
+      File.expand_path('~/a','~/b').should == "#{@var_home}/a"
       File.expand_path('..').should == File.dirname(Dir.pwd)
     end
 
     it "does not replace multiple '/' at the beginning of the path" do
-      File.expand_path('////some/path').should == "////some/path"
+      NATFIXME 'does not replace duplicate leading slashes', exception: SpecFailedException do
+        File.expand_path('////some/path').should == "////some/path"
+      end
     end
 
     it "replaces multiple '/' with a single '/'" do
-      NATFIXME "replaces multiple '/' with a single '/'", exception: SpecFailedException do
-        File.expand_path('/some////path').should == "/some/path"
-      end
+      File.expand_path('/some////path').should == "/some/path"
     end
 
     it "raises an ArgumentError if the path is not valid" do
-      NATFIXME 'raises an ArgumentError if the path is not valid', exception: SpecFailedException do
-        -> { File.expand_path("~a_not_existing_user") }.should raise_error(ArgumentError)
-      end
+      -> { File.expand_path("~a_not_existing_user") }.should raise_error(ArgumentError)
     end
 
     it "expands ~ENV['USER'] to the user's home directory" do
-      NATFIXME "expands ~ENV['USER'] to the user's home directory", exception: SpecFailedException do
-        File.expand_path("~#{ENV['USER']}").should == @db_home
-      end
+      File.expand_path("~#{ENV['USER']}").should == @db_home
     end
 
     it "expands ~ENV['USER']/a to a in the user's home directory" do
-      NATFIXME "expands ~ENV['USER']/a to a in the user's home directory", exception: SpecFailedException do
-        File.expand_path("~#{ENV['USER']}/a").should == "#{@db_home}/a"
-      end
+      File.expand_path("~#{ENV['USER']}/a").should == "#{@db_home}/a"
     end
 
     it "does not expand ~ENV['USER'] when it's not at the start" do
       File.expand_path("/~#{ENV['USER']}/a").should == "/~#{ENV['USER']}/a"
     end
 
     it "expands ../foo with ~/dir as base dir to /path/to/user/home/foo" do
-      NATFIXME 'expands ../foo with ~/dir as base dir to /path/to/user/home/foo', exception: SpecFailedException do
-        File.expand_path('../foo', '~/dir').should == "#{@var_home}/foo"
-      end
+      File.expand_path('../foo', '~/dir').should == "#{@var_home}/foo"
     end
   end
 
@@ -156,21 +140,17 @@
     Encoding.default_external = Encoding::SHIFT_JIS
 
     path = "./a".dup.force_encoding Encoding::CP1251
-    NATFIXME 'returns a String in the same encoding as the argument', exception: SpecFailedException do
-      File.expand_path(path).encoding.should equal(Encoding::CP1251)
+    File.expand_path(path).encoding.should equal(Encoding::CP1251)
 
-      weird_path = [222, 173, 190, 175].pack('C*')
-      File.expand_path(weird_path).encoding.should equal(Encoding::BINARY)
-    end
+    weird_path = [222, 173, 190, 175].pack('C*')
+    File.expand_path(weird_path).encoding.should equal(Encoding::BINARY)
   end
 
   platform_is_not :windows do
     it "expands a path when the default external encoding is BINARY" do
       Encoding.default_external = Encoding::BINARY
       path_8bit = [222, 173, 190, 175].pack('C*')
-      NATFIXME 'expands a path when the default external encoding is BINARY', exception: SpecFailedException do
-        File.expand_path( path_8bit, @rootdir).should == "#{@rootdir}" + path_8bit
-      end
+      File.expand_path( path_8bit, @rootdir).should == "#{@rootdir}" + path_8bit
     end
   end
 
@@ -181,9 +161,7 @@
   platform_is_not :windows do
     it "raises an Encoding::CompatibilityError if the external encoding is not compatible" do
       Encoding.default_external = Encoding::UTF_16BE
-      NATFIXME 'raises an Encoding::CompatibilityError if the external encoding is not compatible', exception: SpecFailedException do
-        -> { File.expand_path("./a") }.should raise_error(Encoding::CompatibilityError)
-      end
+      -> { File.expand_path("./a") }.should raise_error(Encoding::CompatibilityError)
     end
   end
 
@@ -195,9 +173,7 @@
 
   it "does not modify a HOME string argument" do
     str = "~/a"
-    NATFIXME 'converts a pathname to an absolute pathname, using ~ (home) as base', exception: SpecFailedException do
-      File.expand_path(str).should == "#{Dir.home}/a"
-    end
+    File.expand_path(str).should == "#{Dir.home}/a"
     str.should == "~/a"
   end
 
@@ -222,11 +198,9 @@
 
     it "converts a pathname to an absolute pathname, using ~ (home) as base" do
       home = "/rubyspec_home"
-      NATFIXME 'converts a pathname to an absolute pathname, using ~ (home) as base', exception: SpecFailedException do
-        File.expand_path('~').should == home
-        File.expand_path('~', '/tmp/gumby/ddd').should == home
-        File.expand_path('~/a', '/tmp/gumby/ddd').should == File.join(home, 'a')
-      end
+      File.expand_path('~').should == home
+      File.expand_path('~', '/tmp/gumby/ddd').should == home
+      File.expand_path('~/a', '/tmp/gumby/ddd').should == File.join(home, 'a')
     end
 
     it "does not return a frozen string" do
@@ -261,24 +235,18 @@
     } do
       it "uses the user database when passed '~' if HOME is nil" do
         ENV.delete "HOME"
-        NATFIXME "File.expand_path when HOME is not set", exception: SpecFailedException do
-          File.directory?(File.expand_path("~")).should == true
-        end
+        File.directory?(File.expand_path("~")).should == true
       end
 
       it "uses the user database when passed '~/' if HOME is nil" do
         ENV.delete "HOME"
-        NATFIXME "File.expand_path when HOME is not set", exception: SpecFailedException do
-          File.directory?(File.expand_path("~/")).should == true
-        end
+        File.directory?(File.expand_path("~/")).should == true
       end
     end
 
     it "raises an ArgumentError when passed '~' if HOME == ''" do
       ENV["HOME"] = ""
-      NATFIXME "File.expand_path when HOME is not set", exception: SpecFailedException do
-        -> { File.expand_path("~") }.should raise_error(ArgumentError)
-      end
+      -> { File.expand_path("~") }.should raise_error(ArgumentError)
     end
   end
 
@@ -293,9 +261,7 @@
 
     it "raises an ArgumentError" do
       ENV["HOME"] = "non-absolute"
-      NATFIXME 'File.expand_path with a non-absolute HOME raises an ArgumentError', exception: SpecFailedException do
-        -> { File.expand_path("~") }.should raise_error(ArgumentError, 'non-absolute home')
-      end
+      -> { File.expand_path("~") }.should raise_error(ArgumentError, 'non-absolute home')
     end
   end
 end

src/file_object.cpp

@@ -83,6 +83,95 @@ Value FileObject::absolute_path(Env *env, Value path, Value dir) {
 }
 
 Value FileObject::expand_path(Env *env, Value path, Value root) {
+    auto expand_tilde = [&](String &&string) {
+        if (string.is_empty() || string[0] != '~')
+            return string;
+
+        String user;
+        size_t len = 0;
+        for (; len + 1 < string.length(); len++) {
+            if (string[1 + len] == '/')
+                break;
+        }
+        if (len > 0)
+            user = string.substring(1, len);
+
+        String home;
+        if (user.is_empty()) {
+            // if HOME is set, use that...
+            auto home_ptr = getenv("HOME");
+            if (home_ptr) {
+                home = home_ptr;
+            } else {
+                // if not, use the password database...
+                auto pw = getpwuid(getuid());
+                if (!pw)
+                    home = "~";
+                else
+                    home = pw->pw_dir;
+            }
+        } else {
+            auto pw = getpwnam(user.c_str());
+            if (!pw)
+                env->raise("ArgumentError", "user {} doesn't exist", user);
+            else
+                home = pw->pw_dir;
+        }
+
+        if (home.is_empty() || home[0] != '/')
+            env->raise("ArgumentError", "non-absolute home");
+
+        if (string.length() == 1 + user.length()) {
+            return home;
+        } else {
+            assert(string.length() > 1 + user.length());
+            return String::format("{}{}", home, &string[1 + user.length()]);
+        }
+    };
+
+    auto path_string_object = ioutil::convert_using_to_path(env, path);
+    auto path_string = path_string_object->string();
+
+    path_string = expand_tilde(std::move(path_string));
+
+    auto fs_path = std::filesystem::path(path_string.c_str());
+    if (fs_path.is_relative() && root && !root->is_nil()) {
+        root = ioutil::convert_using_to_path(env, root);
+        path_string = expand_tilde(String::format("{}/{}", root->as_string()->string(), path_string));
+        fs_path = std::filesystem::path(path_string.c_str());
+    }
+
+    if (fs_path.string().empty())
+        return new StringObject { std::filesystem::current_path().c_str() };
+
+    if (fs_path.is_relative()) {
+        try {
+            fs_path = std::filesystem::absolute(fs_path);
+        } catch (std::filesystem::filesystem_error &) {
+            env->raise("ArgumentError", "error expanding path 1");
+        }
+    }
+
+    std::filesystem::path expanded;
+    try {
+        expanded = fs_path.lexically_normal();
+    } catch (std::filesystem::filesystem_error &) {
+        env->raise("ArgumentError", "error expanding path");
+    }
+
+    auto default_external = EncodingObject::default_external();
+    auto target_encoding = path_string_object->encoding();
+    if (!default_external->is_compatible_with(target_encoding))
+        target_encoding->raise_compatibility_error(env, default_external);
+
+    auto expanded_string = new StringObject { expanded.c_str(), path_string_object->encoding() };
+    if (expanded_string->length() > 1 && expanded_string->string().last_char() == '/')
+        expanded_string->truncate(expanded_string->length() - 1);
+
+    return expanded_string;
+}
+
+Value FileObject::expand_path_old(Env *env, Value path, Value root) {
     path = ioutil::convert_using_to_path(env, path);
 
     StringObject *merged;