[hgschmie]

Track complementary (read) and (write) field declarations as getter and setter type contracts. This allows a setter to accept a broader type while reads retain the getter type; assignment checks, flow invalidation, hover, completion and navigation use the appropriate direction.

The Factorio API has a lot of definition where a field accepts multiple types for writing but consistently returns a single type for reading (e.g. https://lua-api.factorio.com/latest/classes/LuaEntity.html#last_user as an example).

---@class Burner
---@field (read) fuel? Fuel
---@field (write) fuel? string|Fuel

burner.fuel = "coal" is accepted by the setter contract, and a later read of burner.fuel has type Fuel|nil.

Includes documentation and regressions for optionality, generics, literal member keys, nested assignments, duplicate declarations and language-server features.

Validation on source base 783b007de4456233950cefa1c2e3fe099c693304: all 2,717 workspace tests passed, including 46 directional analyzer and 10 language-server test cases, together with workspace all-target/all-feature Clippy with warnings denied, formatting, diff checks and signatures. The single signed commit preserves the exact approved source tree; these results are reused for the history-only squash.

Review corrections cover receiver/root/ancestor and stable-alias guard invalidation, independent directional overloads, every viable union component's setter contract, annotation recovery, callable-getter assignment completion, nullable discriminant filtering and expression-local cast preservation. English and Chinese documentation describes the union assignment policy. Ordinary-field and completion controls accompany the directional cases.

The directional flow traversal is bounded but has not been performance-profiled. Inherited typed-instance reference-index omissions, ordinary exact-leaf/fresh unary-not flow limits, and the canonical getter-only hover description for a read/write pair remain documented limitations.

The rebase retains index-domain duplicate rules, index-key syntax metadata, separate index-signature resolution, doc-comment ownership, overload signatures and array/container separation. The shared nullable-discriminant correction intentionally also affects ordinary fields; both ordinary and directional regressions pass.

Uncached member lookup adds real unprofiled work. Inherited runtime-assignment shadowing of inherited/aliased declarations and the generic numeric-index semantic-model probe limitation are also retained; these are not claimed fixed.

[github-actions]

Code Review: Directional (read/write) Field Support

Overall this is a well-structured feature with good test coverage. Below are issues and suggestions, ordered roughly by severity.

Correctness / Logic

1. directional_read_at — budget == 0 returns the declared type, silently losing narrowing

if budget == 0 || !seen.insert(current) {
    return Some(read.typ.clone());
}

When the budget is exhausted or a cycle is detected, the function returns the un-narrowed declared type. This is safe but silently degrades. More importantly, seen is a HashSet shared across the whole traversal (cloned per branch), so a node visited on one branch will be treated as a cycle on another branch even when the paths differ. Consider keying seen on (flow_id, path) or at least documenting that this is a conservative over-approximation. Also, budget is a single counter across all branches — a wide CFG can exhaust it early and produce surprising results. Consider making the fallback explicit (e.g. return None to fall back to the normal path) rather than returning read.typ directly, so callers can distinguish "no narrowing" from "gave up".

2. directional_read_at returns Some(...) even when results is empty

If the loop exits without pushing anything (e.g. tree.get_flow_node(current)? returns None), LuaType::from_vec(vec![]) is returned. Verify from_vec on an empty vec produces a sensible type (likely Unknown/Never), otherwise this could mask errors.

3. directional_member — write member lookup ignores visibility/version

let writer = facts.members.iter().find(|other| {
    other.owner == field.owner
        && other.key == field.key
        && other.access == MemberAccess::Write
}).unwrap_or(field);

Unlike member_info_of, this does not check member_version_visible. If a write field is declared in a different version block, it may be picked up incorrectly. The same pattern appears in access.rs and member.rs — consider extracting a shared helper (e.g. find_directional_partner(facts, member, access)) to keep the three lookups consistent.

4. access.rs — member_id remapping only handles Write → Read

if member.access == crate::MemberAccess::Write
    && let Some(read) = facts.members.iter().find(...)
{
    return read.id.clone();
}

This silently rewrites a write member id to the read member id. Callers that legitimately need the write member (e.g. assignment checking) would get the wrong id. Since check_assign now short-circuits via directional_index, this may be fine, but it's fragile — a future caller of resolve_member expecting the write side will be surprised. Consider documenting this invariant or making it opt-in.

5. member_info_of — recursion without cycle guard

if member.access == crate::MemberAccess::Write
    && let Some(read) = ... {
    return member_info_of(model, &MemberRef { id: read.id.clone(), ... }, ...);
}

If a Read member somehow resolves back to a Write member (malformed data), this recurses infinitely. The access == Write guard prevents the direct case, but a defensive depth limit or assertion would be safer.

6. directional_member — nullable handling diverges from member_info_of

if writer.is_nullable && !typ.is_nullable() {
    typ = LuaType::from_vec(vec![typ, LuaType::Nil]);
}

This unconditionally adds nil, whereas member_info_of has a special case for Function/Ref/Def/etc. For a directional field whose write type is a Ref with ?, the two code paths will produce different results. Align the logic (or reuse member_info_of).

7. signature.rs — directional write replaces all overloads

let infos = if let Some((_, write)) = self.directional_member(&owner_ty, &key) {
    vec![write]
} else {
    member::member_infos_with_key_all(self, &owner_ty, &key)
};

If a directional write type is a function type with overloads, this collapses to a single MemberInfo. Confirm this is intended; otherwise merge with the existing infos.

8. facts.rs — duplicate (read)/(write) detection is order-dependent

if access != crate::MemberAccess::ReadWrite {
    self.annotation_errors.push(...);
    return;
}
access = direction;

This correctly rejects (read, write) and (write, read), but the error message says "only one of (read) or (write)" — fine. However, (read, read) is not rejected here; it falls through and is caught later by the duplicate-field checker. That's acceptable but worth a comment. Also note the early return skips the rest of field processing, so the field is dropped entirely — verify that's the desired behavior (vs. keeping it as ReadWrite).

9. duplicate_field.rs — the is_type_def + exactly-2 + read/write exemption

if is_type_def
    && dupes.len() == 2
    && matches!((dupes[0].access, dupes[1].access), (Read, Write) | (Write, Read))
{
    continue;
}

This relies on dupes being sorted/grouped such that the two directional entries are adjacent. If a third unrelated duplicate exists, dupes.len() == 2 fails and the pair is reported as a duplicate — which is arguably correct, but the error message will be confusing ("duplicate field" for a legitimate read/write pair plus an unrelated duplicate). Consider filtering the pair out before the length check.

10. hover/directional.rs — manual source slicing can panic

let before = &text[..u32::from(key.key_range.start()) as usize];

key_range.start() is a byte offset into the file; slicing a String at an arbitrary byte offset panics if it's not a char boundary (e.g. if the range is stale or the file has multi-byte chars before it). Use text.get(..offset) and handle None. Also, tree.get_red_root().text().to_string() clones the entire file just to extract a few comment lines — consider using the token/trivia API instead.

11. hover/directional.rs — doc-comment extraction heuristic is fragile

.skip(1)
.take_while(|line| line.trim_start().starts_with("--- "))

This assumes the doc comment is immediately above the field with no blank line, and only matches --- (not --- or --[[). It also skips the first line unconditionally (.skip(1)), which may drop a legitimate first doc line. Prefer reusing the existing doc-comment extraction used elsewhere in the codebase.

12. definition/mod.rs — directional definition bypasses normal resolution

if let Some((read, write, assignment)) = super::common::directional_index_at(&model, &token)
    && let Some(SemanticId::Member(key)) = if assignment { write.id } else { read.id }
{
    ...
    return Some(...);
}

This runs before label/module/string definition handling. If the cursor is on a string key that is also a module path (unlikely but possible), the directional path wins. More importantly, directional_index_at requires the token to be inside the index key range — verify that hovering the prefix (e.g. property in property.value) still falls through to normal definition. The test only covers the key position.

13. directional_index_at — assignment detection is too narrow

let assignment = index.get_parent::<LuaAssignStat>().is_some_and(|assign| {
    assign.get_var_and_expr_list().0.iter().any(|var| var.get_range() == index.get_range())
});

This only detects direct assignment targets. Compound assignments (+=), for loop variables, and function parameter bindings are not covered. Also, get_parent::<LuaAssignStat>() only finds a direct parent — if the index is wrapped (e.g. (property.value) = x), it won't match. Consider using get_parent_ancestors or a shared "is write target" helper.

14. assignment_target_type — only handles IndexExpr

pub fn assignment_target_type(&self, expr: emmylua_parser::LuaExpr) -> LuaType {
    if let emmylua_parser::LuaExpr::IndexExpr(index) = &expr
        && let Some((_, write)) = self.directional_index(index)
    {
        return write.typ;
    }
    self.type_of_expr(expr.get_syntax_id())
}

This is used in completion providers for assignment targets. For non-index targets (plain names), it falls back to type_of_expr, which is correct. But for index targets that are not directional, it also falls back — fine. However, the function name suggests it should always return the write type; document that it's a best-effort helper.

15. function_provider.rs — behavior change may regress completion

let var_type = builder.semantic_model.assignment_target_type(var.to_expr());
super::equality_provider::contains_function(builder, &var_type).then(|| vec![var_type])

Previously this always returned Some(vec![var_type]). Now it returns None when the target type contains no function. This is a behavioral change beyond the directional feature — verify it's intentional and doesn't break existing completion for non-function assignments. If intentional, it should be a separate commit/PR with its own tests.

Style / Maintainability

16. Fully-qualified crate::MemberAccess everywhere

The diff uses crate::MemberAccess::Read etc. in many places despite MemberAccess being re-exported. Import it (use crate::MemberAccess;) for readability. Same for crate::MemberAccess::ReadWrite in facts.rs.

17. directional.rs — directional_declaration has redundant clone

let def = facts.type_def_by_id(&member.owner)?;
self.directional_member(&self.type_def_ref(def), &member.key)

type_def_ref likely takes ownership or a reference; verify no unnecessary clone. Also SemanticId::TypeDef(owner.clone()) in directional_member clones unnecessarily — owner is already a reference.

18. directional_member — facts is borrowed from self.file_facts_of(...) but used after self.type_of_member(...)

let facts = self.file_facts_of(id.file_id)?;
...
let mut typ = self.type_of_member(&writer.id)?;
...
if let LuaType::Generic(generic) = prefix
    && let SemanticId::TypeDef(owner) = &writer.owner
    && let Some(def) = facts.type_def_by_id(&SemanticId::TypeDef(owner.clone()))

This compiles only if file_facts_of returns a 'db-lifetime reference (not tied to &self). Confirm the borrow checker accepts it; if not, restructure to avoid holding facts across type_of_member.

19. directional_read_at — PathState::default() and HashSet::new() per branch

Cloning path and seen for every branch is O(branches × path length). For hot paths this may be expensive. Consider using an immutable/persistent structure or a worklist with explicit backtracking.

20. directional_read_at — std::iter::once(...).chain(descendants()) is redundant

descendants() on a node typically includes the node itself in rowan/SyntaxNode. If so, once(expr.syntax().clone()) is redundant. Verify and simplify.

21. Test file asymmetric_fields.rs — mixed test styles

The file uses #[test] for most tests but #[googletest::gtest] for one. Be consistent; the codebase seems to use #[gtest] in LS tests and #[test] in analysis tests. The googletest import inside the function is unusual — move to top of file.

22. Test asymmetric_fields_reindex_and_incomplete_directions — ws.def_file overwrite semantics

ws.def_file("defs.lua", DEFS);
assert_eq!(ws.expr_ty("burner.force"), ws.ty("Fuel"));
ws.def_file("defs.lua", &DEFS.replace("(read) force Fuel", "(read) force number"));
assert_eq!(ws.expr_ty("burner.force"), LuaType::Number);

This relies on def_file replacing the file and re-indexing. Verify this is the intended API (vs. def_file appending). If it appends, the test would be checking the wrong thing.

23. asymmetric_fields_keys_and_independent_optionality — keyed[1] = nil expected to fail

assert!(!ws.has_no_diagnostic(DiagnosticCode::AssignTypeMismatch, "keyed[1] = nil"));

The write type is string (non-optional), so nil is rejected — correct. But the read type is number (non-optional), and the test asserts keyed[1] is number. Good. However, the test name says "independent optionality" — consider adding a case where read is optional and write is not (or vice versa) to actually exercise independence.

24. Docs — "no read-only or write-only diagnostic is introduced"

This is a deliberate design choice, but it means (read)-only fields silently accept writes of the read type. Users may expect a diagnostic. At minimum, document this prominently (it is documented, but the phrasing "If only one direction is supplied it is used for both operations" could be misread as "the write type is inferred from the read type"). Consider adding an example.

25. Docs — __newindex modeling not supported

Good to document. Consider also noting that rawset/rawget bypass directional checks.

Minor

• directional.rs (semantic_model): use super::{SemanticModel, member::MemberInfo}; — member module is private; verify MemberInfo is re-exported or make the import path consistent with other files.
• directional.rs: emmylua_parser::LuaExpr and emmylua_parser::LuaIndexExpr are fully qualified in signatures while LuaIndexExpr is imported — inconsistent.
• hover/directional.rs: db.document(model.file_id())? — model.file_id() may differ from key.file_id for cross-file declarations; the hover range is computed from the current file's document, which is correct, but verify token.text_range() is in the current file's coordinate space.
• directional_read_at: the doc comment says "Bound traversal conservatively" but the budget of 512 is a magic number — extract as a named constant.
• check_assign: the new continue after check_value_against skips check_self_member_assign and the rest of the index-expr handling. Confirm that's intended (it likely is, since the directional write type fully describes the contract).

Summary

The feature is well-tested and the design is sound. The main risks are:

1. Silent fallbacks in directional_read_at (budget/cycle → declared type) that can mask narrowing bugs.
2. Inconsistent nullable handling between directional_member and member_info_of.
3. Fragile string slicing in hover that can panic on non-ASCII files.
4. Behavioral change in function_provider.rs unrelated to the feature.
5. Missing version-visibility checks in the three directional-partner lookups.

I'd recommend addressing 1–4 before merge, and extracting a shared find_directional_partner helper to fix 5 consistently.

[hgschmie]

Addressed the two reproduced defects in 7704989b, appended after the original two commits:

• Item 7 — callback writer candidates: repeated directional callback writers previously inferred the first writer's parameter type, while ordinary repeated fields used the final candidate. The same problem reproduced with generic parameters. The fix retains all writer candidates for the existing contextual signature selection policy, with ordering, generic and unrelated-owner controls.
• Item 11 — hover descriptions: bare --- paragraph separators truncated preceding prose, and ---text lost its description. Hover now uses the parser's description attachment API. Regressions cover paragraphs, no-space prose, LF/CRLF and inline-description boundaries. This also removes the whole-file source slicing discussed in item 10; the proposed Unicode panic itself did not reproduce.

Fresh failing-before tests on the original head produced 4 failures and 6 passing controls. With the amendment, all 21 focused tests, 2,636 workspace tests, all-target/all-feature Clippy with warnings denied, and formatting checks pass. The original 11 directional tests remain unchanged.

I independently checked the other reported correctness concerns and retained their current behavior:

• 1–2: visited state is cloned per branch; the shared work budget deliberately falls back to the declared getter type. A missing flow node returns None through ?, rather than reaching an empty result. The shared-predecessor branch regression passes.
• 3–6: current version visibility operates on runtime closure signature documentation; directional field types are document types, so the suggested helper does not establish a field-version-block contract. Normal member identity selects the reader, while assignment checks/context explicitly select the writer. The remapping is one-way, and fresh reference/array/function optionality controls pass.
• 8–9: all four repeated direction-modifier combinations are rejected. Duplicate declarations are grouped by owner and key, without an adjacency requirement; additional declarations retain the existing duplicate/overload rules.
• 12–15: fresh cross-file prefix navigation, nested assignment and ordinary enum/callback/function-union completion controls pass. Lua has no compound assignment or indexed loop/parameter binding; plain names and nondirectional targets retain normal expression typing.

The remaining style/performance/documentation suggestions do not demonstrate another correctness failure. The traversal remains bounded, the one-direction fallback remains the documented contract, and unrelated style changes are left out of this amendment. No new review run is requested here; this PR remains a draft.

[xuhuanzy]

Merging is not being considered for now. The codebase has just undergone a complete refactor, and the maintenance focus is on restoring the usability it had before the refactor, so new features will not be accepted.

[hgschmie]

ok. Do you want me to keep this open or close and reopen when you are ready to take new features?

[CppCXY]

There's also a corresponding PR on the luals side at the moment, so this may be a reasonable PR. I'm inclined to leave it open.