Repository navigation
fix(check): a class's @overload makes it callable; check generic instances by their base - #1362
mareklangiewicz wants to merge 1 commit into
Conversation
…stances by their base `call-non-callable` only accepted a named type with `---@operator call`, so every call through a class's `---@overload` (`Point(1)`, `log('hi')` on a `---@type Logger`) was reported, although inference and `param-count` already treat those overloads as call candidates. A generic instance (`Box<integer>`) fell to the catch-all and was never callable. Both now pass when the type (or the generic's base) declares an `@overload` or `@operator call`; a generic alias is checked on its instantiation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q6PokSjk9vX5WV4FM6Xqkd
There was a problem hiding this comment.
Code Review
Summary
The change extends is_callable to recognize ---@overload on named types and to handle LuaType::Generic instances. Overall the logic is reasonable, but there are a few correctness and consistency concerns.
Issues & Recommendations
1. Generic with unresolved base type returns true (false negative)
LuaType::Generic(generic) => {
let Some(def) = model.type_def_of(&generic.get_base_type_id()) else {
return true; // <-- suspicious
};Returning true here silently suppresses the diagnostic when the base type cannot be resolved. This is inconsistent with the Ref/Def branch, which returns false on a missing def (and relies on analyze_error to report unresolved types). If the intent is "don't double-report unresolved types," it should be documented and ideally consistent with the Ref/Def behavior. As written, a genuinely broken generic base will never produce CallNonCallable.
Recommendation: Return false (matching Ref/Def) or add a comment explaining why true is correct here.
2. Alias generic branch uses a different code path than the non-generic alias branch
if def.kind == TypeDefKind::Alias {
return !model.callable_functions_cached(ty).is_empty();
}For non-generic aliases, the code expands the alias and recurses into is_callable. For generic aliases, it instead queries callable_functions_cached. These two paths can disagree (e.g., an alias to a union, or an alias whose target is itself an alias). This asymmetry is a latent inconsistency.
Recommendation: Prefer expanding the alias target and recursing (as the Ref/Def branch does), or document why the generic case must use callable_functions_cached.
3. has_call_candidate ignores alias expansion for the Def path
The new helper checks def.call_overloads and operator_of("call") directly. For a Def that is itself an alias (handled earlier by the target expansion), this is fine, but the helper is also called from the Generic branch after only checking kind == Alias. If a generic's base def is a Def whose underlying type is an alias, the helper won't expand it. Consider whether has_call_candidate should also handle alias targets, or document the invariant that callers pre-expand aliases.
4. call_overloads semantics for class tables vs instances
The test comment says:
---@overloadon a class makes both the class table and its instances callable.
This is a semantic decision worth confirming. In LuaLS conventions, ---@overload on a class typically describes the constructor (callable on the class table), not necessarily instances. If instances should not be callable, the Logger test case (log('hi') where log is typed Logger) would be incorrect. Please verify this matches the intended type-system semantics and that call_overloads is populated for both the class table and instance defs.
5. is_some_and requires a recent Rust version
.is_some_and(|facts| ...)Option::is_some_and was stabilized in Rust 1.70. Confirm the workspace MSRV supports it (the previous code used let Some(...) = ... else { return false }, which is MSRV-agnostic). Not a bug, just a compatibility note.
6. Minor: doc comment accuracy
The module doc now says generic instances pass "by their base type," but the alias sub-case actually uses callable_functions_cached on the instantiation, not the base type. Slight mismatch between doc and code.
Test Coverage Suggestions
- Add a case for a generic alias (e.g.,
---@alias Callable<T> fun(v: T)) to exercise theTypeDefKind::Aliasbranch insideGeneric. - Add a case where the generic base type is undefined, to pin down the intended behavior of issue #1.
- Add a negative case for
---@overloadon a non-callable instance if the semantics in #4 are clarified.
Verdict
Functionally the change is likely correct for the common cases, but issue #1 (returning true on unresolved generic base) and issue #2 (inconsistent alias handling) should be resolved or explicitly justified before merge.
Summary
call-non-callableaccepts a named type only when it has---@operator call, so a call through a class's---@overloadis reported, although inference andparam-countalready treat those overloads as call candidates:A generic instance (
Box<integer>) falls to the catch-all and is never callable.Evidence
mainde89376, new tests kept):test_no_call_non_callable_on_class_overload: FAILEDtest_no_call_non_callable_on_generic_class_instance: FAILEDcall_non_callabletests.cargo test --workspace --no-fail-fast: onlyemmylua_doc_clifailed, also on plainmain, and only with a--target-diroutside the checkout. With an in-tree target dir its 18 tests pass.call-non-callablereports gone, no report of any kind added. That run also carried a separate local fix for a panic inunify_call_bindings, which is not part of this PR.Merge Danger
Door: two-way
One checker file. Reverting it restores the old reports.
Blast Radius: diagnostics
call-non-callablereports fewer calls. A class whose---@overloadis not meant to make it callable no longer gets this warning.Written with help from Claude Code.