Merge pull request #189 in G/truffleruby from require-keeps-symlinks …

…to master

* commit '57b75473be6f32ee82883045dd91e2737f9a002f': (27 commits)
  Do not expand or canonicalize eval() paths, like MRI
  Add spec for __dir__ and Thread::Backtrace::Location#absolute_path inside eval() with a given filename
  Fix hello-world.gemspec's definition of files
  Autoload Digest::SHA2 instead to fix circular require warning
  Untag MRI tests for #require
  Rename variable
  Warn on circular require like MRI
  Paths given to findFeatureWithExactPath() are always absolute now
  Pass cwd explicitly to avoid calling it many times during #require
  Canonicalize paths in $LOAD_PATH during #require's lookup
  Use dirname() instead of an inlined implementation
  Move filesystem helpers to FeatureLoader
  Add more specs for #require and canonicalizing entries from $LOAD_PATH
  Fix Thread::Backtrace::Location#absolute_path to return a canonical path
  Spec that Thread::Backtrace::Location#absolute_path returns a canonical path
  Simplify dirname() now that it always receives an absolute path
  Only the source directory should be canonicalized by #require_relative
  Canonicalize only where needed in loadMainFile()
  Extract local variable
  Let ensureReadable() create its own File
  ...

lib/truffle/digest.rb

@@ -203,10 +203,10 @@ def block_length
       128
     end
   end
+
+  autoload :SHA2, 'digest/sha2'
 end
 
 def Digest(name)
   Digest.const_get(name.to_sym)
 end
-
-require 'digest/sha2'

spec/ruby/core/kernel/__dir___spec.rb

@@ -5,6 +5,13 @@
     __dir__.should == File.realpath(File.dirname(__FILE__))
   end
 
+  context "when used in eval with a given filename" do
+    it "returns File.dirname(filename)" do
+      eval("__dir__", nil, "foo.rb").should == "."
+      eval("__dir__", nil, "foo/bar.rb").should == "foo"
+    end
+  end
+
   context "when used in eval with top level binding" do
     it "returns the real name of the directory containing the currently-executing file" do
       eval("__dir__", binding).should == File.realpath(File.dirname(__FILE__))

spec/ruby/core/kernel/require_relative_spec.rb

@@ -199,6 +199,46 @@
       $LOADED_FEATURES.should include(@abs_path)
     end
 
+    platform_is_not :windows do
+      describe "with symlinks" do
+        before :each do
+          @symlink_to_code_dir = tmp("codesymlink")
+          File.symlink(CODE_LOADING_DIR, @symlink_to_code_dir)
+          @symlink_basename = File.basename(@symlink_to_code_dir)
+          @requiring_file = tmp("requiring")
+        end
+
+        after :each do
+          rm_r @symlink_to_code_dir, @requiring_file
+        end
+
+        it "does not canonicalize the path and stores a path with symlinks" do
+          symlink_path = "#{@symlink_basename}/load_fixture.rb"
+          absolute_path = "#{tmp("")}#{symlink_path}"
+          canonical_path = "#{CODE_LOADING_DIR}/load_fixture.rb"
+          touch(@requiring_file) { |f|
+            f.puts "require_relative #{symlink_path.inspect}"
+          }
+          load(@requiring_file)
+          ScratchPad.recorded.should == [:loaded]
+
+          features = $LOADED_FEATURES.select { |path| path.end_with?('load_fixture.rb') }
+          features.should include(absolute_path)
+          features.should_not include(canonical_path)
+        end
+
+        it "stores the same path that __FILE__ returns in the required file" do
+          symlink_path = "#{@symlink_basename}/load_fixture_and__FILE__.rb"
+          touch(@requiring_file) { |f|
+            f.puts "require_relative #{symlink_path.inspect}"
+          }
+          load(@requiring_file)
+          loaded_feature = $LOADED_FEATURES.last
+          ScratchPad.recorded.should == [loaded_feature]
+        end
+      end
+    end
+
     it "does not store the path if the load fails" do
       saved_loaded_features = $LOADED_FEATURES.dup
       lambda { require_relative("#{@dir}/raise_fixture.rb") }.should raise_error(RuntimeError)

spec/ruby/core/kernel/require_spec.rb

@@ -17,7 +17,6 @@
   end
 
   it_behaves_like :kernel_require_basic, :require, CodeLoadingSpecs::Method.new
-
   it_behaves_like :kernel_require, :require, CodeLoadingSpecs::Method.new
 end
 
@@ -31,6 +30,5 @@
   end
 
   it_behaves_like :kernel_require_basic, :require, Kernel
-
   it_behaves_like :kernel_require, :require, Kernel
 end

spec/ruby/core/kernel/shared/require.rb

@@ -305,6 +305,80 @@
       $LOADED_FEATURES.should include(@path)
     end
 
+    platform_is_not :windows do
+      describe "with symlinks" do
+        before :each do
+          @symlink_to_code_dir = tmp("codesymlink")
+          File.symlink(CODE_LOADING_DIR, @symlink_to_code_dir)
+
+          $LOAD_PATH.delete(CODE_LOADING_DIR)
+          $LOAD_PATH.unshift(@symlink_to_code_dir)
+        end
+
+        after :each do
+          rm_r @symlink_to_code_dir
+        end
+
+        it "does not canonicalize the path and stores a path with symlinks" do
+          symlink_path = "#{@symlink_to_code_dir}/load_fixture.rb"
+          canonical_path = "#{CODE_LOADING_DIR}/load_fixture.rb"
+          @object.require(symlink_path).should be_true
+          ScratchPad.recorded.should == [:loaded]
+
+          features = $LOADED_FEATURES.select { |path| path.end_with?('load_fixture.rb') }
+          features.should include(symlink_path)
+          features.should_not include(canonical_path)
+        end
+
+        it "stores the same path that __FILE__ returns in the required file" do
+          symlink_path = "#{@symlink_to_code_dir}/load_fixture_and__FILE__.rb"
+          @object.require(symlink_path).should be_true
+          loaded_feature = $LOADED_FEATURES.last
+          ScratchPad.recorded.should == [loaded_feature]
+        end
+      end
+
+      describe "with symlinks in the required feature and $LOAD_PATH" do
+        before :each do
+          @dir = tmp("realdir")
+          mkdir_p @dir
+          @file = "#{@dir}/realfile.rb"
+          touch(@file) { |f| f.puts 'ScratchPad << __FILE__' }
+
+          @symlink_to_dir = tmp("symdir").freeze
+          File.symlink(@dir, @symlink_to_dir)
+          @symlink_to_file = "#{@dir}/symfile.rb"
+          File.symlink("realfile.rb", @symlink_to_file)
+        end
+
+        after :each do
+          rm_r @dir, @symlink_to_dir
+        end
+
+        ruby_version_is ""..."2.4.4" do
+          it "canonicalizes neither the entry in $LOAD_PATH nor the filename passed to #require" do
+            $LOAD_PATH.unshift(@symlink_to_dir)
+            @object.require("symfile").should be_true
+            loaded_feature = "#{@symlink_to_dir}/symfile.rb"
+            ScratchPad.recorded.should == [loaded_feature]
+            $".last.should == loaded_feature
+            $LOAD_PATH[0].should == @symlink_to_dir
+          end
+        end
+
+        ruby_version_is "2.4.4" do
+          it "canonicalizes the entry in $LOAD_PATH but not the filename passed to #require" do
+            $LOAD_PATH.unshift(@symlink_to_dir)
+            @object.require("symfile").should be_true
+            loaded_feature = "#{@dir}/symfile.rb"
+            ScratchPad.recorded.should == [loaded_feature]
+            $".last.should == loaded_feature
+            $LOAD_PATH[0].should == @symlink_to_dir
+          end
+        end
+      end
+    end
+
     it "does not store the path if the load fails" do
       $LOAD_PATH << CODE_LOADING_DIR
       saved_loaded_features = $LOADED_FEATURES.dup
@@ -417,7 +491,7 @@
       $LOADED_FEATURES.should include(@path)
     end
 
-    it "canonicalizes non-unique absolute paths" do
+    it "expands absolute paths containing .." do
       path = File.join CODE_LOADING_DIR, "..", "code", "load_fixture.rb"
       @object.require(path).should be_true
       $LOADED_FEATURES.should include(@path)

spec/ruby/core/thread/backtrace/location/absolute_path_spec.rb

@@ -9,4 +9,42 @@
   it 'returns the absolute path of the call frame' do
     @frame.absolute_path.should == File.realpath(__FILE__)
   end
+
+  context "when used in eval with a given filename" do
+    it "returns filename" do
+      code = "caller_locations(0)[0].absolute_path"
+      eval(code, nil, "foo.rb").should == "foo.rb"
+      eval(code, nil, "foo/bar.rb").should == "foo/bar.rb"
+    end
+  end
+
+  platform_is_not :windows do
+    before :each do
+      @file = fixture(__FILE__, "absolute_path.rb")
+      @symlink = tmp("symlink.rb")
+      File.symlink(@file, @symlink)
+      ScratchPad.record []
+    end
+
+    after :each do
+      rm_r @symlink
+    end
+
+    it "returns a canonical path without symlinks, even when __FILE__ does not" do
+      realpath = File.realpath(@symlink)
+      realpath.should_not == @symlink
+
+      load @symlink
+      ScratchPad.recorded.should == [@symlink, realpath]
+    end
+
+    it "returns a canonical path without symlinks, even when __FILE__ is removed" do
+      realpath = File.realpath(@symlink)
+      realpath.should_not == @symlink
+
+      ScratchPad << -> { rm_r(@symlink) }
+      load @symlink
+      ScratchPad.recorded.should == [@symlink, realpath]
+    end
+  end
 end

spec/ruby/core/thread/backtrace/location/fixtures/absolute_path.rb

@@ -0,0 +1,4 @@
+action = ScratchPad.recorded.pop
+ScratchPad << __FILE__
+action.call if action
+ScratchPad << caller_locations(0)[0].absolute_path

spec/ruby/fixtures/code/load_fixture_and__FILE__.rb

@@ -0,0 +1 @@
+ScratchPad << __FILE__

spec/tags/core/kernel/require_tags.txt

@@ -18,5 +18,3 @@ slow:Kernel#require (concurrently) blocks based on the path
 slow:Kernel.require (concurrently) blocks based on the path
 slow:Kernel#require ($LOADED_FEATURES) complex, enumerator, rational, thread and unicode_normalize are already required
 slow:Kernel.require ($LOADED_FEATURES) complex, enumerator, rational, thread and unicode_normalize are already required
-fails:Kernel#require (path resolution) loads a file that recursively requires itself
-fails:Kernel.require (path resolution) loads a file that recursively requires itself

src/main/java/org/truffleruby/aot/ParserCache.java

@@ -45,19 +45,19 @@ public class ParserCache {
 
     private final Map<String, RootParseNode> cache = new HashMap<>();
 
-    public void add(String canonicalPath) {
-        cache.put(canonicalPath, load(canonicalPath));
+    public void add(String feature) {
+        cache.put(feature, load(feature));
     }
 
-    private RootParseNode load(String canonicalPath) {
+    private RootParseNode load(String feature) {
         final TranslatorDriver driver = new TranslatorDriver(null);
         final StaticScope staticScope = new StaticScope(StaticScope.Type.LOCAL, null);
         final DynamicScope dynamicScope = new DynamicScope(staticScope);
         final ParserConfiguration parserConfiguration = new ParserConfiguration(null, 0, false, true, false);
 
         final RubySource source;
         try {
-            source = SourceLoader.loadNoLogging(null, canonicalPath, true);
+            source = SourceLoader.loadNoLogging(null, feature, true);
         } catch (IOException e) {
             throw new JavaException(e);
         }

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -101,7 +101,6 @@
 import org.truffleruby.language.arguments.RubyArguments;
 import org.truffleruby.language.backtrace.Activation;
 import org.truffleruby.language.backtrace.Backtrace;
-import org.truffleruby.language.control.JavaException;
 import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.language.dispatch.CallDispatchHeadNode;
 import org.truffleruby.language.dispatch.DispatchNode;
@@ -146,8 +145,8 @@
 import org.truffleruby.parser.RubySource;
 
 import java.io.File;
-import java.io.IOException;
 import java.io.PrintStream;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -1387,7 +1386,7 @@ public boolean requireRelative(String feature,
         }
 
         @TruffleBoundary
-        private String getFullPath(final String featureString) {
+        private String getFullPath(String featureString) {
             final String featurePath;
 
             if (new File(featureString).isAbsolute()) {
@@ -1405,28 +1404,17 @@ private String getFullPath(final String featureString) {
                     throw new RaiseException(getContext(), coreExceptions().loadError("cannot infer basepath", featureString, this));
                 }
 
-                featurePath = dirname(sourcePath) + "/" + featureString;
-            }
+                final String cwd = getContext().getFeatureLoader().getWorkingDirectory();
+                sourcePath = getContext().getFeatureLoader().canonicalize(cwd, sourcePath);
 
-            try {
-                return new File(featurePath).getCanonicalPath();
-            } catch (IOException e) {
-                throw new JavaException(e);
+                featurePath = getContext().getFeatureLoader().dirname(sourcePath) + "/" + featureString;
             }
-        }
 
-        private String dirname(String path) {
-            if (path.charAt(0) == File.separatorChar) {
-                return path.substring(0, path.lastIndexOf(File.separatorChar));
-            } else {
-                final String workingDirectory = getContext().getFeatureLoader().getWorkingDirectory();
-                final int lastIndex = path.lastIndexOf(File.separatorChar);
-                if (lastIndex == -1) {
-                    return workingDirectory;
-                } else {
-                    return workingDirectory + "/" + path.substring(0, lastIndex);
-                }
-            }
+            // Normalize the path like File.expand_path() (e.g., remove "../"), but do not resolve
+            // symlinks. MRI does this for #require_relative always, but not for #require, so we
+            // need to do it to be compatible in the case the path does not exist, so the
+            // LoadError's #path is the same as MRI's.
+            return Paths.get(featurePath).normalize().toString();
         }
     }
 

src/main/java/org/truffleruby/core/kernel/TruffleKernelNodes.java

@@ -81,16 +81,17 @@ public boolean load(VirtualFrame frame, DynamicObject file, boolean wrap,
                 throw new UnsupportedOperationException();
             }
 
+            final String feature = StringOperations.getString(file);
             try {
-                final RubySource source = getContext().getSourceLoader().load(StringOperations.getString(file));
+                final RubySource source = getContext().getSourceLoader().load(feature);
                 final RubyRootNode rootNode = getContext().getCodeLoader().parse(source, ParserContext.TOP_LEVEL, null, true, this);
                 final CodeLoader.DeferredCall deferredCall = getContext().getCodeLoader().prepareExecute(
                         ParserContext.TOP_LEVEL, DeclarationContext.topLevel(getContext()), rootNode, null,
                         getContext().getCoreLibrary().getMainObject());
                 deferredCall.call(callNode);
             } catch (IOException e) {
                 errorProfile.enter();
-                throw new RaiseException(getContext(), coreExceptions().loadErrorCannotLoad(file.toString(), this));
+                throw new RaiseException(getContext(), coreExceptions().loadErrorCannotLoad(feature, this));
             }
 
             return true;

src/main/java/org/truffleruby/core/thread/ThreadBacktraceLocationNodes.java

@@ -15,6 +15,8 @@
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.source.SourceSection;
 
+import java.io.File;
+
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.Layouts;
 import org.truffleruby.RubyContext;
@@ -50,7 +52,14 @@ public DynamicObject absolutePath(DynamicObject threadBacktraceLocation,
             if (path == null) {
                 return coreStrings().UNKNOWN.createInstance();
             } else {
-                return makeStringNode.fromRope(getContext().getPathToRopeCache().getCachedPath(path));
+                final String canonicalPath;
+                if (new File(path).isAbsolute()) { // A normal file
+                    canonicalPath = getContext().getFeatureLoader().canonicalize(null, path);
+                } else { // eval()
+                    canonicalPath = path;
+                }
+
+                return makeStringNode.fromRope(getContext().getPathToRopeCache().getCachedPath(canonicalPath));
             }
         }