Repository navigation
fix(types): narrow the lists:* widening across the tree - #555
Merged
Merged
Conversation
file:consult/1 answers [term()] and lists:keyfind/3 wants [tuple()], so a chain of keyfind reads over a config file is untypeable. A tuple pattern in a comprehension generator narrows where keyfind cannot. First of the lists:* tranche (asobi#435); see docs/eqwalizer-idioms.md.
Eight errors, four distinct idioms, all now in docs/eqwalizer-idioms.md:
- `maybe ... end` cannot be typed without an explicit `else`. Verified with a
probe - the same block without one fails, with one is clean. Behaviour is
identical here: every `?=` answers {ok, _} | {error, problem()}, so that is
the only shape which can reach the clause.
- `lists:usort/1`, `append/1` and `reverse/1` widen to [term()]; re-narrowed
rather than replaced.
- Two `lists:foldr/3` accumulators erased their type. Explicit recursion, with
the order lists:foldr/3 gave.
- `lists:keyfind/3` over the literal MIME table cannot see a [tuple()]; a
comprehension narrows the list and the value together.
`persistent_term:get/2` is a boundary, so the cache read takes dynamic() and
hands back the type resolve/0 put there, rather than re-deriving bundle()'s
shape at the read.
Two `?LOG_` calls in tail position needed the `ok` their spec promises.
asobi_console_tests 26/0.
- asobi_extension_reserved_tests: application:get_key/2 answers {ok, term()},
so every module and provider list read out of it widened. One accessor.
- asobi_wire_tests: frame/1 had no spec, so hd/1 and lists:nth/2 over a list
of frames gave term(). Bound each frame by name instead, which also says
which is which. lists:duplicate/2 widens to [term()], and list_to_atom/1
wants a string().
- asobi_social_api_SUITE: CT config is a [tuple()] of term() values, so all
twenty-four lists:keyfind/3 reads widened. One typed accessor rather than
leaving two idioms in the file.
- asobi_console: see previous commit.
Tree 317 -> 275. asobi_wire_tests + asobi_extension_reserved_tests 38/0,
asobi_social_api_SUITE 13/0.
Same idioms, seven more modules:
- asobi_test_helpers gains a shared binary_join/2. The
iolist_to_binary(lists:join(...)) shape had appeared five times by now, which
is where a shared helper starts paying rather than being premature.
- lists:sort/2's comparator arguments are erased too, not just its result, so
asobi_ops_page narrows inside the fun.
- application:get_key/2 answers {ok, term()}; narrowed once per module rather
than at each read.
- file:consult/1 answers [term()], so the term has to be narrowed to a list
before it can be walked.
- lists:dropwhile/2 and lists:flatmap/2 both erase; a comprehension does the
same job with the type intact.
Tree 275 -> lower; every touched module reports NO ERRORS.
…es in src
- The two extension fixtures read persistent_term straight into a typed
context. It is a boundary, so it is narrowed at each read; their outcome
dispatch is also written out rather than passing an unmatched term through a
clause that promised ok | {error, _}.
- asobi_error: lists:sort/1 widened the extension code list, and
lists:keyfind/3 could not see ?CODES as [tuple()]. code() is binary(), which
is worth stating because the first guard I wrote for it was is_atom/1 - the
checker caught that, the tests would not have.
- asobi_ops_params: same keyfind shape over the allowed-sort table.
- asobi_ops_page: lists:sort/2 erases its comparator's arguments, not only its
result.
asobi_error_tests 24/0.
lists:all/2, tl/1, lists:foreach/2, nested lists:foldl/3 and lists:usort/1, plus two more keyfind-over-an-untyped-list reads. Same idioms throughout; see docs/eqwalizer-idioms.md. The nested foldl in asobi_dgram_pose_tests became a single comprehension with a generator per level, which is both typed and shorter than the two funs it replaced. luerl:decode/2 takes and answers Lua values, so asobi_lua_bot_tests narrows it through one boundary function rather than at three call sites.
…exposed
Finishes the `lists:*` pass. Every remaining site is the same handful of
idioms in docs/eqwalizer-idioms.md.
Two findings that were not type errors:
**A narrowing broke a deliberate test fixture.** `asobi_fixture_erase` exists
to hand core values that are OUTSIDE the erase contract, so core's own
validation can be tested - `asobi_extension_erase_tests` configures `other`
and asserts core answers `{bad_return, other}`. Narrowing its return to
`ok | {error, _}` turned that into a case_clause inside the fixture. Its spec
now says `term()`, which is what it actually promises and why. This is exactly
the trap the idioms doc warns about, met in the wild, and only the test suite
caught it.
**Dialyzer found what eqwalizer could not.** Replacing `lists:foldr/3` in
`asobi_console:read_assets/2` left `read_asset/3`'s `{error, _}` clause
unreachable, because the recursion short-circuits instead of threading the
error through every remaining element. Removing it tightened the accumulator's
type, which then exposed that `mime/1` answers `binary() | error` while
`asset()` declares `mime := binary()` - and the controller puts that value
straight into a content-type header. `name/1` already refuses a name whose
extension has no mime, so the invariant holds; it is now restated where the
asset is built, because an atom in a content-type header would be a malformed
response rather than a crash.
Also widened `asobi_ws_handler:terminate/3`'s state to `term()`. Its second
clause exists so a shape it was never designed for cannot crash the process on
the way out, and a test asserts that; the spec described the first clause
rather than the function.
Tree 317 -> 215 across the tranche. eunit 2360/0, ct 390/0, dialyzer clean,
fmt, xref.
Refs #435
🟡 Code Coverage — 76.8%9272 of 12069 lines covered. 🟡 ELP Lint — 192 warnings212 diagnostics found. See job logs for details. ✅ ELP eqWAlizeNo diagnostics. |
…rtions Code review of #555 found two changes that stopped being mechanical, both reproduced with probes against the merge base. **asobi_console loaded a malformed bundle that main refused.** `declared/1` and `collect/2` feed `names/1`, whose entire job is to refuse a manifest value that is not a string, *by name*. Re-narrowing them for the type checker dropped the bad value before `names/1` could see it: a manifest with `"file": 12345` went from `{error, {bad_asset_name, ...}}` to loading with that chunk silently absent. A Vite upgrade emitting a non-string path would have shipped a console missing a screen with nothing saying so. Both are back to `[term()]`, which is what `names/1` already takes. Two tests pin it; they are red on the previous commit. This is the second time this cleanup has filtered ahead of a validator - the first was asobi_world_chat in tranche 1 - so it is now a rule in docs/eqwalizer-idioms.md rather than an anecdote: re-narrowing is only free when the producer already guarantees the type, and never in front of the function that reports the problem. **A pattern in a generator is a filter, not a match.** Rewriting a fold in asobi_dgram_pose_tests turned `{Records, 0} = ...` into `{Records, 0} <- [...]`, so a saturated tick was skipped rather than asserted on. asobi_dgram_pose treats saturation as something worth knowing before players report teleporting. The match is back, in a named helper the comprehension generates over, and `slot_of/1` restores the function_clause that `#{slot := S} <- Records` also swallowed. Also from the review: - `frames/1`'s second clause was dead *and* harmful: it wrapped a bare frame into a list, so a regression to a single frame - the #330 / nova#399 shape these tests exist to pin - would have passed `length/1`. Deleted. - `narrow_bundle/1` was an identity function laundering `dynamic()`. It matches the bundle's shape now, so a clobbered persistent_term key is a legible function_clause. - The `meck:history/1` match in asobi_ops_leaderboards_tests asserted exactly one query, which is what "grouped and capped" means. Restored. - `calls/0` in the two erase/export fixtures had the same trap `run/2` did: it is the observation side of a fixture built to catch a call core should not have made, so it no longer filters malformed records out of the evidence. - The unreachable `mime/1` branch raises rather than returning, so it does not advertise an outcome in `problem()` that cannot happen. Five amendments to docs/eqwalizer-idioms.md, which matter more than usual with 217 errors still to fix against it: the re-narrow precondition above, the generator-is-a-filter rule, `[X | _]` versus `[X] =` for keyfind semantics, that `lists:reverse/1` widens so accumulate-then-reverse needs a second re-narrow, and that `foldr/3` short-circuits from the right so replacing one changes which error is reported. eunit 2362/0, ct 390/0, dialyzer clean, fmt, eqwalize-all 217.
Both are solved rather than deferred, and neither needed a suppression.
**The rebar3 provider modules.** `-behaviour(provider)` was reported as a
behaviour that does not exist, because `provider` lives in rebar3 and rebar3 is
not a dep of asobi. The attribute was buying nothing: rebar3 dispatches a
provider on its exported `init/1`, `do/1` and `format_error/1`, and with the
behaviour module absent no callback checking happened either. Removed, with the
reason recorded at each site. rebar.config already excludes both modules from
dialyzer on the same grounds, and asobi_extension_reserved_tests already
asserts the three exports, so the contract is still pinned by something.
Their four real type errors are fixed too: an `--out` argument narrowed to a
path, the vite port narrowed to an integer, and `quote/1` no longer feeds
`unicode:characters_to_binary/1`'s error tuple to `json:encode/1` - a provider
argument that is not valid unicode now fails rather than being emitted into the
generated vite config.
**`asobi_extension_reserved:namespaces/0`'s `:=`.** eqwalizer approximated the
whole map to `#{dynamic() => dynamic()}` and said so, which is why this was
worth doing deliberately: fixing the key operator makes the count go up before
it comes down. It surfaced three unspec'd producers and four `lists:usort/1`
widenings behind them. Verified `namespaces/0` returns exactly the same set
before and after - 79 http, 31 rpc, 10 lua, 1 queue, 23 tables - because a
reserved-name regression here silently un-reserves a namespace.
Also clears `asobi_console_tests`, whose pre-existing `code:priv_dir/1` error
the gate inherited when this branch added tests to it.
eunit 2362/0, ct 390/0, fmt.
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.
Tranche 2 of #435, scoped by pattern rather than directory — the measurement in tranche 1 showed the work clusters by idiom, not by folder.
Tree: 317 → 217. Twenty-six modules, one repeated set of idioms.
Why this shape
After #554 I classified all 367 remaining errors by reading each one's source line rather than its sub-expression.
lists:*was by far the largest identifiable class. The two agent recommendations from #554 both turned out to be aimed at smaller targets:lists:*widening?=chains)maps:geton untyped mapspersistent_term:get→ boundary?LOG_returningterm()gen_server:callresultmin/maxThere are two
min/maxerrors left in the whole tree, so the shared-numeric-module question from #554 was about two call sites. Keeping them per-module.The idioms
All in
docs/eqwalizer-idioms.md, which this PR extends. Cited at the fix sites rather than restated.usort/1is O(n log n) in C;[X || X <- lists:usort(L), is_binary(X)]keeps it and restores the type.foldlrule.lists:keyfind/3cannot — that covers the 31 keyfind sites, mostly reads out of CT config,file:consult/1output andapplication:get_key/2.maybe ... endneeds an explicitelse. Probe-verified: the identical block fails without one, is clean with it.lists:sort/2erases its comparator's arguments, not only its result.One shared helper earned its place:
asobi_test_helpers:binary_join/2, after theiolist_to_binary(lists:join(...))shape appeared five times.Two findings that were not type errors
A narrowing broke a deliberate test fixture.
asobi_fixture_eraseexists to return values outside the erase contract so core's validation can be tested. Narrowing its return turned that into acase_clauseinside the fixture. Only the test suite caught it — the exact trap the idioms doc warns about, met in the wild.Dialyzer found what eqwalizer could not. Replacing a
foldrleft a clause unreachable; removing it tightened an accumulator, which exposed thatmime/1answersbinary() | errorwhileasset()declaresmime := binary()— and the controller puts that into a content-type header. Not live (name/1gates it), but the invariant is now restated where the asset is built.Also widened
asobi_ws_handler:terminate/3's state toterm(): its second clause exists so an unexpected shape cannot crash the process on the way out, and a test asserts that. The spec described the first clause rather than the function.Not in this PR
src/extensions/rebar3_asobi_console.erl— its last 2 sites sit alongside abehaviour_does_not_existfor-behaviour(provider), which is an analysis-config question (rebar3 is not in the eqwalizer app graph), not a type fix. Touching the module would oblige the gate to demand a decision that is yours.Checks
eunit 2360/0, ct 390/0, dialyzer clean, fmt, xref. Every touched module reports
NO ERRORS.