Skip to content

Commit daab005

Browse files
lukaszsamsonclaude
andcommitted
Route no_return mismatches to review and label union domains
Three review findings: - A no_return()/none() spec against a non-empty inferred return was auto-passed as :inference_wider, hiding potentially wrong specs. It is not a mechanical contradiction either (inference over-approximates, so a non-empty return does not prove returnability) -- route it to :mixed for human judgment. Six stdlib functions surface, all correct no_return() specs over BIF delegations, acknowledged with comments. - The spec-domain body check unions argument positions across spec clauses, admitting cartesian combinations no single clause allows. Findings on multi-clause-spec functions are now tagged "[union domain]" in the report and :per_position_union in the JSON; single-clause specs are exact. Documented in the header. - Literal map types are translated as CLOSED maps; documented why this is the exact Erlang typespec semantics (a map value must have every association matched by the type; extra keys require an explicit optional(any()) => any(), which is handled by the open_rest branch) and not an under-approximation. Also adds visibility for coverage limits: the JSON now lists the 30 spec'd functions without inferred signatures (no comparison possible) and counts entries whose translation was inexact (825), so silent auto-triage under approximation is quantified. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent b6bb5fe commit daab005

2 files changed

Lines changed: 87 additions & 15 deletions

File tree

lib/elixir/scripts/compare_specs_and_signatures.exs

Lines changed: 81 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,13 @@ defmodule SpecToDescr do
300300
defp builtin(:map, [], _e, _d), do: {empty_map(), true}
301301
defp builtin(:map, :any, _e, _d), do: {open_map(), true}
302302

303+
# Literal map types are CLOSED per Erlang typespec semantics: a map value
304+
# belongs to a map type only if every association in the VALUE is matched
305+
# by an association in the TYPE, so %{required(:a) => integer()} does NOT
306+
# admit extra keys (that requires an explicit optional(any()) => any(),
307+
# which is why map() itself is defined as %{optional(any()) => any()} and
308+
# handled by the :open_rest branch below). closed_map/1 is therefore the
309+
# exact translation, not an under-approximation.
303310
defp builtin(:map, assocs, env, d) do
304311
{fields, exact} =
305312
Enum.map_reduce(assocs, true, fn
@@ -516,6 +523,15 @@ defmodule Verdict do
516523
not empty?(sret_u) and not empty?(effective) and disjoint?(sret_u, effective) ->
517524
:contradiction
518525

526+
# A no_return()/none() spec against a non-empty inferred return
527+
# cannot be auto-passed as :inference_wider: the spec claims the
528+
# function never returns while inference says it may. It is not a
529+
# mechanical :contradiction either -- inference over-approximates,
530+
# so a non-empty return does not PROVE returnability. Route to
531+
# human judgment.
532+
empty?(sret_u) and not empty?(effective) ->
533+
:mixed
534+
519535
subtype?(sret_u, effective) and subtype?(effective, sret_u) ->
520536
:equivalent
521537

@@ -717,6 +733,13 @@ defmodule Main do
717733
# each spec'd function's argument domain taken from its translated spec
718734
# instead of the default list of dynamics (Module.Types.warnings/7).
719735
# Informational: results are reported but never gate.
736+
#
737+
# APPROXIMATION: for multi-clause specs the domain is the PER-POSITION
738+
# union of the clause argument types, which admits cartesian combinations
739+
# no single spec clause allows (f(atom, atom) | f(int, int) yields
740+
# {atom | int, atom | int}). Findings on such functions are tagged
741+
# "[union domain]" in the report and :per_position_union in the JSON;
742+
# single-clause specs are exact.
720743

721744
defp check_bodies(module, entries, cache, opts) do
722745
with true <- opts[:check_bodies],
@@ -739,6 +762,11 @@ defmodule Main do
739762
{{e.function, e.arity}, {mode, args}}
740763
end
741764

765+
union_domains =
766+
for %{slices: slices} = e <- entries, length(slices) > 1, into: MapSet.new() do
767+
{e.function, e.arity}
768+
end
769+
742770
{warnings, sigs} =
743771
Module.Types.warnings(module, file, attrs, defs, :all, cache, fn fun_arity ->
744772
case domains do
@@ -754,13 +782,21 @@ defmodule Main do
754782
with true <- is_map_key(domains, fun_arity),
755783
{_kind, {:infer, _dom, [_ | _] = clauses}, _mapping} <- Map.get(sigs, fun_arity) do
756784
ret = clauses |> Enum.map(fn {_args, ret} -> ret end) |> Enum.reduce(&opt_union/2)
757-
Map.put(e, :body_check, %{return: ret, relation: body_relation(ret, e)})
785+
786+
domain =
787+
if MapSet.member?(union_domains, fun_arity), do: :per_position_union, else: :spec
788+
789+
Map.put(e, :body_check, %{
790+
return: ret,
791+
relation: body_relation(ret, e),
792+
domain: domain
793+
})
758794
else
759795
_ -> e
760796
end
761797
end)
762798

763-
{entries, Enum.map(warnings, &format_body_warning/1)}
799+
{entries, Enum.map(warnings, &format_body_warning(&1, union_domains))}
764800
else
765801
_ -> {entries, []}
766802
end
@@ -794,7 +830,7 @@ defmodule Main do
794830

795831
# Warning entries are {module, warning, {file, meta, {mod, fun, arity}}}
796832
# as stored by Module.Types.Helpers.
797-
defp format_body_warning({warning_module, warning, location}) do
833+
defp format_body_warning({warning_module, warning, location}, union_domains) do
798834
message =
799835
try do
800836
%{message: message} = warning_module.format_diagnostic(warning)
@@ -803,19 +839,24 @@ defmodule Main do
803839
_ -> inspect(warning, limit: 5)
804840
end
805841

806-
{mfa, position} =
842+
{mfa, position, fun_arity} =
807843
case location do
808844
{file, meta, {mod, fun, arity}} ->
809845
line = if is_list(meta), do: Keyword.get(meta, :line), else: nil
810846

811847
{"#{inspect(mod)}.#{fun}/#{arity}",
812-
"#{Path.relative_to_cwd(to_string(file))}:#{line || "?"}"}
848+
"#{Path.relative_to_cwd(to_string(file))}:#{line || "?"}", {fun, arity}}
813849

814850
_ ->
815-
{"?", "?"}
851+
{"?", "?", nil}
816852
end
817853

818-
%{mfa: mfa, message: message, location: position}
854+
domain =
855+
if fun_arity && MapSet.member?(union_domains, fun_arity),
856+
do: :per_position_union,
857+
else: :spec
858+
859+
%{mfa: mfa, message: message, location: position, domain: domain}
819860
end
820861

821862
defp analyze_function(module, name, arity, spec_clauses, cache, env) do
@@ -899,6 +940,12 @@ defmodule Main do
899940
}
900941

901942
hints = Enum.filter(all, &(&1.verdict == :spec_wider_return))
943+
no_signature = Enum.filter(all, &(&1.verdict == :no_signature))
944+
945+
inexact =
946+
Enum.count(all, fn e ->
947+
Enum.any?(e[:slices] || [], &(not (&1.exact_args and &1.exact_ret)))
948+
end)
902949

903950
body_warnings =
904951
results
@@ -908,11 +955,19 @@ defmodule Main do
908955
out =
909956
case opts[:format] do
910957
"report" ->
911-
render_report(stats, residue, untranslatable, all, gate, body_warnings)
958+
render_report(stats, residue, untranslatable, all, gate, body_warnings, inexact)
912959

913960
"json" ->
914961
JSON.encode_to_iodata!(
915-
render_json(stats, residue, untranslatable, hints, body_warnings)
962+
render_json(
963+
stats,
964+
residue,
965+
untranslatable,
966+
hints,
967+
body_warnings,
968+
no_signature,
969+
inexact
970+
)
916971
)
917972

918973
"prompt" ->
@@ -967,7 +1022,7 @@ defmodule Main do
9671022
body_check:
9681023
case e[:body_check] do
9691024
nil -> nil
970-
bc -> %{return: to_quoted_string(bc.return), relation: bc.relation}
1025+
bc -> %{return: to_quoted_string(bc.return), relation: bc.relation, domain: bc.domain}
9711026
end
9721027
}
9731028
end
@@ -980,19 +1035,25 @@ defmodule Main do
9801035
# inference proved the return strictly narrower than an exactly-translated
9811036
# spec. Not gated (specs are deliberately wide contracts), but listed so
9821037
# documentation-tightening candidates remain identifiable.
983-
defp render_json(stats, residue, untranslatable, hints, body_warnings) do
1038+
defp render_json(stats, residue, untranslatable, hints, body_warnings, no_signature, inexact) do
9841039
%{
9851040
elixir: System.version(),
9861041
format_version: 3,
9871042
stats: stats,
9881043
residue: Enum.map(residue, &entry_json/1),
9891044
untranslatable: Enum.map(untranslatable, &untranslatable_json/1),
9901045
tightening_hints: Enum.map(hints, &entry_json/1),
991-
body_warnings: body_warnings
1046+
body_warnings: body_warnings,
1047+
# Functions with a spec but no inferred signature: nothing to compare,
1048+
# so they receive no coverage from this tool -- listed for visibility.
1049+
no_signature: Enum.map(no_signature, &mfa_string/1),
1050+
# How many compared functions involved an inexact (over-approximated)
1051+
# spec translation somewhere; per-slice precision flags carry details.
1052+
approximate_translations: inexact
9921053
}
9931054
end
9941055

995-
defp render_report(stats, residue, untranslatable, all, gate, body_warnings) do
1056+
defp render_report(stats, residue, untranslatable, all, gate, body_warnings, inexact) do
9961057
total = length(all)
9971058
auto = Enum.count(all, &(&1.verdict in @auto))
9981059

@@ -1035,6 +1096,7 @@ defmodule Main do
10351096
auto-triaged: #{auto} (#{percent(auto, total)}) -- #{inspect(Map.take(stats, @auto))}
10361097
residue: #{length(residue)} -- #{inspect(Map.take(stats, @residue))}
10371098
untranslatable: #{Map.get(stats, :untranslatable, 0)}
1099+
approximate translations: #{inexact} (per-slice precision flags in JSON)
10381100
#{gate_section}
10391101
"""
10401102

@@ -1090,7 +1152,8 @@ defmodule Main do
10901152
lines =
10911153
Enum.map_join(warnings, "\n", fn w ->
10921154
message = String.replace(w.message, "\n", "\n ")
1093-
" #{w.mfa} (#{w.location}): #{message}"
1155+
tag = if w.domain == :per_position_union, do: " [union domain]", else: ""
1156+
" #{w.mfa} (#{w.location})#{tag}: #{message}"
10941157
end)
10951158

10961159
[
@@ -1121,7 +1184,10 @@ defmodule Main do
11211184
defp percent(n, total), do: "#{Float.round(n * 100 / total, 1)}%"
11221185

11231186
defp render_prompt(stats, residue, untranslatable, body_warnings) do
1124-
json = JSON.encode_to_iodata!(render_json(stats, residue, untranslatable, [], body_warnings))
1187+
json =
1188+
JSON.encode_to_iodata!(
1189+
render_json(stats, residue, untranslatable, [], body_warnings, [], 0)
1190+
)
11251191

11261192
[
11271193
"""

lib/elixir/scripts/compare_specs_exclusions.txt

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ File.rm_rf/1 # mixed
4848
IEx.Pry.annotate_quoted/3 # mixed
4949
IEx.Pry.whereami/3 # mixed
5050
Inspect.Algebra.container_doc_with_opts/6 # mixed
51+
Kernel.exit/1 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
52+
Kernel.throw/1 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
5153
Keyword.put/3 # mixed
5254
Keyword.replace!/3 # mixed
5355
Keyword.update!/3 # mixed
@@ -65,7 +67,11 @@ Macro.unique_var/2 # mixed
6567
Mix.Project.get!/0 # mixed
6668
Mix.Release.from_config!/3 # mixed
6769
Mix.Tasks.Format.formatter_for_file/2 # mixed
70+
Mix.raise/1 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
6871
NaiveDateTime.from_iso8601/2 # mixed
72+
Process.hibernate/3 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
73+
System.halt/0 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
74+
System.halt/1 # mixed: no_return() spec is correct; inference over-approximates the BIF delegation
6975
Task.async/1 # mixed
7076
Task.async/3 # mixed
7177
Time.convert/2 # mixed

0 commit comments

Comments
 (0)