Improve errors and diagnostics (#757)

* Add custom Surface.CompileError, which can handle column and snippet

* Normalize compile error across Elixir versions

* Add Surface.Case with ANSI helpers

lib/mix/tasks/compile/surface/validate_components.ex

@@ -41,7 +41,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponents do
       missing_props_names = required_props_names -- existing_props_names
 
       for prop_name <- missing_props_names do
-        message = "Missing required property \"#{prop_name}\" for component <#{node_alias}>"
+        message = "missing required property \"#{prop_name}\" for component <#{node_alias}>"
 
         message =
           if prop_name == :id and Helpers.is_stateful_component(module) do

lib/surface/base_component.ex

@@ -123,7 +123,9 @@ defmodule Surface.BaseComponent do
     components = Enum.uniq_by(components_calls, & &1.component)
 
     {imports, requires} =
-      for %{component: mod, line: line, dep_type: dep_type} <- components, mod != env.module, reduce: {[], []} do
+      for %{component: mod, file: file, line: line, dep_type: dep_type} <- components,
+          mod != env.module,
+          reduce: {[], []} do
         {imports, requires} ->
           case dep_type do
             :export ->
@@ -135,9 +137,15 @@ defmodule Surface.BaseComponent do
                ], requires}
 
             :compile ->
+              # We use `require` for macros or when there's an error loading the
+              # module. This way if the missing/failing module is created/fixed,
+              # Elixir will recompile this file.
+              # NOTE: there's a bug in Elixir that report the error with the wrong line
+              # in versions <= 1.17. See https://github.com/elixir-lang/elixir/issues/13542
+              # for details.
               {imports,
                [
-                 quote line: line do
+                 quote file: file, line: line do
                    require(unquote(mod)).__info__(:module)
                  end
                  | requires

lib/surface/catalogue.ex

@@ -104,15 +104,15 @@ defmodule Surface.Catalogue do
         subject
 
       _ ->
-        message = """
-        no subject defined for #{inspect(type)}
+        message = "no subject defined for #{inspect(type)}"
 
-        Hint: You can define the subject using the :subject option. Example:
+        hint = """
+        you can define the subject using the :subject option. Example:
 
           use #{inspect(type)}, subject: MyApp.MyButton
         """
 
-        Surface.IOHelper.compile_error(message, caller.file, caller.line)
+        Surface.IOHelper.compile_error(message, hint, caller.file, caller.line)
     end
   end
 

lib/surface/compile_error.ex

@@ -0,0 +1,69 @@
+defmodule Surface.CompileError do
+  @moduledoc """
+  An exception raised when there's a Surface compile error.
+
+  The following fields of this exceptions are public and can be accessed freely:
+
+    * `:file` (`t:Path.t/0` or `nil`) - the file where the error occurred, or `nil` if
+      the error occurred in code that did not come from a file
+    * `:line` - the line where the error occurred
+    * `:column` - the column where the error occurred
+    * `:description` - a description of the error
+    * `:hint` - a hint to help the user to fix the issue
+
+  """
+
+  @support_snippet Version.match?(System.version(), ">= 1.17.0")
+
+  defexception [:file, :line, :column, :snippet, :hint, description: "compile error"]
+
+  @impl true
+  def message(%{
+        file: file,
+        line: line,
+        column: column,
+        description: description,
+        hint: hint
+      }) do
+    format_message(file, line, column, description, hint)
+  end
+
+  if @support_snippet do
+    defp format_message(file, line, column, description, hint) do
+      message =
+        if File.exists?(file) do
+          {lineCode, _} = File.stream!(file) |> Stream.with_index() |> Enum.at(line - 1)
+          lineCode = String.trim_trailing(lineCode)
+          :elixir_errors.format_snippet(:error, {line, column}, file, description, lineCode, %{})
+        else
+          prefix = IO.ANSI.format([:red, "error:"])
+          "#{prefix} #{description}"
+        end
+
+      hint =
+        if hint do
+          "\n\n" <> :elixir_errors.format_snippet(:hint, nil, nil, hint, nil, %{})
+        else
+          ""
+        end
+
+      location = Exception.format_file_line_column(Path.relative_to_cwd(file), line, column)
+      location <> "\n" <> message <> hint
+    end
+  else
+    defp format_message(file, line, column, description, hint) do
+      location = Exception.format_file_line_column(Path.relative_to_cwd(file), line, column)
+
+      hint =
+        if hint do
+          prefix = IO.ANSI.format([:blue, "hint:"])
+          "\n\n#{prefix} " <> hint
+        else
+          ""
+        end
+
+      prefix = IO.ANSI.format([:red, "error:"])
+      location <> "\n#{prefix} " <> description <> hint
+    end
+  end
+end

lib/surface/compiler.ex

@@ -283,13 +283,15 @@ defmodule Surface.Compiler do
         {:ok, ast} ->
           process_directives(ast)
 
-        {:error, {message, line}, meta} ->
-          IOHelper.warn(message, compile_meta.caller, meta.file, line)
+        {:error, {message, line, column}, meta} ->
+          IOHelper.warn(message, compile_meta.caller, meta.file, {line, column})
           %AST.Error{message: message, meta: meta}
 
-        {:error, {message, details, line}, meta} ->
-          details = if details, do: "\n\n" <> details, else: ""
-          IOHelper.warn(message <> details, compile_meta.caller, meta.file, line)
+        {:error, {message, details, line, column}, meta} ->
+          # TODO: turn it back as a warning when using @after_verify in Elixir >= 0.14.
+          # Make sure to check if the genarated `require <component>.__info__()` doesn't get called,
+          # raising Elixir's CompileError.
+          IOHelper.compile_error(message, details, meta.file, {line, column})
           %AST.Error{message: message, meta: meta}
       end
     end
@@ -854,8 +856,9 @@ defmodule Surface.Compiler do
       end
     else
       false ->
-        {:error, {"cannot render <#{node_alias}> (MacroComponents must export an expand/3 function)", meta.line},
-         meta}
+        {:error,
+         {"cannot render <#{node_alias}> (MacroComponents must export an expand/3 function)", meta.line,
+          meta.column}, meta}
 
       error ->
         handle_convert_node_to_ast_error(node_alias, error, meta)
@@ -1246,7 +1249,7 @@ defmodule Surface.Compiler do
         !Map.has_key?(slot_entries, name) or
           Enum.all?(Map.get(slot_entries, name, []), &Helpers.is_blank_or_empty/1) do
       message = "missing required slot \"#{name}\" for component <#{meta.node_alias}>"
-      IOHelper.warn(message, meta.caller, meta.file, meta.line)
+      IOHelper.warn(message, meta.caller, meta.file, {meta.line, meta.column})
     end
 
     for {slot_name, slot_entry_instances} <- slot_entries,
@@ -1508,13 +1511,13 @@ defmodule Surface.Compiler do
   defp handle_convert_node_to_ast_error(name, error, meta) do
     case error do
       {:error, message, details} ->
-        {:error, {"cannot render <#{name}> (#{message})", details, meta.line}, meta}
+        {:error, {"cannot render <#{name}> (#{message})", details, meta.line, meta.column}, meta}
 
       {:error, message} ->
-        {:error, {"cannot render <#{name}> (#{message})", meta.line}, meta}
+        {:error, {"cannot render <#{name}> (#{message})", meta.line, meta.column}, meta}
 
       _ ->
-        {:error, {"cannot render <#{name}>", meta.line}, meta}
+        {:error, {"cannot render <#{name}>", meta.line, meta.column}, meta}
     end
   end
 

lib/surface/compiler/eex_engine.ex

@@ -874,6 +874,7 @@ defmodule Surface.Compiler.EExEngine do
            meta:
              %AST.Meta{
                module: mod,
+               file: file,
                line: line,
                column: col
              } = meta
@@ -882,7 +883,7 @@ defmodule Surface.Compiler.EExEngine do
        ])
        when not is_nil(mod) do
     %{attributes: attributes, directives: directives, meta: %{node_alias: node_alias}} = component
-    store_component_call(meta.caller.module, node_alias, mod, attributes, directives, line, col, :compile)
+    store_component_call(meta.caller.module, node_alias, mod, attributes, directives, file, line, col, :compile)
     [to_dynamic_nested_html(children) | to_dynamic_nested_html(nodes)]
   end
 
@@ -960,12 +961,12 @@ defmodule Surface.Compiler.EExEngine do
         {requires, Map.put(by_name, name, Enum.reverse(slot_entries))}
       end)
 
-    %{caller: caller, node_alias: node_alias, line: line, column: col} = component.meta
+    %{caller: caller, node_alias: node_alias, file: file, line: line, column: col} = component.meta
     %{props: props, directives: directives} = component
 
     if type != AST.FunctionComponent do
       dep_type = if is_atom(mod) and function_exported?(mod, :transform, 1), do: :compile, else: :export
-      store_component_call(caller.module, node_alias, mod, props, directives, line, col, dep_type)
+      store_component_call(caller.module, node_alias, mod, props, directives, file, line, col, dep_type)
     end
 
     [requires, %{component | slot_entries: slot_entries_by_name} | to_dynamic_nested_html(nodes)]
@@ -984,6 +985,7 @@ defmodule Surface.Compiler.EExEngine do
       module,
       attributes,
       directives,
+      meta.file,
       meta.line,
       meta.column,
       :compile
@@ -1143,7 +1145,7 @@ defmodule Surface.Compiler.EExEngine do
     |> Macro.var(nil)
   end
 
-  defp store_component_call(module, node_alias, component, props, directives, line, col, dep_type)
+  defp store_component_call(module, node_alias, component, props, directives, file, line, col, dep_type)
        when dep_type in [:compile, :export] do
     # No need to store dynamic modules
     if !match?(%Surface.AST.AttributeExpr{}, component) do
@@ -1152,6 +1154,7 @@ defmodule Surface.Compiler.EExEngine do
         component: component,
         props: map_attrs(props),
         directives: map_attrs(directives),
+        file: file,
         line: line,
         column: col,
         dep_type: dep_type

lib/surface/compiler/helpers.ex

@@ -291,7 +291,7 @@ defmodule Surface.Compiler.Helpers do
 
   defp hint_for_unloaded_module(node_alias) do
     """
-    Hint: Make sure module `#{node_alias}` can be successfully compiled.
+    make sure module `#{node_alias}` can be successfully compiled.
 
     If the module is namespaced, you can use its full name. For instance:
 

lib/surface/io_helper.ex

@@ -13,15 +13,51 @@ defmodule Surface.IOHelper do
     IO.warn(message, stacktrace)
   end
 
+  if Version.match?(System.version(), ">= 1.14.0") do
+    def warn(message, _caller, file, {line, column}) do
+      IO.warn(message, file: file, line: line, column: column)
+    end
+  else
+    # TODO: Remove this clause in Surface v0.13 and set required elixir to >= v1.14
+    def warn(message, caller, file, {line, _column}) do
+      warn(message, caller, file, line)
+    end
+  end
+
   def warn(message, caller, file, line) do
     stacktrace = Macro.Env.stacktrace(%{caller | file: file, line: line})
 
     IO.warn(message, stacktrace)
   end
 
-  @spec compile_error(String.t(), String.t(), integer()) :: no_return()
+  if Version.match?(System.version(), ">= 1.14.0") do
+    def compile_error(message, file, {line, column}) do
+      reraise(%Surface.CompileError{file: file, line: line, column: column, description: message}, [])
+    end
+  else
+    # TODO: Remove this clause in Surface v0.13 and set required elixir to >= v1.14
+    def compile_error(message, file, {line, _column}) do
+      reraise(%Surface.CompileError{line: line, file: file, description: message}, [])
+    end
+  end
+
   def compile_error(message, file, line) do
-    reraise(%CompileError{line: line, file: file, description: message}, [])
+    reraise(%Surface.CompileError{line: line, file: file, description: message}, [])
+  end
+
+  if Version.match?(System.version(), ">= 1.14.0") do
+    def compile_error(message, hint, file, {line, column}) do
+      reraise(%Surface.CompileError{file: file, line: line, column: column, description: message, hint: hint}, [])
+    end
+  else
+    # TODO: Remove this clause in Surface v0.13 and set required elixir to >= v1.14
+    def compile_error(message, hint, file, {line, _column}) do
+      reraise(%Surface.CompileError{file: file, line: line, description: message, hint: hint}, [])
+    end
+  end
+
+  def compile_error(message, hint, file, line) do
+    reraise(%Surface.CompileError{line: line, file: file, description: message, hint: hint}, [])
   end
 
   @spec syntax_error(String.t(), String.t(), integer()) :: no_return()

test/mix/tasks/compile/surface/validate_components_test.exs

@@ -73,7 +73,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponentsTest do
                compiler_name: "Surface",
                details: nil,
                file: Path.expand("code"),
-               message: "Missing required property \"title\" for component <RequiredPropTitle>",
+               message: "missing required property \"title\" for component <RequiredPropTitle>",
                position: {0, 2},
                severity: :warning
              }
@@ -118,7 +118,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponentsTest do
                details: nil,
                file: Path.expand("code"),
                message: ~S"""
-               Missing required property "id" for component <LiveComponentHasRequiredIdProp>
+               missing required property "id" for component <LiveComponentHasRequiredIdProp>
 
                Hint: Components using `Surface.LiveComponent` automatically define a required `id` prop to make them stateful.
                If you meant to create a stateless component, you can switch to `use Surface.Component`.
@@ -190,7 +190,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponentsTest do
                compiler_name: "Surface",
                details: nil,
                file: Path.expand("code"),
-               message: "Missing required property \"title\" for component <#Macro>",
+               message: "missing required property \"title\" for component <#Macro>",
                position: {1, 4},
                severity: :warning
              }
@@ -220,7 +220,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponentsTest do
                  Path.expand(
                    "test/support/mix/tasks/compile/surface/validate_components_test/live_view_with_external_template.sface"
                  ),
-               message: "Missing required property \"value\" for component <ComponentCall>",
+               message: "missing required property \"value\" for component <ComponentCall>",
                position: {1, 2},
                severity: :warning
              }
@@ -263,7 +263,7 @@ defmodule Mix.Tasks.Compile.Surface.ValidateComponentsTest do
                compiler_name: "Surface",
                details: nil,
                file: Path.expand("code"),
-               message: "Missing required property \"list\" for component <Recursive>",
+               message: "missing required property \"list\" for component <Recursive>",
                position: {0, 2},
                severity: :warning
              }

test/support/case.ex

@@ -0,0 +1,13 @@
+defmodule Surface.Case do
+  @moduledoc """
+  This module defines a generic test case importing common funcions for tests.
+  """
+
+  use ExUnit.CaseTemplate
+
+  using do
+    quote do
+      import ANSIHelpers
+    end
+  end
+end

test/support/conn_case.ex

@@ -15,6 +15,7 @@ defmodule Surface.ConnCase do
       use Surface.LiveViewTest
       import Floki
       import FlokiHelpers
+      import ANSIHelpers
 
       # The default endpoint for testing
       @endpoint Endpoint