This commit makes the "Wrap return type in Result" assist prefer type aliases of standard library
type when the are in scope, use at least one generic parameter, and have the name "Result".
The last restriction was made in an attempt to avoid false assumptions about which type the
user is referring to, but that might be overly strict. We could also do something like this, in
order of priority:
* Use the alias named "Result".
* Use any alias if only a single one is in scope, otherwise:
* Use the standard library type.
This is easy to add if others feel differently that is appropriate, just let me know.
internal: Lay basic ground work for standalone mbe tests
Most of our mbe hir-def tests don't actually do anything name res relevant, we can (and should) move those down the stack into `mbe/hir-expand`.
Handle attributes correctly in "Flip comma"
Attributes often contain path followed by a token tree (e.g. `align(2)`), and the previous code handled them as two separate items, which led to results such as `#[repr(alignC, (2))]`.
An alternative is to just make the assist unavailable in attributes, like we do in macros. But contrary to macros, attributes often have a fixed form, so this seems useful.
Fixes#18013.
Attributes often contain path followed by a token tree (e.g. `align(2)`, and the previous code handled them as two separate items, which led to results such as `#[repr(alignC, (2))]`.
An alternative is to just make the assist unavailable in attributes, like we do in macros. But contrary to macros, attributes often have a fixed form, so this seems useful.
internal: Add doc comments to OpQueue
I spent a while debugging some OpQueue behaviours and found the API slightly confusing, so I've added doc comments to clarify what each OpQueue method does.
internal: Improve inlay hint resolution reliability
The payload now ships the range the inlay hint ought to be triggered for instead of trying to estimate it from its position which is somewhat brittle
Do not report missing unsafe on `addr_of[_mut]!(EXTERN_OR_MUT_STATIC)`
The compiler no longer does as well; see https://github.com/rust-lang/rust/pull/125834.
Also require unsafe when accessing `extern` `static` (other than by `addr_of!()`).
Fixes#17978.
fix: `std::error::Error` is object unsafe
Fixes#17998
I tried to get generic predicates of assoc function itself, not inherited from the parent here;
0ae42bd425/crates/hir-ty/src/object_safety.rs (L420-L442)
But this naive equality check approach doesn't work when the assoc function has one or more generic paramters like;
```rust
trait Foo {}
trait Bar: Foo {
fn bar(&self);
}
```
because the generic predicates of the parent, `Bar` is `[^1.0 implements Foo]` and the generic predicates of `fn bar` is `[^1.1 implements Foo]`, which are different.
This PR implements a correct logic for filtering out parent generic predicates for this.
This makes the generated impl's indentation match the ADT it targets, improving formatting when
using nested modules inside of the same file or when defining types inside of a function.
Consider field attributes when converting from tuple to named struct and the opposite
Fixes#17983.
I tried to use the `SourceChangeBuilder::make_mut()` API, but it duplicated the attribute...
fix: Don't add reference when it isn't needed for the "Extract variable" assist
I.e. don't generate `let var_name = &foo()`. Because it always irritates me when I need to fix that.
Anything that creates a new value don't need a reference. That excludes mostly field accesses and indexing.
I had a thought that we can also not generate a reference for fields and indexing as long as the type is `Copy`, but sometimes people impl `Copy` even when they don't want to copy the values (e.g. a large type), so I didn't do that.
Fix incorrect symbol definitions in SCIP output
The SCIP output incorrectly marks some symbols as definitions because it doesn't account for the file ID when comparing the token's range to its definition's range.
This means that if a symbol is referenced in a file at the same position at which it is defined in another file, that reference will be marked as a definition. I was quite surprised by how common this is. For example, `PartialEq` is defined [here](https://github.com/rust-lang/rust/blob/1.80.1/library/core/src/cmp.rs#L273) and `uuid` references it [here](https://github.com/uuid-rs/uuid/blob/1.8.0/src/lib.rs#L329). And what do you know, they're both at offset 10083! In our large monorepo, this happens for basically every common stdlib type!
feat: Create an assist to convert closure to freestanding fn
The assist converts all captures to parameters.
Closes#17920.
This was more work than I though, since it has to handle a bunch of edge cases...
Based on #17941. Needs to merge it first.
internal: Avoid newlines in fetch errors
Most logs lines don't have newlines, ensure fetch errors follow this pattern. This makes it easier to see which log line is associated with the error.
Before:
2024-08-28T21:11:58.431856Z ERROR FetchWorkspaceError:
rust-analyzer failed to discover workspace
After:
2024-08-28T21:11:58.431856Z ERROR FetchWorkspaceError: rust-analyzer failed to discover workspace
I.e. don't generate `let var_name = &foo()`.
Anything that creates a new value don't need a reference. That excludes mostly field accesses and indexing.
I had a thought that we can also not generate a reference for fields and indexing as long as the type is `Copy`, but sometimes people impl `Copy` even when they don't want to copy the values (e.g. a large type), so I didn't do that.
Expand proc-macros in workspace root, not package root
Should fix https://github.com/rust-lang/rust-analyzer/issues/17748. The approach is generally not perfect though as rust-project.json projects don't benefit from this (still, nothing changes in that regard)
fix: Fix "Unwrap block" assist with block modifiers
The assist just assumes the `{` will be the first character, which led to strange outputs such as `nsafe {`.
Fixes#17964.
Always show error lifetime arguments as `'_`
Fixes#17947
Changed error lifetime argument presentation in non-test environment to `'_` and now showing them even if all of args are error lifetimes.
This also influenced some of the other tests like `extract_function.rs`, `predicate.rs` and `type_pos.rs`. Not sure whether I need to refrain from adding lifetimes args there. Happy to fix if needed
fix: add extra_test_bin_args to test explorer test runner
`@HKalbasi` I thought I included this in #17470 but it appears not so I have created a new issue #17959 for this fix.
In most places where we set a search scope it is a single file, and so the fast path will actually harm performance, since it has to search for aliases in the whole project.
The only exception that qualifies for the fast path is SSR (there is an exception that don't qualify for the fast path as it search for `use` items). It sets the search scope to avoid dependencies. We could make it use the fast path, but I didn't bother.
perf: Speed up search for short associated functions, especially very common identifiers such as `new`
`@Veykril` said in https://github.com/rust-lang/rust-analyzer/pull/17908#issuecomment-2292958068 that people complain searches for `new()` are slow (they are right), so here I am to help!
The search is used by IDE features such as rename and find all references.
The search is slow because we need to verify each candidate, and that requires analyzing it; the key to speeding it up is to avoid the analysis where possible.
I did that with a bunch of tricks that exploits knowledge about the language and its possibilities. The first key insight is that associated methods may only be referenced in the form `ContainerName::func_name` (parentheses are not necessary!) (Rust doesn't include a way to `use Container::func_name`, and even if it will in the future most usages are likely to stay in that form.
Searching for `::` will help only a bit, but searching for `Container` can help considerably, since it is very rare that there will be two identical instances of both a container and a method of it.
However, things are not as simple as they sound. In Rust a container can be aliased in multiple ways, and even aliased from different files/modules. If we will try to resolve the alias, we will lose any gain from the textual search (although very common method names such as `new` will still benefit, most will suffer because there are more instances of a container name than its associated item).
This is where the key trick enters the picture. The key insight is that there is still a textual property: a container namer cannot be aliased, unless its name is mentioned in the alias declaration, or a name of alias of it is mentioned in the alias declaration.
This becomes a fixpoint algorithm: we expand our list of aliases as we collect more and more (possible) aliases, until we eventually reach a fixpoint. A fixpoint is not guaranteed (and we do have guards for the rare cases where it does not happen), but it is almost so: most types have very few aliases, if at all.
We do use some semantic information while analyzing aliases. It's a balance: too much semantic analysis, and the search will become slow. But too few of it, and we will bring many incorrect aliases to our list, and risk it expands and expands and never reach a fixpoint. At the end, based on benchmarks, it seems worth to do a lot to avoid adding an alias (but not too much), while it is worth to do a lot to avoid the need to semantically analyze func_name matches (but again, not too much).
After we collected our list of aliases, we filter matches based on this list. Only if a match can be real, we do semantic analysis for it.
The results are promising: searching for all references on `new()` in `base-db` in the rust-analyzer repository, which previously took around 60 seconds, now takes as least as two seconds and a half (roughly), while searching for `Vec::new()`, almost an upper bound to how much a symbol can be used, that used to take 7-9 minutes(!) now completes in 100-120 seconds, and with less than half of non-verified results (aka. false positives).
This is the less strictly correct (but faster) branch of this patch; it can miss some (rare) cases (there is a test for that - `goto_ref_on_short_associated_function_complicated_type_magic_can_confuse_our_logic()`). There is another branch that have no false negatives but is slower to search (`Vec::new()` never reaches a fixpoint in aliases collection there). I believe it is possible to create a strategy that will have the best of both worlds, but it will involve significant complexity and I didn't bother, especially considering that in the vast majority of the searches the other branch will be more than enough. But all in all, I decided to bring this branch (of course if the maintainers will agree), since our search is already not 100% accurate (it misses macros), and I believe there is value in the additional perf.
You can find the strict branch at https://github.com/ChayimFriedman2/rust-analyzer/tree/speedup-new-usages-strict.
Should fix#7404, I guess (will check now).
fix: run flycheck without rev_deps when target is specified
Since querying for a crate's target is a call to salsa and therefore blocking, flycheck task is now deferred out of main thread by using `GlobalState`s `deferred_task_queue`. Fixes#17829 and https://github.com/rust-lang/rustlings/issues/2071
The search is used by IDE features such as rename and find all references.
The search is slow because we need to verify each candidate, and that requires analyzing it; the key to speeding it up is to avoid the analysis where possible.
I did that with a bunch of tricks that exploits knowledge about the language and its possibilities. The first key insight is that associated methods may only be referenced in the form `ContainerName::func_name` (parentheses are not necessary!) (Rust doesn't include a way to `use Container::func_name`, and even if it will in the future most usages are likely to stay in that form.
Searching for `::` will help only a bit, but searching for `Container` can help considerably, since it is very rare that there will be two identical instances of both a container and a method of it.
However, things are not as simple as they sound. In Rust a container can be aliased in multiple ways, and even aliased from different files/modules. If we will try to resolve the alias, we will lose any gain from the textual search (although very common method names such as `new` will still benefit, most will suffer because there are more instances of a container name than its associated item).
This is where the key trick enters the picture. The key insight is that there is still a textual property: a container namer cannot be aliased, unless its name is mentioned in the alias declaration, or a name of alias of it is mentioned in the alias declaration.
This becomes a fixpoint algorithm: we expand our list of aliases as we collect more and more (possible) aliases, until we eventually reach a fixpoint. A fixpoint is not guaranteed (and we do have guards for the rare cases where it does not happen), but it is almost so: most types have very few aliases, if at all.
We do use some semantic information while analyzing aliases. It's a balance: too much semantic analysis, and the search will become slow. But too few of it, and we will bring many incorrect aliases to our list, and risk it expands and expands and never reach a fixpoint. At the end, based on benchmarks, it seems worth to do a lot to avoid adding an alias (but not too much), while it is worth to do a lot to avoid the need to semantically analyze func_name matches (but again, not too much).
After we collected our list of aliases, we filter matches based on this list. Only if a match can be real, we do semantic analysis for it.
The results are promising: searching for all references on `new()` in `base-db` in the rust-analyzer repository, which previously took around 60 seconds, now takes as least as two seconds and a half (roughly), while searching for `Vec::new()`, almost an upper bound to how much a symbol can be used, that used to take 7-9 minutes(!) now completes in 100-120 seconds, and with less than half of non-verified results (aka. false positives).
This is the less strictly correct (but faster) of this patch; it can miss some (rare) cases (there is a test for that - `goto_ref_on_short_associated_function_complicated_type_magic_can_confuse_our_logic()`). There is another branch that have no false negatives but is slower to search (`Vec::new()` never reaches a fixpoint in aliases collection there). I believe it is possible to create a strategy that will have the best of both worlds, but it will involve significant complexity and I didn't bother, especially considering that in the vast majority of the searches the other branch will be more than enough. But all in all, I decided to bring this branch (of course if the maintainers will agree), since our search is already not 100% accurate (it misses macros), and I believe there is value in the additional perf.
This avoids the need to analyze the file when we are not inside a macro call.
This is especially important for the optimization in the next commit(s), as there the common case will be to descent into macros but then not analyze.
Remove the ability to configure the user config path
Being able to do this makes little sense as this is effectively a cyclic dependency (and we do not want to fixpoint this really).
Priming caches is a performance win, but it takes a lock on the salsa
database and prevents rust-analyzer from responding to e.g. go-to-def
requests.
This causes confusion for users, who see the spinner next to
rust-analyzer in the VS Code footer stop, so they start attempting to
navigate their code.
Instead, set the `quiescent` status in LSP to false during cache
priming, so the VS Code spinner persists until we can respond to any
LSP request.
chore(config): remove `invocationLocation` in favor of `invocationStrategy`
These flags were added to help rust-analyzer integrate with repos requiring non-Cargo invocations. The consensus is that having two independent settings are no longer needed. This change removes `invocationLocation` in favor of `invocationStrategy` and changes the internal representation of `InvocationStrategy::Once` to hold the workspace root.
Closes#17848.
These flags were added to help rust-analyzer integrate with repos
requiring non-Cargo invocations. The consensus is that having two
independent settings are no longer needed. This change removes
`invocationLocation` in favor of `invocationStrategy` and changes
the internal representation of `InvocationStrategy::Once` to hold
the workspace root.
fix: Wrong BoundVar index when lowering impl trait parameter of parent generics
Fixes#17711
From the following test code;
```rust
//- minicore: deref
use core::ops::Deref;
struct Struct<'a, T>(&'a T);
trait Trait {}
impl<'a, T: Deref<Target = impl Trait>> Struct<'a, T> {
fn foo(&self) -> &Self { self }
fn bar(&self) {
let _ = self.foo();
}
}
```
when we call `register_obligations_for_call` for `let _ = self.foo();`,
07659783fd/crates/hir-ty/src/infer/expr.rs (L1939-L1952)
we are querying `generic_predicates` and it has `T: Deref<Target = impl Trait>` predicate from the parent `impl Struct`;
07659783fd/crates/hir-ty/src/lower.rs (L375-L399)
but as we can see above, lowering `TypeRef = impl Trait` doesn't take into account the parent generic parameters, so the `BoundVar` index here is `0`, as `fn foo` has no generic args other than parent's,
But this `BoundVar` is pointing at `'a` in `<'a, T: Deref<Target = impl Trait>>`.
So, in the first code reference `register_obligations_for_call`'s L:1948 - `.substitute(Interner, parameters)`, we are substituting `'a` with `Ty`, not `Lifetime` and this makes panic inside the chalk.
This PR fixes this wrong `BoundVar` index in such cases
Add scip/lsif flag to exclude vendored libaries
#17809 changed StaticIndex to include vendored libraries. This PR adds a flag to disable that behavior.
At work, our monorepo has too many rust targets to index all at once, so we split them up into several shards. Since all of our libraries are vendored, if rust-analyzer includes them, sharding no longer has much benefit, because every shard will have to index the entire transitive dependency graphs of all of its targets. We get around the issue presented in #17809 because some other shard will index the libraries directly.
