Add options for enabling/disabling Gradient type-checking for specific apps/files #124

japhib · 2022-08-25T21:59:59Z

My proposed solution for #123 . Let me know what you think of this approach.

Stuff left to do:

Add some automated tests for this functionality
Probably refactor some of the logic so it's not all stuffed into the mix task
Update README with info on how to use these configs

erszcz · 2022-09-18T19:58:03Z

Awesome! I'm happy to merge this if we can get a test or two for the logic in place. Would you have the time to add them?

japhib · 2022-09-19T16:01:25Z

Great - I can definitely add some tests for this.

japhib · 2022-10-04T19:46:56Z

Hi, just added some tests for this. They're a bit complicated; the existing testing looked like it only operated on individual files, but in my case I wanted to be able to edit the files in a project and then run the task on the entire project, which I ended up doing through the shell. I'm happy to walk through the changes if any of them are unclear.

I'm thinking about maybe refactoring some of the logic from the Mix task for filtering files into something like UmbrellaUtils; what do you think of that? If you're fine with how it looks currently, I won't bother with that.

The only other thing, which I'm now remembering, is that I wanted to add a note in the README for how to use this new logic. I'll add that in the next little bit.

erszcz · 2022-10-12T16:58:22Z

Thanks for the contribution!

I approved the CI run and we get some formatting errors there, but it's not a biggie.
However, I have some problems running Gradient on the example umbrella app - here are the steps I'm taking:

git checkout japhib/umbrella-config
git rebase main # this step actually doesn't make a difference, I tried both with and without it
mix clean
mix deps.get
mix compile
cd examples/simple_umbrella_app/
mix deps.get
mix gradient

The result is that I get this:

Generated app_b app
** (RuntimeError) Error resolving .ex filename from compiled .beam filename '/Users/erszcz/work/esl/gradient/examples/simple_umbrella_app/_build/dev/lib/app_a/ebin/Elixir.AppA.beam': {:ok, [[{:attribute, 1, :file, {'lib/app_a.ex', 1}}, {:attribute, 1, :module, AppA}, {:attribute, 1, :compile, [:no_auto_import]}, {:attribute, 1, :export, [__info__: 1, hello: 0]}, {:attribute, 1, :spec, {{:__info__, 1}, [{:type, 1, :fun, [{:type, 1, :product, [{:type, 1, :union, [{:atom, 1, :attributes}, {:atom, 1, :compile}, {:atom, 1, :functions}, {:atom, 1, :macros}, {:atom, 1, :md5}, {:atom, 1, :exports_md5}, {:atom, 1, :module}, {:atom, 1, :deprecated}]}]}, {:type, 1, :any, []}]}]}}, {:function, 0, :__info__, 1, [{:clause, 0, [{:atom, 0, :module}], [], [{:atom, 0, AppA}]}, {:clause, 0, [{:atom, 0, :functions}], [], [{:cons, 0, {:tuple, 0, [{:atom, 0, :hello}, {:integer, 0, 0}]}, {nil, 0}}]}, {:clause, 0, [{:atom, 0, :macros}], [], [nil: 0]}, {:clause, 0, [{:atom, 0, :exports_md5}], [], [{:bin, 0, [{:bin_element, 0, {:string, 0, [240, 105, 247, 119, 22, 50, 219, 207, 90, 95, 127, 92, 159, 46, 131, 169]}, :default, :default}]}]}, {:clause, 0, [{:match, 0, {:var, 0, :Key}, {:atom, 0, :attributes}}], [], [{:call, 0, {:remote, 0, {:atom, 0, :erlang}, {:atom, 0, :get_module_info}}, [{:atom, 0, AppA}, {:var, 0, :Key}]}]}, {:clause, 0, [{:match, 0, {:var, 0, :Key}, {:atom, 0, :compile}}], [], [{:call, 0, {:remote, 0, {:atom, 0, :erlang}, {:atom, 0, :get_module_info}}, [{:atom, 0, AppA}, {:var, 0, :Key}]}]}, {:clause, 0, [{:match, 0, {:var, 0, :Key}, {:atom, 0, :md5}}], [], [{:call, 0, {:remote, 0, {:atom, 0, :erlang}, {:atom, 0, :get_module_info}}, [{:atom, 0, AppA}, {:var, 0, :Key}]}]}, {:clause, 0, [{:atom, 0, :deprecated}], [], [nil: 0]}]}, {:function, 15, :hello, 0, [{:clause, 15, [], [], [{:atom, 15, :world}]}]}]]}
    (gradient 0.1.0) lib/mix/tasks/gradient.ex:363: Mix.Tasks.Gradient.ex_filename_from_beam/1
    (gradient 0.1.0) lib/mix/tasks/gradient.ex:336: anonymous fn/5 in Mix.Tasks.Gradient.filter_files_with_magic_comment/4
    (elixir 1.13.4) lib/enum.ex:4034: Enum.filter_list/2
    (gradient 0.1.0) lib/mix/tasks/gradient.ex:277: anonymous fn/1 in Mix.Tasks.Gradient.filter_enabled_paths/1
    (elixir 1.13.4) lib/enum.ex:1597: anonymous fn/3 in Enum.map/2
    (stdlib 3.16) maps.erl:410: :maps.fold_1/3
    (elixir 1.13.4) lib/enum.ex:2408: Enum.map/2
    (gradient 0.1.0) lib/mix/tasks/gradient.ex:276: Mix.Tasks.Gradient.filter_enabled_paths/1

This is quite easy to fix with the following patch - it's probably a regression from #121:

diff --git a/lib/mix/tasks/gradient.ex b/lib/mix/tasks/gradient.ex
index 7a68ec5..6434557 100644
--- a/lib/mix/tasks/gradient.ex
+++ b/lib/mix/tasks/gradient.ex
@@ -354,7 +354,7 @@ defmodule Mix.Tasks.Gradient do

   defp ex_filename_from_beam(beam_path) do
     case ElixirFileUtils.get_forms(beam_path) do
-      {:ok, [{:attribute, _, :file, {filename, _}} | _]} ->
+      {:ok, [[{:attribute, _, :file, {filename, _}} | _]]} ->
         # Convert the *.beam compiled filename to its corresponding *.ex source file
         # (it's a charlist initially so we pipe it through to_string)
         filename |> to_string()

However, this results in:

$ mix gradient
Loading deps...
Compiling project...
Typechecking files...
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
No errors found!

With another patch:

diff --git a/lib/mix/tasks/gradient.ex b/lib/mix/tasks/gradient.ex
index 7a68ec5..0e01f38 100644
--- a/lib/mix/tasks/gradient.ex
+++ b/lib/mix/tasks/gradient.ex
@@ -354,10 +354,10 @@ defmodule Mix.Tasks.Gradient do

   defp ex_filename_from_beam(beam_path) do
     case ElixirFileUtils.get_forms(beam_path) do
-      {:ok, [{:attribute, _, :file, {filename, _}} | _]} ->
+      {:ok, [[{:attribute, _, :file, {filename, _}} | _]]} ->
         # Convert the *.beam compiled filename to its corresponding *.ex source file
         # (it's a charlist initially so we pipe it through to_string)
-        filename |> to_string()
+        filename |> to_string() |> IO.inspect()

       error ->
         raise "Error resolving .ex filename from compiled .beam filename #{inspect(beam_path)}: #{inspect(error)}"

We get:

$ mix gradient
==> gradient
Compiling 1 file (.ex)
Loading deps...
Compiling project...
"lib/app_a.ex"
"lib/app_a_helper.ex"
"lib/app_b.ex"
"lib/app_b_helper.ex"
Typechecking files...
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
Cannot load tokens: {:error, :enoent}
No errors found!

But the printed paths don't seem to be valid - they're relative to the umbrella apps' dirs, not the umbrella top directory.

Am I missing the point and the intended use is not to call mix gradient from the top of the umbrella?

japhib · 2022-10-12T22:46:06Z

Thanks for all the info - and no, you're not missing the point, it's definitely intended to be run that way. I'll look into these errors. Sorry about that, I guess I should've tested better!

…ing/disabling Gradient checks Fix analyzing beam files instead of ex files Working on unit tests for changes Fix unit tests

japhib · 2022-11-01T20:09:23Z

Everything in this branch is now fixed -- should be good to go.

I just saw #138. Looks like there are a lot more changes in that branch, which I haven't looked through all the way, but it looks like you've also contributed to that branch @erszcz. What do you want to do here? Still want this branch to be merged, or should I close this one and we merge #138 instead?

japhib · 2022-11-01T20:10:57Z

lib/gradient.ex

@@ -133,7 +133,6 @@ defmodule Gradient do
  defp maybe_specify_forms(forms, opts) do
    unless opts[:no_specify] do
      forms
-      |> put_source_path(opts)


As mentioned in #138 this line is not necessary because before maybe_specify_forms/2 is called, we already call put_source_path.

japhib · 2022-11-01T20:11:46Z

lib/mix/tasks/clean_examples.ex

@@ -0,0 +1,14 @@
+defmodule Mix.Tasks.CleanExamples do


I had some test failures while running some of the tests that were fixed by removing the compiled test files so they could be re-compiled from scratch. I'm adding this mix task, along with an alias to call it before mix test, to automate the process so that others won't experience the same issues.

japhib · 2022-11-01T20:15:41Z

lib/mix/tasks/gradient.ex

@@ -66,14 +70,18 @@ defmodule Mix.Tasks.Gradient do

    options = Enum.reduce(options, [], &prepare_option/2)

+    # Compile the project before the analysis
+    maybe_compile_project(options)


I moved this earlier in the pipeline to avoid a specific issue - when you run mix clean before mix gradient inside examples/simple_umbrella_app, you would get an error about AppA.hello() being undefined.

When I dug in, the reason is that in Gradualizer, gradualizer_db:init/1 is called before AppA is compiled, so Gradualizer doesn't have any of the sources from this project loaded.

It turns out that gradualizer_db:init/1 is called when we call Application.ensure_all_started(:gradualizer), so the solution is simply to move the compilation step above, so it happens before we start Gradualizer.

Good catch, thanks!

japhib · 2022-11-01T20:16:51Z

test/mix/tasks/gradient_test.exs

+      File.cd!(dir)
+
+      # mix clean first
+      assert {_, 0} = System.cmd("mix", ["clean"])


Added a mix clean before any tests for simple_umbrella_app, in order to reproduce the bug I mentioned.

…ror when reading file fails.

japhib · 2022-11-10T20:35:04Z

Hi @erszcz , any chance you could take a look at this check failure?

https://github.com/esl/gradient/actions/runs/3440202375/jobs/5738398944

lib/gradient.ex:194: The type of the function :erlang.++(), called doesn't match the surrounding calling context.
Warning: It has the following type
(nonempty_list(T1), list(T2) -> nonempty_list(T1 | T2))
(nonempty_list(T1), nonempty_improper_list(T2, T3) -> nonempty_improper_list(T1 | T2, T3))
(nonempty_list(T1), T2 -> nonempty_improper_list(T1, T2))
([], T -> T)

Total errors: 1
Error: Process completed with exit code 1.

Looks like the error comes from this code:

# lib/gradient.ex:194
  defp maybe_prepend_app_path(app_path, path) do
    unless is_absolute_path(path) do
      String.to_charlist(app_path) ++ '/' ++ path # This is the line the error refers to
    else
      path
    end
  end

Guessing this is related to josefs/Gradualizer#440

Edit: Sorry, I realize it's kind of a big ask to help debug code which I added ... but I'm pretty sure this code should be fine. Plus, I'm having a hard time digging into the Gradualizer source code to see what the real issue is here. If it could print a more specific error message, e.g. just including what types it thinks this expression currently resolves to (in addition to what it currently prints), that would be helpful.

japhib · 2022-11-10T23:08:42Z

Thoughts on this one vs. #138 ?

luk-pau-es · 2022-11-14T07:31:21Z

Thoughts on this one vs. #138 ?

Hello @japhib
Since you took some time to answer @erszcz comments, I have tried to find cause of errors in your code (which I believe I have found), plus I have upgraded dependencies and branch with master to have the latest features.

If you could look into this PR #138 and ascertain if I followed your line of thought that would be great!

… bugs

japhib · 2022-11-15T20:56:00Z

Just left a comment on #138. I suspect that's the way to go. If you're fine with cherry-picking a few more changes (as mentioned in the other comment), I'll go ahead and close this branch.

japhib · 2022-11-15T23:53:12Z

I also opened josefs/Gradualizer#484 since I feel the error message for the mix gradient CI failure here has insufficient info for me to be able to debug it.

erszcz · 2022-11-23T16:54:14Z

Superseded by #138. One more time, great work!

japhib mentioned this pull request Sep 8, 2022

Options for enabling/disabling Gradient on specific files or umbrella apps #123

Closed

japhib force-pushed the umbrella-config branch 2 times, most recently from d83d549 to 158f605 Compare October 4, 2022 19:43

luk-pau-es mentioned this pull request Oct 31, 2022

Check apps in an umbrella project #138

Merged

japhib added 3 commits November 1, 2022 14:04

Add ability to configure individual apps in an umbrella app for enabl…

99c5697

…ing/disabling Gradient checks Fix analyzing beam files instead of ex files Working on unit tests for changes Fix unit tests

Clean compile before testing on umbrella app

39dde9e

Fix mix task when cleaned first

bd04a48

japhib force-pushed the umbrella-config branch from 1f9a007 to bd04a48 Compare November 1, 2022 20:04

japhib commented Nov 1, 2022

View reviewed changes

japhib and others added 6 commits November 4, 2022 15:55

Don't prepend app_path for absolute paths. Use File.read! to raise er…

da07e85

…ror when reading file fails.

Merge branch 'main' into umbrella-config

740ec3d

Mix format

2f13126

Fix tests for CI

84d50d8

Merge branch 'main' into umbrella-config

867a8ce

Improve error message in ElixirFileUtils.load_tokens

ac4c260

erszcz mentioned this pull request Nov 15, 2022

Invalid paths in umbrella warnings #143

Closed

japhib added a commit to japhib/gradient that referenced this pull request Nov 15, 2022

Add in changes from esl#124 - clean before testing and fix some minor…

406c7f6

… bugs

japhib mentioned this pull request Nov 15, 2022

Error message needs more info: The type of the function _____, called on line ___ doesn't match the surrounding calling context. josefs/Gradualizer#484

Closed

luk-pau-es pushed a commit that referenced this pull request Nov 23, 2022

Add in changes from #124 - clean before testing and fix some minor bugs

dba7c7a

erszcz closed this Nov 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options for enabling/disabling Gradient type-checking for specific apps/files #124

Add options for enabling/disabling Gradient type-checking for specific apps/files #124

japhib commented Aug 25, 2022

erszcz commented Sep 18, 2022

japhib commented Sep 19, 2022

japhib commented Oct 4, 2022

erszcz commented Oct 12, 2022

japhib commented Oct 12, 2022

japhib commented Nov 1, 2022

japhib Nov 1, 2022

japhib Nov 1, 2022

japhib Nov 1, 2022

erszcz Nov 7, 2022

japhib Nov 1, 2022

japhib commented Nov 10, 2022 •

edited

Loading

japhib commented Nov 10, 2022

luk-pau-es commented Nov 14, 2022

japhib commented Nov 15, 2022

japhib commented Nov 15, 2022

erszcz commented Nov 23, 2022

Add options for enabling/disabling Gradient type-checking for specific apps/files #124

Add options for enabling/disabling Gradient type-checking for specific apps/files #124

Conversation

japhib commented Aug 25, 2022

erszcz commented Sep 18, 2022

japhib commented Sep 19, 2022

japhib commented Oct 4, 2022

erszcz commented Oct 12, 2022

japhib commented Oct 12, 2022

japhib commented Nov 1, 2022

japhib Nov 1, 2022

Choose a reason for hiding this comment

japhib Nov 1, 2022

Choose a reason for hiding this comment

japhib Nov 1, 2022

Choose a reason for hiding this comment

erszcz Nov 7, 2022

Choose a reason for hiding this comment

japhib Nov 1, 2022

Choose a reason for hiding this comment

japhib commented Nov 10, 2022 • edited Loading

japhib commented Nov 10, 2022

luk-pau-es commented Nov 14, 2022

japhib commented Nov 15, 2022

japhib commented Nov 15, 2022

erszcz commented Nov 23, 2022

japhib commented Nov 10, 2022 •

edited

Loading