Skip to content

fix: namespace completion to only use the short name #8350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 22, 2025

Conversation

Rob23oba
Copy link
Contributor

@Rob23oba Rob23oba commented May 15, 2025

This PR changes namespace completion to use the same algorithm as declaration identifier completion, which makes it use the short name (last name component) for completions instead of the full name, avoiding namespace duplications.

Closes #5654

@Rob23oba Rob23oba requested a review from mhuisi as a code owner May 15, 2025 09:33
@github-actions github-actions bot added changelog-server Language server, widgets, and IDE extensions toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN labels May 15, 2025
@leanprover-community-bot
Copy link
Collaborator

leanprover-community-bot commented May 15, 2025

Mathlib CI status (docs):

  • ❗ Batteries/Mathlib CI will not be attempted unless your PR branches off the nightly-with-mathlib branch. Try git rebase 88078930a92aa55089e936a5fe8e99ec65ab2ceb --onto 6ca31baa55e4b29c9667085736863f9ca840b16c. You can force Mathlib CI using the force-mathlib-ci label. (2025-05-15 11:20:19)
  • ✅ Mathlib branch lean-pr-testing-8350 has successfully built against this PR. (2025-05-19 20:00:17) View Log

Copy link
Contributor

@mhuisi mhuisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

It's probably fine, but could you also double-check that completion still performs roughly the same with this change with import Mathlib?

You can do so by rebasing your PR on nightly-with-mathlib , creating a new project that depends on the PR testing branch, setting Lean4 > Trace: Server in VS Code to 'messages', observing the timings in the 'Lean: Editor' output panel for completion requests that VS Code emits and comparing against master. Single character identifier completion requests are usually the worst case.

@Rob23oba Rob23oba force-pushed the no-namespace-dup branch from a8a56b8 to 2460fe0 Compare May 19, 2025 18:45
leanprover-community-mathlib4-bot added a commit to leanprover-community/batteries that referenced this pull request May 19, 2025
leanprover-community-mathlib4-bot added a commit to leanprover-community/mathlib4 that referenced this pull request May 19, 2025
@leanprover-community-bot leanprover-community-bot added the builds-mathlib CI has verified that Mathlib builds against this PR label May 19, 2025
@Rob23oba
Copy link
Contributor Author

I ran the following test:

Test code

import Mathlib

open Lean Elab Command Server Completion

def test (c : Syntax) : CommandElabM Unit := do
  elabCommand c
  let fileMap := FileMap.ofString c.getTrailing?.get!.str
  let position := c.getTailPos?.get!
  let pos := fileMap.toPosition position
  let params : Lsp.CompletionParams := {
    textDocument := {
      uri := "file://Basic.lean"
    }
    position := {
      line := pos.line
      character := pos.column
    }
  }
  for _ in [0:30] do
    let trees ← getInfoTrees
    for tree in trees do
      let token ← RequestCancellationToken.new
      let time ← IO.monoNanosNow
      let hb ← IO.getNumHeartbeats
      discard <| (find? params fileMap position c tree {}).run token
      let hb' ← IO.getNumHeartbeats
      let time' ← IO.monoNanosNow
      Lean.logInfo m!"timing: {time' - time}, heartbeats: {hb' - hb}"

elab "#test_completion " c:command : command => test c

#test_completion #check A

on both nightly-testing and this PR, the results:

  1. heartbeats increased marginally: 19379397 -> 19467124 (both rather consistent)
  2. time decreased slightly (although this is probably just noise): average 691ms, standard deviation 25ms -> avarage 683ms, standard deviation 12ms

@mhuisi
Copy link
Contributor

mhuisi commented May 22, 2025

Great! :-)

@mhuisi mhuisi added this pull request to the merge queue May 22, 2025
Merged via the queue into leanprover:master with commit 2594a8e May 22, 2025
19 checks passed
@Rob23oba Rob23oba deleted the no-namespace-dup branch May 22, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builds-mathlib CI has verified that Mathlib builds against this PR changelog-server Language server, widgets, and IDE extensions toolchain-available A toolchain is available for this PR, at leanprover/lean4-pr-releases:pr-release-NNNN
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Namespace completion inserts full name rather than remainder
3 participants