bug: LSP completion dialog should only recommend public members of a module

[0xNeshi]

Bug Report

Cairo version:
2.6.3

Current behavior:

When defining a module with some public members and trying to import said members, a completion dialog appears, but the language server includes non-public members in the dialog. Additionally, the lang. server includes non-members in the dialog (in this case ShapeGeometry).

Example below:

Trying to import non-public members (and non-members) results in an error of course:

Importing a public member works as expected:

Expected behavior:

The completion dialog should only include actual module members and only those that can actually be imported.

Steps to reproduce:

• create a new Scarb package
• open lib.cairo in VS Code (ensure Cairo 1.0 extension is installed)
• paste the code from "Related code" section below into the opened file
• put the cursor at the end of the incomplete use circle:: statement
• prompt the completion dialog to open (usually CTRL + Space)

Related code:

trait ShapeGeometry<T> {
    fn boundary(self: T) -> u64;
}

mod circle {
    use super::ShapeGeometry;

    #[derive(Drop)]
    pub struct Circle {
        pub radius: u64,
    }

    impl CircleGeometry of ShapeGeometry<Circle> {
        fn boundary(self: Circle) -> u64 {
            (2 * 314 * self.radius) / 100
        }
    }
}

use circle::

Other information:

Related issue: #5664

[Utilitycoder]

Hi, I would love to work on this. I however need pointers to where this should be fixed.

[mkaput]

Task is yours. Thanks ♥️

The entry point for completion logic is here:

cairo/crates/cairo-lang-language-server/src/ide/completion/mod.rs

Lines 19 to 59 in c6d6029

	/// Compute completion items at a given cursor position.
	#[tracing::instrument(
	level = "debug",
	skip_all,
	fields(uri = %params.text_document_position.text_document.uri)
	)]
	pub fn complete(params: CompletionParams, db: &RootDatabase) -> Option<CompletionResponse> {
	let text_document_position = params.text_document_position;
	let file_id = db.file_for_url(&text_document_position.text_document.uri)?;
	let mut position = text_document_position.position;
	position.character = position.character.saturating_sub(1);
	let mut node = db.find_syntax_node_at_position(file_id, position.to_cairo())?;
	let lookup_items = db.collect_lookup_items_stack(&node)?;
	let module_file_id = db.find_module_file_containing_node(&node)?;
	// Skip trivia.
	while ast::Trivium::is_variant(node.kind(db))
	|| node.kind(db) == SyntaxKind::Trivia
	|| node.kind(db).is_token()
	{
	node = node.parent().unwrap_or(node);
	}
	let trigger_kind =
	params.context.map(|it| it.trigger_kind).unwrap_or(CompletionTriggerKind::INVOKED);
	match completion_kind(db, node) {
	CompletionKind::Dot(expr) => {
	dot_completions(db, file_id, lookup_items, expr).map(CompletionResponse::Array)
	}
	CompletionKind::ColonColon(segments) if !segments.is_empty() => {
	colon_colon_completions(db, module_file_id, lookup_items, segments)
	.map(CompletionResponse::Array)
	}
	_ if trigger_kind == CompletionTriggerKind::INVOKED => {
	Some(CompletionResponse::Array(generic_completions(db, module_file_id, lookup_items)))
	}
	_ => None,
	}
	}

This looks to be ::-completion, which is computed here:

cairo/crates/cairo-lang-language-server/src/ide/completion/completions.rs

Lines 115 to 188 in c6d6029

	#[tracing::instrument(level = "trace", skip_all)]
	pub fn colon_colon_completions(
	db: &(dyn SemanticGroup + 'static),
	module_file_id: ModuleFileId,
	lookup_items: Vec<LookupItemId>,
	segments: Vec<PathSegment>,
	) -> Option<Vec<CompletionItem>> {
	// Get a resolver in the current context.
	let resolver_data = match lookup_items.into_iter().next() {
	Some(item) => {
	(*item.resolver_data(db).ok()?).clone_with_inference_id(db, InferenceId::NoContext)
	}
	None => Resolver::new(db, module_file_id, InferenceId::NoContext).data,
	};
	let mut resolver = Resolver::with_data(db, resolver_data);
	let mut diagnostics = SemanticDiagnostics::default();
	let item = resolver
	.resolve_concrete_path(&mut diagnostics, segments, NotFoundItemType::Identifier)
	.ok()?;
	Some(match item {
	ResolvedConcreteItem::Module(module_id) => db
	.module_items(module_id)
	.unwrap_or_default()
	.iter()
	.map(|item| CompletionItem {
	label: item.name(db.upcast()).to_string(),
	kind: ResolvedGenericItem::from_module_item(db, *item)
	.ok()
	.map(resolved_generic_item_completion_kind),
	..CompletionItem::default()
	})
	.collect(),
	ResolvedConcreteItem::Trait(item) => db
	.trait_functions(item.trait_id(db))
	.unwrap_or_default()
	.iter()
	.map(|(name, _)| CompletionItem {
	label: name.to_string(),
	kind: Some(CompletionItemKind::FUNCTION),
	..CompletionItem::default()
	})
	.collect(),
	ResolvedConcreteItem::Impl(item) => item
	.concrete_trait(db)
	.map(|trait_id| {
	db.trait_functions(trait_id.trait_id(db))
	.unwrap_or_default()
	.iter()
	.map(|(name, _)| CompletionItem {
	label: name.to_string(),
	kind: Some(CompletionItemKind::FUNCTION),
	..CompletionItem::default()
	})
	.collect()
	})
	.unwrap_or_default(),
	ResolvedConcreteItem::Type(ty) => match ty.lookup_intern(db) {
	TypeLongId::Concrete(ConcreteTypeId::Enum(enum_id)) => db
	.enum_variants(enum_id.enum_id(db))
	.unwrap_or_default()
	.iter()
	.map(|(name, _)| CompletionItem {
	label: name.to_string(),
	kind: Some(CompletionItemKind::ENUM_MEMBER),
	..CompletionItem::default()
	})
	.collect(),
	_ => vec![],
	},
	_ => vec![],
	})
	}

As you can see, this is basically just a banch of ifs for various AST items.

I guess you simply have to put appropriate filters here:

cairo/crates/cairo-lang-language-server/src/ide/completion/completions.rs

Lines 137 to 148 in c6d6029

	ResolvedConcreteItem::Module(module_id) => db
	.module_items(module_id)
	.unwrap_or_default()
	.iter()
	.map(|item| CompletionItem {
	label: item.name(db.upcast()).to_string(),
	kind: ResolvedGenericItem::from_module_item(db, *item)
	.ok()
	.map(resolved_generic_item_completion_kind),
	..CompletionItem::default()
	})
	.collect(),

---

BTW It would be awesome if you'd also contribute completion E2E tests, as you're touching this. There are none yet, but they should be pretty similar to hover test for example:

https://github.com/starkware-libs/cairo/blob/main/crates/cairo-lang-language-server/tests/e2e/hover.rs
https://github.com/starkware-libs/cairo/tree/main/crates/cairo-lang-language-server/tests/test_data/hover

Would you like to make this part? I don't want to enforce this on you. 😃

[mkaput]

@Utilitycoder hey, how's it going? Would you like some assistance from my side in this area?

[Utilitycoder]

@mkaput I have been sick but slowly getting back to optimum health. I will take a look and let you know If I'd be able to proceed with it.

[mkaput]

Moving this issue back to the backlog due to lack of activity. It's open for grabbing again!

[piotmag769]

I think it is done (at least partially) after #6060 and #6091, no? We may just want to review if the logic of using public items only is needed somewhere else @mkaput @gilbens-starkware

[mkaput]

I think it is done (at least partially) after #6060 and #6091, no? We may just want to review if the logic of using public items only is needed somewhere else @mkaput @gilbens-starkware

I didn't verify what you said, but I doubt so: Gil's work is adding auto-imports for stuff that is pub still. The problem is that LS is also suggesting things that are not pub at all.

For example, in the screenshot from the issue description, ShapeGeometry should never be suggested because it's not pub use.

[piotmag769]

Okay, so it is basically about fixing dot completions then

[gilbens-starkware]

Indeed it is not fixed.
However, a good entry point now will be to look at visible_traits_from_module, this issue will need something similar e.g. visible_items_from_issue.