Merge pull request #1804 from opal/elia/errors-and-filenames-review

better errors, better backtraces, filenames with extensions
opal · Apr 26, 2018 · ecf5348 · ecf5348
2 parents 1bf23ff + e67d849
commit ecf5348
Show file tree

Hide file tree

Showing 30 changed files with 325 additions and 151 deletions.
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -105,6 +105,7 @@ Lint/BooleanSymbol:
 Lint/InheritException:
   Exclude:
     - 'lib/opal/builder.rb'
+    - 'lib/opal/errors.rb'
     - 'opal/**/*.rb'
     - 'stdlib/**/*.rb'
 
@@ -350,6 +351,8 @@ Naming/AccessorMethodName:
   Exclude:
     # StringScanner has method 'get_byte'
     - 'stdlib/strscan.rb'
+    # Exception has method 'set_backtrace'
+    - 'opal/corelib/error.rb'
 
 Style/RescueStandardError:
   Enabled: false
@@ -378,3 +381,17 @@ Style/MutableConstant:
   Exclude:
     - 'opal/**/*.rb'
     - 'stdlib/**/*.rb'
+
+Style/EmptyCaseCondition:
+  Enabled: false
+
+
+# Use whatever suites the situation, sometimes assign_inside_condition is
+# more readable over assign_to_condition despite the risk of repeating the
+# variable name.
+Style/ConditionalAssignment:
+  Enabled: false
+
+Naming/MemoizedInstanceVariableName:
+  Exclude:
+    - lib/opal/parser/patch.rb # it's a monkey-patch on the parser gem
diff --git a/lib/opal/builder.rb b/lib/opal/builder.rb
@@ -80,19 +80,21 @@ def self.build(*args, &block)
 
     def build(path, options = {})
       source = read(path)
+      path = path_reader.expand(path)
       build_str(source, path, options)
     end
 
-    def build_str(source, filename, options = {})
+    def build_str(source, rel_path, options = {})
       return if source.nil?
-      path = path_from_filename(filename)
-      asset = processor_for(source, filename, path, options)
-      requires = preload + asset.requires + tree_requires(asset, path)
+      abs_path = expand_path(rel_path)
+      rel_path = expand_ext(rel_path)
+      asset = processor_for(source, rel_path, abs_path, options)
+      requires = preload + asset.requires + tree_requires(asset, abs_path)
       requires.map { |r| process_require(r, options) }
       processed << asset
       self
     rescue MissingRequire => error
-      raise error, "A file required by #{filename.inspect} wasn't found.\n#{error.message}", error.backtrace
+      raise error, "A file required by #{rel_path.inspect} wasn't found.\n#{error.message}", error.backtrace
     end
 
     def build_require(path, options = {})
@@ -132,38 +134,35 @@ def append_paths(*paths)
 
     private
 
-    def tree_requires(asset, path)
-      dirname =
-        if path.nil? || path.empty?
-          Dir.pwd
-        else
-          File.dirname(File.expand_path(path))
-        end
-
-      paths = path_reader.paths.map { |p| File.expand_path(p) }
+    def tree_requires(asset, asset_path)
+      dirname = asset_path.to_s.empty? ? Pathname.pwd : Pathname(asset_path).expand_path.dirname
+      abs_base_paths = path_reader.paths.map { |p| File.expand_path(p) }
 
       asset.required_trees.flat_map do |tree|
-        expanded = File.expand_path(tree, dirname)
-        base = paths.find { |p| expanded.start_with?(p) }
-        next [] if base.nil?
+        abs_tree_path = dirname.join(tree).expand_path.to_s
+        abs_base_path = abs_base_paths.find { |p| abs_tree_path.start_with?(p) }
 
-        globs = extensions.map { |ext| File.join base, tree, '**', "*.#{ext}" }
+        if abs_base_path
+          abs_base_path = Pathname(abs_base_path)
+          entries_glob  = Pathname(abs_tree_path).join('**', "*{.js,}.{#{extensions.join ','}}")
 
-        Dir[*globs].map do |file|
-          Pathname(file).relative_path_from(Pathname(base)).to_s.gsub(/(\.js)?(\.(?:#{extensions.join '|'}))#{REGEXP_END}/, '')
+          Pathname.glob(entries_glob).map { |file| file.relative_path_from(abs_base_path).to_s }
+        else
+          [] # the tree is not part of any known base path
         end
       end
     end
 
-    def processor_for(source, filename, path, options)
-      processor = processors.find { |p| p.match? path } ||
-                  raise(ProcessorNotFound, "can't find processor for filename: " \
-                                           "#{filename.inspect}, "\
-                                           "path: #{path.inspect}, "\
+    def processor_for(source, rel_path, abs_path, options)
+      processor = processors.find { |p| p.match? abs_path } ||
+                  raise(ProcessorNotFound, "can't find processor for rel_path: " \
+                                           "#{rel_path.inspect}, "\
+                                           "abs_path: #{abs_path.inspect}, "\
                                            "source: #{source.inspect}, "\
                                            "processors: #{processors.inspect}"
                   )
-      processor.new(source, filename, @compiler_options.merge(options))
+
+      processor.new(source, rel_path, @compiler_options.merge(options))
     end
 
     def read(path)
@@ -186,48 +185,61 @@ def read(path)
       end
     end
 
-    def process_require(filename, options)
-      filename = filename.gsub(/\.(rb|js|opal)#{REGEXP_END}/, '')
-      return if prerequired.include?(filename)
-      return if already_processed.include?(filename)
-      already_processed << filename
+    def process_require(rel_path, options)
+      return if prerequired.include?(rel_path)
+      return if already_processed.include?(rel_path)
+      already_processed << rel_path
 
-      source = stub?(filename) ? '' : read(filename)
+      source = stub?(rel_path) ? '' : read(rel_path)
 
       if source.nil?
-        message = "can't find file: #{filename.inspect}"
+        message = "can't find file: #{rel_path.inspect}"
         case missing_require_severity
         when :error   then raise LoadError, message
-        when :warning then warn "can't find file: #{filename.inspect}"
+        when :warning then warn "can't find file: #{rel_path.inspect}"
         when :ignore  then # noop
         end
 
         return # the handling is delegated to the runtime
       end
 
-      path = path_from_filename(filename)
-      asset = processor_for(source, filename, path, options.merge(requirable: true))
-      process_requires(filename, asset.requires + tree_requires(asset, path), options)
+      abs_path = expand_path(rel_path)
+      rel_path = expand_ext(rel_path)
+      asset = processor_for(source, rel_path, abs_path, options.merge(requirable: true))
+      process_requires(rel_path, asset.requires + tree_requires(asset, abs_path), options)
       processed << asset
     end
 
-    def path_from_filename(filename)
-      return if stub?(filename)
-      (path_reader.expand(filename) || File.expand_path(filename)).to_s
+    def expand_ext(path)
+      abs_path = path_reader.expand(path)
+
+      if abs_path
+        File.join(
+          File.dirname(path),
+          File.basename(abs_path)
+        )
+      else
+        path
+      end
+    end
+
+    def expand_path(path)
+      return if stub?(path)
+      (path_reader.expand(path) || File.expand_path(path)).to_s
     end
 
-    def process_requires(filename, requires, options)
+    def process_requires(rel_path, requires, options)
       requires.map { |r| process_require(r, options) }
     rescue MissingRequire => error
-      raise error, "A file required by #{filename.inspect} wasn't found.\n#{error.message}", error.backtrace
+      raise error, "A file required by #{rel_path.inspect} wasn't found.\n#{error.message}", error.backtrace
     end
 
     def already_processed
       @already_processed ||= Set.new
     end
 
-    def stub?(filename)
-      stubs.include?(filename)
+    def stub?(path)
+      stubs.include?(path)
     end
 
     def extensions

diff --git a/lib/opal/builder_processors.rb b/lib/opal/builder_processors.rb
@@ -85,7 +85,7 @@ def source_map
 
       def compiled
         @compiled ||= begin
-          compiler = compiler_for(@source, file: @filename.gsub(/\.(rb|js|opal)#{REGEXP_END}/, ''))
+          compiler = compiler_for(@source, file: @filename)
           compiler.compile
           compiler
         end
@@ -100,10 +100,7 @@ def requires
       end
 
       def required_trees
-        compiled.required_trees.map do |tree|
-          # Remove any leading ./ after joining to dirname
-          File.join(File.dirname(@filename), tree).sub(%r{^(\./)*}, '')
-        end
+        compiled.required_trees
       end
 
       # Also catch a files with missing extensions and nil.

diff --git a/lib/opal/cli.rb b/lib/opal/cli.rb
@@ -114,7 +114,7 @@ def create_builder
 
     def show_sexp
       evals_or_file do |contents, filename|
-        buffer = ::Opal::Source::Buffer.new(filename)
+        buffer = ::Opal::Parser::SourceBuffer.new(filename)
         buffer.source = contents
         sexp = Opal::Parser.default_parser.parse(buffer)
         output.puts sexp.inspect

diff --git a/lib/opal/compiler.rb b/lib/opal/compiler.rb
@@ -5,6 +5,7 @@
 require 'opal/fragment'
 require 'opal/nodes'
 require 'opal/eof_content'
+require 'opal/errors'
 
 module Opal
   # Compile a string of ruby code into javascript.
@@ -177,15 +178,15 @@ def compile
     end
 
     def parse
-      @buffer = ::Opal::Source::Buffer.new(file, 1)
+      @buffer = ::Opal::Parser::SourceBuffer.new(file, 1)
       @buffer.source = @source
 
       @parser = Opal::Parser.default_parser
 
       begin
         sexp, comments, tokens = @parser.tokenize(@buffer)
-      rescue ::Parser::SyntaxError => error
-        raise ::SyntaxError, error.message, error.backtrace
+      rescue ::Opal::Error, ::Parser::SyntaxError => error
+        raise ::Opal::SyntaxError.with_opal_backtrace(error, file)
       end
 
       @sexp = s(:top, sexp || s(:nil))
@@ -225,7 +226,7 @@ def method_calls
     # method simply appends the filename and curent line number onto
     # the message and raises it.
     def error(msg, line = nil)
-      raise SyntaxError, "#{msg} :#{file}:#{line}"
+      raise ::Opal::SyntaxError, "#{msg} -- #{file}:#{line}"
     end
 
     # This is called when a parsing/processing warning occurs. This

diff --git a/lib/opal/errors.rb b/lib/opal/errors.rb
@@ -1,8 +1,12 @@
 # frozen_string_literal: true
 
 module Opal
+  # Generic Opal error
+  class Error < StandardError
+  end
+
   # raised if Gem not found in Opal#use_gem
-  class GemNotFound < StandardError
+  class GemNotFound < Error
     # name of gem that not found
     attr_reader :gem_name
 
@@ -12,4 +16,71 @@ def initialize(gem_name)
       super("can't find gem #{gem_name}")
     end
   end
+
+  class CompilationError < Error
+    attr_accessor :location
+  end
+
+  class ParsingError < CompilationError
+  end
+
+  class RewritingError < ParsingError
+  end
+
+  class SyntaxError < ::SyntaxError
+    attr_accessor :location
+
+    # Not redefining #backtrace because of https://bugs.ruby-lang.org/issues/14693
+    def self.with_opal_backtrace(error, path)
+      new_error = new(error.message)
+      backtrace = error.backtrace.to_a
+      backtrace.unshift OpalBacktraceLocation.new(error, path).to_s
+      new_error.set_backtrace backtrace
+      new_error
+    end
+  end
+
+  # Loosely compatible with Thread::Backtrace::Location
+  class OpalBacktraceLocation
+    attr_reader :error, :path
+
+    def initialize(error, path)
+      @error = error
+      @path = path
+    end
+
+    def location
+      if error.respond_to? :location
+        error.location
+      elsif error.respond_to?(:diagnostic) && error.diagnostic.respond_to?(:location)
+        error.diagnostic.location
+      end
+    end
+
+    def lineno
+      location.line if location
+    end
+
+    # Use source code as the label
+    def label
+      case
+      when location.respond_to?(:source_line)
+        location.source_line
+      when location.respond_to?(:expression)
+        location.expression.source_line
+      end
+    end
+
+    def to_s
+      string = path
+      string += ":#{lineno}" if lineno
+      string += ':in '
+      if label
+        string += "`#{label}'"
+      else
+        string += 'unknown'
+      end
+      string
+    end
+  end
 end
diff --git a/lib/opal/nodes/def.rb b/lib/opal/nodes/def.rb
@@ -144,8 +144,7 @@ def compile_arity_check
       def source_location
         file = @sexp.loc.expression.source_buffer.name
         line = @sexp.loc.line
-
-        "['#{file}.rb', #{line}]"
+        "['#{file}', #{line}]"
       end
 
       def comments_code