From b4f3ea9cbafd4d8866b24e5c751c68628bcc849e Mon Sep 17 00:00:00 2001 From: rifuki Date: Tue, 25 Aug 2026 16:55:16 +0700 Subject: [PATCH] fix(completion): offer using-for extensions on values, not just non-contract types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `using SafeERC20 for IERC20` produced no completions on a variable declared as `IERC20`. The suppression that caused it was deliberate but keyed on the wrong thing: let is_contract_name = resolved_node_id .map(|nid| cache.contract_kinds.contains_key(&nid)) The intent, per the comment above it, was that typing a *type name* — `Lock.` — should offer Lock's own members rather than functions from `using Pool for *`. That is right. But the check asks "is this type a contract?", and `IERC20.` and a `paymentToken` declared as `IERC20` carry the identical typeIdentifier, so the answer is yes for both. Values lost their extensions along with type names. The distinction cannot be recovered from the type. It has to come from where the receiver was resolved, which already knows: `name_to_type` means a variable, `name_to_node_id` means a contract/library/interface name. That was simply discarded before reaching `completions_for_type`. Threads a `ReceiverKind` down from the resolvers instead. Plain access keeps whatever the name resolved to; a cast (`IERC20(addr).`), a mapping index, and any member reached by walking a chain are values regardless of the name in front, so they get extensions too. One extra hop was needed for the cast case. A contract name with no reverse entry in `type_to_node` resolves to the synthetic `__node_id_N` marker, which can never match a real using-for key like `t_contract$_IHooks_$1840`, so `lookup_using_for` now falls back to matching on the node id. Covers repro 1 of #225. The nested-enum and live-edit repros in that issue are separate and not addressed here. Tests use the poolmanager.json fixture, which carries `using Hooks for IHooks` and a `hooks` variable declared as `IHooks`. `hasPermission` and `isValidHookAddress` exist only on the library, never on the interface, so they tell the two sources apart: hooks. must offer them — failed on main IHooks(addr). must offer them — failed on main IHooks. must not — already correct, now locked in --- docs/pages/reference/completions.md | 8 ++ src/completion.rs | 133 +++++++++++++++++++++------- tests/completion.rs | 75 ++++++++++++++++ 3 files changed, 182 insertions(+), 34 deletions(-) diff --git a/docs/pages/reference/completions.md b/docs/pages/reference/completions.md index 3930c20..2948fa6 100644 --- a/docs/pages/reference/completions.md +++ b/docs/pages/reference/completions.md @@ -82,6 +82,14 @@ Final member set is composed from: - `using_for` matches, - `using_for_wildcard`. +The last two are included only when the receiver is a **value**, not when it +names a type: `paymentToken.` (declared as `IERC20`) gets `using SafeERC20 for +IERC20` extensions, while `IERC20.` gets only the interface's own members. Both +carry the same `typeIdentifier`, so the distinction is carried down from name +resolution — a `name_to_type` hit is a value, a `name_to_node_id` hit is a type +name. Casts (`IERC20(addr).`), mapping indexes and chained members are values +regardless of the name in front of them. + ## Scope-aware name resolution When resolving a symbol in context, completion walks: diff --git a/src/completion.rs b/src/completion.rs index 961a7c5..310a5d9 100644 --- a/src/completion.rs +++ b/src/completion.rs @@ -1261,12 +1261,29 @@ fn strip_type_suffix(type_id: &str) -> &str { /// Look up using-for completions for a type, trying suffix variants. /// The AST stores types with different suffixes (_storage_ptr, _storage, _memory_ptr, etc.) /// across different contexts, so we try multiple forms. -fn lookup_using_for(cache: &CompletionCache, type_id: &str) -> Vec { +fn lookup_using_for( + cache: &CompletionCache, + type_id: &str, + node_id: Option, +) -> Vec { // Exact match first if let Some(items) = cache.using_for.get(type_id) { return items.clone(); } + // A contract name that has no entry in `type_to_node` resolves to the + // synthetic `__node_id_N` marker, which can never match a real key like + // `t_contract$_IHooks_$1840`. Recover by matching on the node id instead. + if let Some(nid) = node_id + && type_id.starts_with("__node_id_") + && let Some((_, items)) = cache + .using_for + .iter() + .find(|(tid, _)| extract_node_id_from_type(tid.as_str()) == Some(nid)) + { + return items.clone(); + } + // Strip to base form, then try all common suffix variants let base = strip_type_suffix(type_id); let variants = [ @@ -1290,7 +1307,11 @@ fn lookup_using_for(cache: &CompletionCache, type_id: &str) -> Vec Vec { +fn completions_for_type( + cache: &CompletionCache, + type_id: &str, + receiver: ReceiverKind, +) -> Vec { // Address type if type_id == "t_address" || type_id == "t_address_payable" { let mut items = address_members(); @@ -1335,16 +1356,17 @@ fn completions_for_type(cache: &CompletionCache, type_id: &str) -> Vec Vec Option { - // Direct type lookup +/// Whether the expression before the dot *names a type* or *is a value* of one. +/// +/// Both produce the same `typeIdentifier` — `IERC20.` and a `paymentToken` +/// declared as `IERC20` both resolve to `t_contract$_IERC20_$N` — so the +/// distinction cannot be recovered from the type alone and has to be carried +/// down from wherever the receiver was resolved. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ReceiverKind { + /// `IERC20.` — the contract/library/interface itself. + TypeName, + /// `paymentToken.` — a value whose type is that contract. + Value, +} + +/// Resolve a type identifier for a name, considering name_to_type and +/// name_to_node_id, reporting whether the name was a variable (a value) or a +/// contract/library/interface name (a type). +fn resolve_name_to_type_id_kind( + cache: &CompletionCache, + name: &str, +) -> Option<(String, ReceiverKind)> { + // Direct type lookup — the name is a variable, so this is a value. if let Some(tid) = cache.name_to_type.get(name) { - return Some(tid.to_string()); + return Some((tid.to_string(), ReceiverKind::Value)); } // Contract/library/interface name → synthesize a type id from node id if let Some(node_id) = cache.name_to_node_id.get(name) { // Find a matching typeIdentifier in type_to_node (reverse lookup) for (tid, nid) in &cache.type_to_node { if nid == node_id { - return Some(tid.to_string()); + return Some((tid.to_string(), ReceiverKind::TypeName)); } } // Fallback: use a synthetic marker so completions_for_type can resolve via node_id - return Some(format!("__node_id_{}", node_id)); + return Some((format!("__node_id_{}", node_id), ReceiverKind::TypeName)); } None } @@ -1406,7 +1447,7 @@ pub fn find_innermost_scope( /// declarations for a matching name. If not found, follow `scope_parent` to the /// next enclosing scope and check again. Stop at the first match. /// -/// Falls back to `resolve_name_to_type_id` (flat lookup) if scope resolution +/// Falls back to `resolve_name_to_type_id_kind` (flat lookup) if scope resolution /// finds nothing, or if the scope data is unavailable. pub fn resolve_name_in_scope( cache: &CompletionCache, @@ -1414,7 +1455,20 @@ pub fn resolve_name_in_scope( byte_pos: usize, file_id: FileId, ) -> Option { - let mut current_scope = find_innermost_scope(cache, byte_pos, file_id)?; + resolve_name_in_scope_kind(cache, name, byte_pos, file_id).map(|(tid, _)| tid) +} + +/// Like [`resolve_name_in_scope`], but also reports whether the resolved name +/// was a declaration in scope (a value) or a contract name (a type). +fn resolve_name_in_scope_kind( + cache: &CompletionCache, + name: &str, + byte_pos: usize, + file_id: FileId, +) -> Option<(String, ReceiverKind)> { + let Some(mut current_scope) = find_innermost_scope(cache, byte_pos, file_id) else { + return resolve_name_to_type_id_kind(cache, name); + }; // Walk up the scope chain loop { @@ -1422,7 +1476,7 @@ pub fn resolve_name_in_scope( if let Some(decls) = cache.scope_declarations.get(¤t_scope) { for decl in decls { if decl.name == name { - return Some(decl.type_id.clone()); + return Some((decl.type_id.clone(), ReceiverKind::Value)); } } } @@ -1435,7 +1489,7 @@ pub fn resolve_name_in_scope( if let Some(decls) = cache.scope_declarations.get(&base_id) { for decl in decls { if decl.name == name { - return Some(decl.type_id.clone()); + return Some((decl.type_id.clone(), ReceiverKind::Value)); } } } @@ -1451,7 +1505,7 @@ pub fn resolve_name_in_scope( // Scope walk found nothing — fall back to flat lookup // (handles contract/library names which aren't in scope_declarations) - resolve_name_to_type_id(cache, name) + resolve_name_to_type_id_kind(cache, name) } /// Resolve a name within a type context to get the member's type. @@ -1535,10 +1589,19 @@ fn resolve_name( name: &str, scope_ctx: Option<&ScopeContext>, ) -> Option { + resolve_name_kind(cache, name, scope_ctx).map(|(tid, _)| tid) +} + +/// Like [`resolve_name`], but also reports whether the name was a value or a type. +fn resolve_name_kind( + cache: &CompletionCache, + name: &str, + scope_ctx: Option<&ScopeContext>, +) -> Option<(String, ReceiverKind)> { if let Some(ctx) = scope_ctx { - resolve_name_in_scope(cache, name, ctx.byte_pos, ctx.file_id) + resolve_name_in_scope_kind(cache, name, ctx.byte_pos, ctx.file_id) } else { - resolve_name_to_type_id(cache, name) + resolve_name_to_type_id_kind(cache, name) } } @@ -1553,11 +1616,10 @@ pub fn get_dot_completions( return items; } - // Try to resolve the identifier's type - let type_id = resolve_name(cache, identifier, scope_ctx); - - if let Some(tid) = type_id { - return completions_for_type(cache, &tid); + // Plain access, so a contract name stays a type name and a variable stays + // a value — exactly the distinction using-for extensions hinge on. + if let Some((tid, receiver)) = resolve_name_kind(cache, identifier, scope_ctx) { + return completions_for_type(cache, &tid, receiver); } vec![] @@ -1590,13 +1652,15 @@ pub fn get_chain_completions( } // foo(). — could be a function call or a type cast like IFoo(addr). // First check if it's a type cast: name matches a contract/interface/library + // A cast yields a *value* of that type, so extensions apply even + // though the name itself is a contract. if let Some(type_id) = resolve_name(cache, &seg.name, scope_ctx) { - return completions_for_type(cache, &type_id); + return completions_for_type(cache, &type_id, ReceiverKind::Value); } // Otherwise look up as a function call — check all function_return_types for ((_, fn_name), ret_type) in &cache.function_return_types { if fn_name == &seg.name { - return completions_for_type(cache, ret_type); + return completions_for_type(cache, ret_type, ReceiverKind::Value); } } return vec![]; @@ -1607,7 +1671,7 @@ pub fn get_chain_completions( && tid.starts_with("t_mapping") && let Some(val_type) = extract_mapping_value_type(&tid) { - return completions_for_type(cache, &val_type); + return completions_for_type(cache, &val_type, ReceiverKind::Value); } return vec![]; } @@ -1651,9 +1715,10 @@ pub fn get_chain_completions( current_type = resolve_member_type(cache, &ctx_type, &seg.name, &seg.kind); } - // Return completions for the final resolved type + // Return completions for the final resolved type. Anything reached by + // walking members is a value, never a type name. match current_type { - Some(tid) => completions_for_type(cache, &tid), + Some(tid) => completions_for_type(cache, &tid, ReceiverKind::Value), None => vec![], } } diff --git a/tests/completion.rs b/tests/completion.rs index d2e9080..8f03ddc 100644 --- a/tests/completion.rs +++ b/tests/completion.rs @@ -3690,3 +3690,78 @@ fn test_type_meta_items_have_detail() { let iid_item = items.iter().find(|i| i.label == "interfaceId").unwrap(); assert_eq!(iid_item.detail.as_deref(), Some("bytes4")); } + +// ── using-for extensions on values (issue #225) ──────────────────────────── + +/// `using Hooks for IHooks` must offer the library's functions on a *value* +/// typed `IHooks`. Previously the receiver's kind was inferred from its +/// typeIdentifier, and because `IHooks` is a contract the extensions were +/// suppressed on values as well as on the type name. +#[test] +fn test_using_for_extensions_appear_on_a_contract_typed_value() { + let cache = load_cache(); + + let labels: Vec = get_dot_completions(&cache, "hooks", None) + .into_iter() + .map(|i| i.label) + .collect(); + + assert!( + !labels.is_empty(), + "`hooks` (declared as IHooks) resolved to nothing" + ); + // `hasPermission` and `isValidHookAddress` exist only on the Hooks library, + // not on the IHooks interface, so they can only have arrived via using-for. + for expected in ["hasPermission", "isValidHookAddress"] { + assert!( + labels.iter().any(|l| l == expected), + "`hooks.` is missing the `using Hooks for IHooks` extension {expected:?}; got {labels:?}" + ); + } +} + +/// The flip side: typing the *type name* must not pull in library extensions, +/// which is the behavior the original suppression was written to protect. +#[test] +fn test_using_for_extensions_absent_on_the_type_name() { + let cache = load_cache(); + + let labels: Vec = get_dot_completions(&cache, "IHooks", None) + .into_iter() + .map(|i| i.label) + .collect(); + + // Library-only names must not leak onto the type. Names such as `beforeSwap` + // are deliberately not asserted here: IHooks declares them itself, so they + // legitimately appear either way. + for unexpected in ["hasPermission", "isValidHookAddress"] { + assert!( + !labels.iter().any(|l| l == unexpected), + "`IHooks.` should not offer the library extension {unexpected:?}; got {labels:?}" + ); + } +} + +/// A cast produces a value, so `IHooks(addr).` gets the extensions even though +/// the name before the parenthesis is a contract. +#[test] +fn test_using_for_extensions_appear_after_a_cast() { + use solidity_language_server::completion::get_chain_completions; + + let cache = load_cache(); + let chain = vec![DotSegment { + name: "IHooks".to_string(), + kind: AccessKind::Call, + call_args: Some("addr".to_string()), + }]; + + let labels: Vec = get_chain_completions(&cache, &chain, None) + .into_iter() + .map(|i| i.label) + .collect(); + + assert!( + labels.iter().any(|l| l == "hasPermission"), + "`IHooks(addr).` should offer library extensions; got {labels:?}" + ); +}