[juliasilge]

Addresses posit-dev/positron#12098.

When you type an object name like outPath again and again, ark now returns one completion item for it, with the same icon and position each time:

Before this change, the icon and the position of the item changed between requests.

The cause was in the composite completion merger in composite.rs. The same R object comes back from three sources with three different kinds: VARIABLE from the document, STRUCT from the search path, and VALUE from the workspace. The dedupe key was (label, kind), so all three items survived. The merger kept them in a HashMap, so their order was random. 😱 Positron dedupes completions by insertText and keeps the first item with equal priority (see src/vs/editor/contrib/suggest/browser/suggest.ts). As a result, Positron showed a different one of the three items on each request.

Changes

• VARIABLE, STRUCT and VALUE now share one dedupe class. Object items also key on their insert text, so a quoted subset column ("x") and a global object x stay separate. All other kinds stay distinct. For example, for as a keyword and for as a snippet both remain, and so do glue:: and glue().
• When sources collide, the first source wins. The search path comes before the document, so the kept item has the search path docs and the {package} label.
• If the document also defines the object, the kept item takes the VARIABLE kind and sort_text. The user sees the variable icon, as the reporter expected, and the item stays in the variable sort bin. A pipe or subset column is already a VARIABLE, so it keeps its own sort_text.
• The merger uses an IndexMap in place of a HashMap, so the merge order is deterministic now. 😅 This adds indexmap to crates/ark/Cargo.toml.
• The trace log for a collision now shows the real kind of both items.

Known limitations

• A document assignment can shadow a package object, for example letters <- 1:3 that you did not run yet. The user then sees one letters {base} item with the variable icon and the docs from base. Before this change, Positron picked one of the two items at random.
• A data frame column in a pipe hides a package object with the same name. Positron already collapsed these two items before this change.
• Non-syntactic names (`my var`) are not deduplicated. The document label includes the backticks, but the search path label does not.

Testing

• New unit tests send synthetic items into the merge. They cover each rule: object kinds merge, the VARIABLE kind and sort_text takeover, different insert text, keyword and snippet, and package and function.
• New integration tests run real R through get_completions_at_cursor. One test asks for completions five times in a row to copy the retype behavior. It expects the same single item each time. Other tests cover an object that is only in the document, a pipe column, and a subset column.
• To do a manual test in Positron:
  1. In an R file, run outPath <- "myDirectory".
  2. On a new line, type out. Then delete it and type it again. Do this a few times.
  3. Before this change, the icon and position of outPath changed. After this change, you see one outPath item with the variable icon, in the same position each time.

Positron Release Notes

New Features

• N/A

Bug Fixes

• R completions now show one item for an object that the document defines and that is also in the global environment. The item has the same icon and position each time (Code Suggestions icons and order change by retyping the variable positron#12098).

[samclark2015]

LGTM! One comment below, a possible improvement on behavior. Good to merge with or without.

BTW - It looks like this (at least partially) addresses #813, in addition to the Positron issue 👀

[samclark2015]

This promotes the document-level definition, but it drops some useful data. For instance, documentation ("Defined in this document on line N.") is missing, and label_details points to the shadowed definition's package source. With letters <- 1:3 unrun, you get letters {base}.

Could we replace the item, instead of just updating these two attributes? I think that resolves the letters known limitation, since the search-path item doesn't carry docs for objects. It'd also cover function parameters (function(letters) lett), which hit the same merge but never get fixed by running code.

[juliasilge]

Thanks, that is a great point! Added in a53afb4.