Add dialyzer to project, CI, and clean up type-spec issues#535
Merged
Add dialyzer to project, CI, and clean up type-spec issues#535
Conversation
- clean up issues
- updated CI for running and caching
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The project had no static type analysis. Many
@specs had drifted from the implementations they describe, hiding real-but-silent issues: success-typing mismatches in chat-model response handling, schemas with non-nullable types whose default-nilstructs couldn't satisfy them, anEcto.Changeset\\default-arg pattern that broke its own contract, and stream-decoder return shapes that didn't match what the implementations actually returned. Without dialyzer running in CI, these would keep accumulating.Solution
Added
:dialyxir ~> 1.4to the project, fixed every reachable warning, suppressed the unavoidable ones in a documented ignore file, and wired a cached dialyzer step into the GitHub Actions workflow so future drift surfaces on PRs.The bulk of the fixes fall into a few patterns:
do_process_response/2claimed it returned{:error, String.t()}but actually returned{:error, %LangChainError{}}, and missingTokenUsage.t()and tagged-error returns from list paths. Each provider'sdecode_stream/2claimed a one-tuple return but actually returned{[map()], String.t()}. Both specs were corrected acrosschat_open_ai,chat_open_ai_responses,chat_orq,chat_mistral_ai, andchat_perplexity. Once the truthful return types were in place, dialyzer's cascading "this clause can never match" warnings on the case-arms indo_api_request/4resolved themselves.do_api_request/4retry_countdefault. Main recently changedretry_count \\ 3toretry_count \\ nilacross all chat-model and image providers, but the@specstill declaredinteger()(ornon_neg_integer()). Updated each spec tointeger() | nilso the auto-generateddo_api_request/3arity satisfies the contract.DeepResearch.@type t()declared fields likeid: String.t()but the structs default tonil, so the auto-generatedchangeset/1(fromresult \\ %__MODULE__{}) couldn't satisfy its own contract. Relaxed the field types toString.t() | nilto match the actual struct shape, and added an explicit@spec changeset(map())for the auto-generated/1arity.Utils.handle_stream_fn,Utils.fire_streamed_callback,TokenUsage.set_wrapped, andTokenUsage.getall had specs narrower than what their callers actually pass (struct vs structural-map mismatches). Broadened to accept the values that real call sites use.Function.executelost the:interruptvariant.normalize_execution_resultandexecute_with_error_handlingspecs omitted{:interrupt, msg, data}even though both functions handle and return it. Added it, which un-stuck the case-clause analysis inLLMChain.execute_tool_call.LangChainErroralias inlib/chains/chain_callbacks.ex, which was usingLangChainError.t/0in two@typedeclarations without aliasing the module — causedunknown_typewarnings.chat_deepseek(map_size(metadata) > 0was always true after the implementation always added:logprobs) anddeep_research(defensive fallback clauses preceding clauses already covered them)..dialyzer_ignore.exswith explanatory comments:ReqLLM's upstream spec forgenerate_text/3andstream_text/3declaresString.t() | list()for the messages argument, but the actual API accepts%ReqLLM.Context{}(which is what we pass).chat_anthropic's streamingdo_api_request/4has defensive{:ok, {:error, _}}andother ->clauses that are statically unreachable but load-bearing for tests using Mimic mocks that inject arbitrary shapes.MessageDelta.to_message's defensive{:error, _reason}catchall after{:error, %Ecto.Changeset{}}.Message.newcurrently only returns the changeset error, but the catchall future-proofs against spec changes.lib/web_socket.exhas eightpattern_matchwarnings against Mint.WebSocket return-shape mismatches (3-tuple{:error, conn, exception}vs 2-tuple{:error, reason}). These are pre-existing on main and look like real bugs in the web_socket module — flagged for a follow-up PR rather than fixed here, since this PR's scope is the dialyzer setup itself.Related to #396 - Previous PR for adding Dialyzer
Changes
mix.exs— Added:dialyxir, configureddialyzer:withignore_warnings,plt_file, andplt_core_pathto land both PLTs inpriv/plts/for predictable cache contents..github/workflows/elixir.yml— Added anid: beamto the existing pinnedsetup-beamstep, then a four-step dialyzer flow gated onmatrix.lint: restore PLT cache → build PLTs on miss → save cache as a separate step (so a dialyzer failure doesn't discard a freshly-built PLT) → run with--format github --format dialyxirfor both inline PR annotations and readable log output. Newactions/cache/restore@…andactions/cache/save@…SHAs are pinned to match the existing zizmor-hardened pattern..dialyzer_ignore.exs(new) — Documents the unavoidable warning families above..gitignore— Excludes/priv/plts/.lib/chat_models/chat_open_ai.ex,chat_open_ai_responses.ex,chat_orq.ex,chat_mistral_ai.ex,chat_perplexity.ex— Correcteddo_process_response/2anddecode_stream/2specs.lib/chat_models/chat_open_ai.ex,chat_open_ai_responses.ex,chat_orq.ex,chat_mistral_ai.ex,chat_perplexity.ex,chat_anthropic.ex,chat_ollama_ai.ex,chat_deepseek.ex,lib/images/open_ai_image.ex,lib/images/modelslab_image.ex— Updateddo_api_requestspecs to allownilforretry_count.lib/chat_models/chat_google_ai.ex,chat_vertex_ai.ex—get_message_contents/1spec now admitsnil(matches thecontent: nilclause).lib/chat_models/chat_anthropic.ex— Tightenedfor_api/1spec toToolCall.t() | ToolResult.t()(commented-out clauses were misleading dialyzer).lib/chat_models/chat_deepseek.ex— Removed always-falseelsebranch inmerge_response_metadata/2.lib/chat_models/chat_perplexity.ex— Broadeneddo_api_request/4return spec to include{:ok, Req.Response.t()}from the streaming branch.lib/chains/llm_chain.ex— Removed redundantif chain.verbose, do: IO.inspect(...)guards inside theverbose: trueclause head (the head pattern already filtered).lib/chains/chain_callbacks.ex— Addedalias LangChain.LangChainErrorso@typedeclarations resolve.lib/function.ex— Added:interruptvariant tonormalize_execution_result/2andexecute_with_error_handling/4return specs.lib/utils.ex— Broadenedhandle_stream_fn/3first arg tomap()andfire_streamed_callback/2tomap()so chat-model structs match.lib/token_usage.ex—set_wrapped/2spec now reflects the catchall fallback clause;get/1widened toany()to match itsdef get(_)fallthrough.lib/tools/deep_research/research_request.ex,research_result.ex,research_status.ex— Field types relaxed toT | nil; added@spec changeset(map())for the auto-generated/1arity.lib/tools/deep_research.ex— Removed two genuinely unreachableformat_research_result/1andformat_sources/1fallback clauses.Testing
Full test suite passes locally (
mix test— 27 doctests + 1724 tests, 0 failures, 141 excluded live-API tests). Dialyzer runs clean:Total errors: 15, Skipped: 15, Unnecessary Skips: 0, done (passed successfully). The PLT-cache flow was verified locally by deletingpriv/plts/and rerunning to confirm both core and project PLTs land in the cached directory.No new tests added — the changes are spec corrections and dead-code removals, both verified by the existing test suite. One round of test failures during development caught me wrongly deleting two
chat_anthropic.exclauses that looked unreachable to dialyzer but are exercised by Mimic-mocked Req responses; those clauses were restored and added to.dialyzer_ignore.exsinstead.