Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dialyxir to CI #1248

Merged
merged 8 commits into from
Jun 19, 2023
Merged

Add dialyxir to CI #1248

merged 8 commits into from
Jun 19, 2023

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

This:

  • adds dialyxir to CI (cached PLT)
  • tweaks some code to get rid of compilation warnings
  • solves all existing Dialyzer -issued warnings

I may add comments to my own changes to clarify intention or source, if applicable (though the individual commits should tell part of the story already - they will have comments).

Comment on lines +4 to +6
{"lib/absinthe/middleware/async.ex", :unknown_function, 117},
{"lib/absinthe/middleware/batch.ex", :unknown_function, 213},
{"lib/absinthe/utils/render.ex", :improper_list_constr, 51},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme know if you prefer a broader "no-line" entry.

Comment on lines +11 to +12
@stopped_at_token_limit ":stopped_at_token_limit"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because nimble_parsec specific errors as String.t().

Another solution is to pull request there so the error reason can be term(), for example.

@@ -30,7 +30,8 @@ defmodule Absinthe.Mixfile do
],
deps: deps(),
dialyzer: [
plt_core_path: "priv/plts",
plt_add_deps: :apps_direct,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For lolspeed 😄

All changes to mix.lock are due to mix deps.update ex_doc
Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, Thanks!

@benwilson512 benwilson512 merged commit fab6ac9 into absinthe-graphql:master Jun 19, 2023
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/dialyxir-in-ci branch June 19, 2023 16:34
@benwilson512
Copy link
Contributor

@paulo-ferraz-oliveira I ended up removing dialyxer from the CI in #1253 since it immediately had issues w/ PLT caches that I didn't understand. You're welcome to reintroduce it but some guidance about how to manage the caches when adding Elixir / OTP versions would be appreciated.

@paulo-ferraz-oliveira
Copy link
Contributor Author

Caches usually should depend on whatever would invalidate them (in terms of key naming), as in

key: "${{ runner.os }}-erlang-${{ matrix.otp }}-elixir-${{ matrix.elixir }}-${{ hashFiles('mix.lock') }}"

(OS + OTP version + Elixir version + mix.lock hash)

What problems did you run into?

As for guidance, I think the above should be enough, but maybe you want something different (?)

@benwilson512
Copy link
Contributor

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 30, 2023

Edit: it's not cache -related, but Elixir 1.15 -related.

Oh, from the top of my head this might be because we recently released a new setup-beam version that changed where stuff was kept (1.16), so the cached elements fail because the cache is no longer found in the same place (OS-wise) and the PLT is static in references.

Would you be willing to delete the caches? I'd pull request again to re-introduce dialyxir in CI and you shouldn't see that problem (well, at least not until setup-beam breaks again 😢).

Check erlef/setup-beam#214 for some more history.

I just opened an issue for setup-beam (to discuss with maintainers) where this would potentially help avoid this class of issues.

@benwilson512
Copy link
Contributor

Actually so if you look at this job here https://github.com/absinthe-graphql/absinthe/actions/runs/5423460943/jobs/9861420841 I added a cache key prefix and if you look at the cache action you can see it didn't find any PLT cache, but it still failed.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 30, 2023

Yeah, I see that. Strange that it's complaining about a missing function: https://github.com/absinthe-graphql/absinthe/actions/runs/5423460943/jobs/9861420841#step:10:18.

The function is valid, though, as per https://www.erlang.org/doc/man/prettypr.html#text-1, but maybe it's not added to the analysis scope for dialyxir?

Lemme try to look at this closer. I need to fork, pull your branch, and make it fail before I can figure out what needs fixing :)

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 30, 2023

Yeah, so I also got it failing here: https://github.com/paulo-ferraz-oliveira/absinthe/actions/runs/5425961530/jobs/9867389890

Now on to figure stuff out 😄

Edit: will also be sure to use Erlang 25.3.2.3 and Elixir 1.15.1, locally, to make sure no issue is apparent from that distinction.

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 30, 2023

Not breaking using Elixir 1.14.5, but breaking using 1.15.1. IIrc, this is an intentional change for 1.15.x (lemme look that up).

Edit: also potentially related > jeremyjh/dialyxir#478

@paulo-ferraz-oliveira
Copy link
Contributor Author

@benwilson512, I "fixed" it by doing:

   def application do
    [extra_applications: [:crypto, :logger | extra_extra_applications(Mix.env())]]
   end
 
  def extra_extra_applications(:test), do: [:syntax_tools]
  def extra_extra_applications(_), do: []

but this seems kinda hackish. I don't know what is including prettypr in this case, since in other Elixir lib.s I don't see the same issue.

@paulo-ferraz-oliveira
Copy link
Contributor Author

A slightly less hackish (at least consistent with 1.15 release notes change) would be:

  def project do
    [
      ...
      prune_code_paths: prune_code_paths(Mix.env())
    ]
  end

  defp prune_code_paths(:test), do: false
  defp prune_code_paths(_), do: true

@paulo-ferraz-oliveira
Copy link
Contributor Author

As per https://github.com/paulo-ferraz-oliveira/absinthe/actions/runs/5426289985/jobs/9868142420, from commit https://github.com/paulo-ferraz-oliveira/absinthe/commit/b6c39deef09d93103e84f4de4bb4b7b9cf6235ce, you can see that CI passes (your branch without further changes). If you find the test-specific option prune_code_paths set to false to have dialyxir pass (it's not only in CI, but locally too) I can add it. Otherwise we can wait for a dialyxir update that fixes it (?)

I'll open a pull request for that, and we can continue there.

@benwilson512
Copy link
Contributor

Thanks for all of the work following up @paulo-ferraz-oliveira!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants