Skip to content

Conversation

@BurntSushi
Copy link
Member

Most of the work here was doing some light refactoring to facilitate
sensible testing. That is, we don't want to list every builtin included
in most tests, so we add some structure to the completion type returned.
Tests can now filter based on whether a completion is a builtin or not.

Otherwise, builtins are found using the existing infrastructure for
object.attr completions (where we hard-code the module name
builtins).

I did consider changing the sort order based on whether a completion
suggestion was a builtin or not. In particular, it seemed like it might
be a good idea to sort builtins after other scope based completions,
but before the dunder and sunder attributes. Namely, it seems likely
that there is an inverse correlation between the size of a scope and
the likelihood of an item in that scope being used at any given point.
So it might be a good idea to prioritize the likelier candidates in
the completions returned.

Additionally, the number of items introduced by adding builtins is quite
large. So I wondered whether mixing them in with everything else would
become too noisy.

However, it's not totally clear to me that this is the right thing to
do. Right now, I feel like there is a very obvious lexicographic
ordering that makes "finding" the right suggestion to activate
potentially easier than if the ranking mechanism is less clear.
(Technically, the dunder and sunder attributes are not sorted
lexicographically, but I'd put forward that most folks don't have an
intuitive understanding of where _ ranks lexicographically with
respect to "regular" letters. Moreover, since dunder and sunder
attributes are all grouped together, I think the ordering here ends up
being very obvious after even a quick glance.)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 27, 2025

mypy_primer results

No ecosystem changes detected ✅

@AlexWaygood AlexWaygood added server Related to the LSP server ty Multi-file analysis & type inference labels Jun 27, 2025
@BurntSushi BurntSushi force-pushed the ag/completion-builtins branch from e4b9b1d to 1d7559f Compare June 27, 2025 13:12
@codspeed-hq
Copy link

codspeed-hq bot commented Jun 27, 2025

CodSpeed WallTime Performance Report

Merging #18982 will not alter performance

Comparing ag/completion-builtins (67134b1) with main (a3c79d8)

Summary

✅ 8 untouched benchmarks

Most of the work here was doing some light refactoring to facilitate
sensible testing. That is, we don't want to list every builtin included
in most tests, so we add some structure to the completion type returned.
Tests can now filter based on whether a completion is a builtin or not.

Otherwise, builtins are found using the existing infrastructure for
`object.attr` completions (where we hard-code the module name
`builtins`).

I did consider changing the sort order based on whether a completion
suggestion was a builtin or not. In particular, it seemed like it might
be a good idea to sort builtins after other scope based completions,
but before the dunder and sunder attributes. Namely, it seems likely
that there is an inverse correlation between the size of a scope and
the likelihood of an item in that scope being used at any given point.
So it *might* be a good idea to prioritize the likelier candidates in
the completions returned.

Additionally, the number of items introduced by adding builtins is quite
large. So I wondered whether mixing them in with everything else would
become too noisy.

However, it's not totally clear to me that this is the right thing to
do. Right now, I feel like there is a very obvious lexicographic
ordering that makes "finding" the right suggestion to activate
potentially easier than if the ranking mechanism is less clear.
(Technically, the dunder and sunder attributes are not sorted
lexicographically, but I'd put forward that most folks don't have an
intuitive understanding of where `_` ranks lexicographically with
respect to "regular" letters. Moreover, since dunder and sunder
attributes are all grouped together, I think the ordering here ends up
being very obvious after even a quick glance.)
@BurntSushi BurntSushi force-pushed the ag/completion-builtins branch from 1d7559f to 67134b1 Compare June 27, 2025 13:59
@BurntSushi BurntSushi merged commit 5f6b0de into main Jun 27, 2025
36 checks passed
@BurntSushi BurntSushi deleted the ag/completion-builtins branch June 27, 2025 14:20
BurntSushi added a commit that referenced this pull request Jun 27, 2025
I renamed a field on a `Completion` struct in #18982, and it looks like
this caused the playground to fail to build:
https://github.com/astral-sh/ruff/actions/runs/15928550050/job/44931734349

Maybe building that playground can be added to CI for pull requests?
@BurntSushi BurntSushi mentioned this pull request Jun 27, 2025
dcreager added a commit that referenced this pull request Jun 27, 2025
* main:
  [ty] Add builtins to completions derived from scope (#18982)
  [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487)
  [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981)
  [ty] Add micro-benchmark for #711 (#18979)
  [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974)
  [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975)
  [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963)
  [`pylint`] Fix `PLC0415` example (#18970)
  [ty] Add environment variable to dump Salsa memory usage stats (#18928)
  [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678)
  Bump 0.12.1 (#18969)
  [`FastAPI`] Add fix safety section to `FAST002` (#18940)
  [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965)
  [ty] Resolve python environment in `Options::to_program_settings` (#18960)
  [`ruff`] Fix false positives and negatives in `RUF010` (#18690)
  [ty] Fix rendering of long lines that are indented with tabs
  [ty] Add regression test for diagnostic rendering panic
  [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)
BurntSushi added a commit that referenced this pull request Jun 27, 2025
I renamed a field on a `Completion` struct in #18982, and it looks like
this caused the playground to fail to build:

https://github.com/astral-sh/ruff/actions/runs/15928550050/job/44931734349

Maybe building that playground can be added to CI for pull requests?
@AlexWaygood
Copy link
Member

I did consider changing the sort order based on whether a completion suggestion was a builtin or not. In particular, it seemed like it might be a good idea to sort builtins after other scope based completions, but before the dunder and sunder attributes. Namely, it seems likely that there is an inverse correlation between the size of a scope and the likelihood of an item in that scope being used at any given point. So it might be a good idea to prioritize the likelier candidates in the completions returned.

Additionally, the number of items introduced by adding builtins is quite large. So I wondered whether mixing them in with everything else would become too noisy.

However, it's not totally clear to me that this is the right thing to do. Right now, I feel like there is a very obvious lexicographic ordering that makes "finding" the right suggestion to activate potentially easier than if the ranking mechanism is less clear. (Technically, the dunder and sunder attributes are not sorted lexicographically, but I'd put forward that most folks don't have an intuitive understanding of where _ ranks lexicographically with respect to "regular" letters. Moreover, since dunder and sunder attributes are all grouped together, I think the ordering here ends up being very obvious after even a quick glance.)

I think I'd be in favour of making this change so that they're sorted below other candidates... in the playground here, it was slightly annoying that Protocol was sorted below PendingDeprecationWarning in the list of autocompletions. It seems obvious that it's the most likely candidate for what I want, given that I just imported it! 😆

image

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Looks great!

@BurntSushi
Copy link
Member Author

@AlexWaygood I think if we were going to go that route, we'd need to be more thoughtful about our ranking. I'm pretty concerned about having an unpredictable order and that making it difficult for folks to scan the list quickly to find what they're looking for.

@AlexWaygood
Copy link
Member

I definitely get that concern. I think having the likeliest attribute be as close to the top of the list as possible is quite high-value, but I definitely agree that having a consistent and predictable behaviour is also high-value. I'm fine with parking this for now and coming back to it later!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

server Related to the LSP server ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants