Skip to content
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

⬆️ rust-analyzer 2022-11-28 #105834

Closed
wants to merge 101 commits into from

Conversation

arcnmx
Copy link
Contributor

@arcnmx arcnmx commented Dec 17, 2022

The prior update included checkOnSave multiple targets, but missed the fix for the regression it caused.

I performed a git subtree merge, not sure if the commit diff noise is normal? This also could be updated further, but I wanted to be conservative for now.

r? @lnicola

fasterthanlime and others added 30 commits July 26, 2022 11:53
This removes some RPC when creating and emitting diagnostics, and
simplifies the bridge slightly.

After this change, there are no remaining methods which take advantage
of the support for `&mut` references to objects in the store as
arguments, meaning that support for them could technically be removed if
we wanted. The only remaining uses of immutable references into the
store are `TokenStream` and `SourceFile`.
…adowing-another-type, r=estebank

Point at a type parameter shadowing another type

This patch fixes a part of rust-lang#97459.
proc_macro/bridge: send diagnostics over the bridge as a struct

This removes some RPC when creating and emitting diagnostics, and
simplifies the bridge slightly.

After this change, there are no remaining methods which take advantage
of the support for `&mut` references to objects in the store as
arguments, meaning that support for them could technically be removed if
we wanted. The only remaining uses of immutable references into the
store are `TokenStream` and `SourceFile`.

r? `@eddyb`
This fixes a typo first appearing in rust-lang#94624
in which test-macro diagnostic uses "a" article twice.

Since I searched sources for " a a " sequences,
I also fixed the same issue in a few source files where I found it.

Signed-off-by: Petr Portnov <gh@progrm-jarvis.ru>
initial josh subtree sync

This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did
```
git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master
git merge FETCH_HEAD
./rustup-toolchain HEAD && ./miri fmt
git commit -am rustup
```
Unlike the [previous attempt](rust-lang/miri#2554), this does not add a new root commit to the repo.

Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible).
It's easy to just use `unescape_literal` + `byte_from_char`.
…=matklad

Unescaping cleanups

Some code improvements, and some error message improvements.

Best reviewed one commit at a time.

r? ````@matklad````
…r=lnicola

⬆️ rust-analyzer

r? ``@ghost``
currently only works with struct pattern
fasterthanlime and others added 11 commits November 25, 2022 13:06
Mega-sync from `rust-lang/rust`

This essentially implements `@oli-obk's` suggestion here rust-lang/rust-analyzer#13459 (comment), with `@eddyb's` help.

This PR is equivalent to 14 syncs (back and forth) between `rust-lang/rust` and `rust-lang/rust-analyzer`.

Working from this list (from bottom to top):

```
(x) a2a1d99 ⬆️ rust-analyzer
(x) 79923c3 ⬆️ rust-analyzer
(x) c60b1f6 ⬆️ rust-analyzer
(x) 8807fc4 ⬆️ rust-analyzer
(x) a99a48e ⬆️ rust-analyzer
(x) 4f55ebb ⬆️ rust-analyzer
(x) f5fde4d ⬆️ rust-analyzer
(x) 459bbb4 ⬆️ rust-analyzer
(x) 65e1dc4 ⬆️ rust-analyzer
(x) 3e358a6 ⬆️ rust-analyzer
(x) 31519bb ⬆️ rust-analyzer
(x) 8231fee ⬆️ rust-analyzer
(x) 22c8c9c ⬆️ rust-analyzer
(x) 9d2cb42 ⬆️ rust-analyzer
```

(This listed was assembled by doing a `git subtree push`, which made a branch, and looking at the new commits in that branch, picking only those that were `⬆️ rust-analyzer` commits)

We used the following commands to simulate merges in both directions:

```shell
TO_MERGE=22c8c9c40 # taken from the list above, bottom to top
git merge --no-edit --no-ff $TO_MERGE
git merge --no-edit --no-ff $(git -C ../rust log --pretty=format:'%cN | %s | %ad => %P' | rg -m1 -F "$(git show --no-patch --pretty=format:%ad $TO_MERGE)" | tee /dev/stderr | rg '.* => \S+ (\S+)$' --replace '$1')
```

We encountered no merge conflicts that Git wasn't able to solve by doing it this way.

Here's what the commit graph looks like (as shown in the Git Lens VSCode extension):

<img width="1345" alt="image" src="https://user-images.githubusercontent.com/7998310/203984523-7c1a690a-8224-416c-8015-ed6e49667066.png">

This PR closes rust-lang#13459

## Does this unbreak `rust->ra` syncs?

Yes, here's how we tried:

In `rust-analyzer`:

  * check out `subtree-fix` (this PR's branch)
  * make a new branch off of it: `git checkout -b subtree-fix-merge-test`
  * simulate this PR getting merged with `git merge master`

In `rust`:

  * pull latest master
  * make a new branch: `git checkout -b test-change`
  * mess with rust-analyzer (I added a comment to `src/tools/rust-analyzer/Cargo.toml`)
  * commit
  * run `git subtree push -P src/tools/rust-analyzer ra-local final-sync` (this follows the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html))

This created a `final-sync` branch in `rust-analyzer`.

In `rust-analyzer`:

  * `git merge --no-ff final-sync` (this follows the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html))

Now `git log` in `rust-analyzer` shows this:

```
commit 460128387e46ddfc2b95921b2d7f6e913a3d2b9f (HEAD -> subtree-fix-merge-test)
Merge: 0513fc02a 9ce6a734f
Author: Amos Wenger <amoswenger@gmail.com>
Date:   Fri Nov 25 13:28:24 2022 +0100

    Merge branch 'final-sync' into subtree-fix-merge-test

commit 0513fc02a08ea9de952983624bd0a00e98044b36
Merge: 38c98d1 6918009
Author: Amos Wenger <amoswenger@gmail.com>
Date:   Fri Nov 25 13:28:02 2022 +0100

    Merge branch 'master' into subtree-fix-merge-test

commit 9ce6a734f37ef8e53689f1c6f427a9efafe846bd (final-sync)
Author: Amos Wenger <amoswenger@gmail.com>
Date:   Fri Nov 25 13:26:26 2022 +0100

    Mess with rust-analyzer just for fun
```

And `git diff 0513fc02a08ea9de952983624bd0a00e98044b36` shows this:

```patch
diff --git a/Cargo.toml b/Cargo.toml
index 286ef1e7d..c9e24cd19 100644
--- a/Cargo.toml
+++ b/Cargo.toml
`@@` -32,3 +32,5 `@@` debug = 0
 # ungrammar = { path = "../ungrammar" }

 # salsa = { path = "../salsa" }
+
+# lol, hi
```

## Does this unbreak `ra->rust` syncs?

Yes, here's how we tried.

From `rust`:

  * `git checkout -b sync-from-ra`
  * `git subtree pull -P src/tools/rust-analyzer ra-local subtree-fix-merge-test` (this is adapted from the [Clippy sync guide](https://doc.rust-lang.org/nightly/clippy/development/infrastructure/sync.html#performing-the-sync-from-clippy-to-rust-langrust), you would normally use `ra-upstream master` but we're simulating things here)

A commit editor pops up, there was no merge conflicts.

## How do we prevent this from happening again?

Like `@bjorn3` said in rust-lang/rust-analyzer#13459 (comment)

> Whenever syncing from rust-analyzer -> rust you have to immediately sync the merge commit from rust -> rust-analyzer to prevent merge conflicts in the future.

But if we get it wrong again, at least now we have a not-so-painful way to fix it.
Encode the variants of `HirFileId` in a u32 with MSB as the tag

This saves 10mb on `self` analysis, while this does limit us to 2billion real files and 2 billion macro expansions, I doubt we will ever hit that limit :) `HirFileId` is used a lot, so going from 8 bytes to 4 is a decent win.
…ue4u

fix: filter unnecessary completions after colon

close rust-lang#13597
related: rust-lang#10173

This PR also happens to fix two extra issues:

1. The test case in https://github.com/rust-lang/rust-analyzer/blob/master/crates/ide-completion/src/tests/attribute.rs#L778-L801 was never triggered in previous behavior.

after:

https://user-images.githubusercontent.com/26110087/201476995-56adf955-0fa7-4f75-ab32-28a8e6cb9504.mp4

<del>
2. completions were triggered even in invalid paths, like

```rust
fn main() {
    core:::::$0
}
```

```rust
#[:::::$0]
struct X;
```

</del>

only `:::` is excluded as discussed in rust-lang/rust-analyzer#13611 (comment)
…r=Veykril

fix: check tail expressions more precisely in `extract_function`

Fixes rust-lang#13620

When extracting expressions with control flows into a function, we can avoid wrapping tail expressions in `Option` or `Result` when they are also tail expressions of the container we're extracting from (see rust-lang#7840, rust-lang#9773). This is controlled by `ContainerInfo::is_in_tail`, but we've been computing it by checking if the tail expression of the range to extract is contained in the container's syntactically last expression, which may be a block that contains both tail and non-tail expressions (e.g. in rust-lang#13620, the range to be extracted is not a tail expression but we set the flag to true).

This PR tries to compute the flag as precise as possible by utilizing `for_each_tail_expr()` (and also moves the flag to `Function` struct as it's more of a property of the function to be extracted than of the container).
The prior update included checkOnSave multiple targets:
rust-lang/rust-analyzer#13290
but missed the fix for the regression it caused:
rust-lang/rust-analyzer#13661

Merge commit '6d61be8e65ac0fd45eaf178e1f7a1ec6b582de1f'
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

Failed to set assignee to lnicola: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/wg-rls-2

@lnicola
Copy link
Member

lnicola commented Dec 17, 2022

r? @fasterthanlime

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2022

Failed to set assignee to fasterthanlime: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@fasterthanlime
Copy link
Contributor

I performed a git subtree merge, not sure if the commit diff noise is normal? This also could be updated further, but I wanted to be conservative for now.

Could you specify exactly the commands you ran? These are easier to review than the result, for me at least x)

(But at first glance, this looks okay to me - I think we have to do another rust->ra sync right after this gets merged, right? cc @bjorn3)

@bjorn3
Copy link
Member

bjorn3 commented Dec 17, 2022

Yeah, doing a rust->ra sync immediately helps avoid conflicts on future syncs.

@lnicola
Copy link
Member

lnicola commented Dec 17, 2022

So I never quite got this, what does "immediately" mean here?

  • before another RA -> rust sync is done
  • before anything else gets merged to the rust repository
  • something else?

@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 17, 2022

Could you specify exactly the commands you ran? These are easier to review than the result, for me at least x)

git subtree --prefix=src/tools/rust-analyzer pull rust-analyzer 2022-11-28
git subtree --prefix=src/tools/rust-analyzer merge 2022-11-28 # alternatively (same result for either)

Auto-merging src/tools/rust-analyzer/crates/hir-def/src/nameres.rs
Merge made by the 'ort' strategy.

src/tools/rust-analyzer/crates/flycheck/src/lib.rs | 13 +-
.../hir-def/src/macro_expansion_tests/builtin_fn_macro.rs | 6 +-
.../crates/hir-expand/src/builtin_fn_macro.rs | 13 +-
src/tools/rust-analyzer/crates/hir-expand/src/db.rs | 2 +-
src/tools/rust-analyzer/crates/hir-expand/src/hygiene.rs | 8 +-
src/tools/rust-analyzer/crates/hir-expand/src/lib.rs | 227 ++++++-------
.../ide-assists/src/handlers/add_missing_impl_members.rs | 99 ++++++
.../src/handlers/convert_tuple_struct_to_named_struct.rs | 43 ++-
.../crates/ide-assists/src/handlers/extract_function.rs | 210 ++++++++++--
.../crates/ide-assists/src/handlers/generate_impl.rs | 411 +++++++++++++++++++-----
src/tools/rust-analyzer/crates/ide-assists/src/lib.rs | 1 +
.../crates/ide-assists/src/tests/generated.rs | 25 +-
.../rust-analyzer/crates/ide-completion/src/context.rs | 28 +-
src/tools/rust-analyzer/crates/ide-completion/src/lib.rs | 1 -
.../crates/ide-completion/src/tests/attribute.rs | 24 ++
.../crates/ide-completion/src/tests/special.rs | 90 +++++-
.../ide-db/src/test_data/test_symbol_index_collection.txt | 134 ++------
.../rust-analyzer/crates/ide/src/goto_declaration.rs | 123 ++++++-
src/tools/rust-analyzer/crates/ide/src/hover.rs | 101 +++---
src/tools/rust-analyzer/crates/ide/src/hover/render.rs | 48 ++-
src/tools/rust-analyzer/crates/ide/src/hover/tests.rs | 35 ++
.../rust-analyzer/crates/project-model/src/workspace.rs | 58 ++--
.../rust-analyzer/crates/rust-analyzer/src/config.rs | 20 +-
.../rust-analyzer/crates/rust-analyzer/src/handlers.rs | 14 +-
.../rust-analyzer/crates/rust-analyzer/src/reload.rs | 6 +-
src/tools/rust-analyzer/docs/user/generated_config.adoc | 2 +-
src/tools/rust-analyzer/editors/code/package.json | 10 +-
27 files changed, 1306 insertions(+), 446 deletions(-)

@bjorn3
Copy link
Member

bjorn3 commented Dec 17, 2022

So I never quite got this, what does "immediately" mean here?

Before a change on the rust-analyzer side is done that changes the same code as was synced here I believe. I believe the issue is that if this sync back isn't done that the merge commit created here by git subtree will conflict with the changes you make in some cases. Personally for cg_clif I sync back before making any change to cg_clif just to be safe: https://github.com/bjorn3/rustc_codegen_cranelift/blob/8b478012086f4df5c21e4ec0016631fac163133f/scripts/rustup.sh#L30-L43

@fasterthanlime
Copy link
Contributor

> Auto-merging src/tools/rust-analyzer/crates/hir-def/src/nameres.rs

I'm not sure why that one's happening.


Re:

This also could be updated further, but I wanted to be conservative for now.

I think updating to the latest RA (rather than one from 3 weeks ago) is preferable here, due to the extra sync required in the other direction to avoid merge conflicts further down the line.

By picking an earlier version we're just doing an extra ra->rust, rust->ra sync round, as far as I can tell 🤷‍♀️

@arcnmx arcnmx mentioned this pull request Dec 17, 2022
@arcnmx
Copy link
Contributor Author

arcnmx commented Dec 17, 2022

I think updating to the latest RA (rather than one from 3 weeks ago) is preferable here, due to the extra sync required in the other direction to avoid merge conflicts further down the line.

Fair enough, this was just closer to what was already in-tree.

git subtree --prefix=src/tools/rust-analyzer pull rust-analyzer 2022-12-12

Auto-merging src/tools/rust-analyzer/crates/hir-def/src/nameres.rs
Auto-merging src/tools/rust-analyzer/crates/hir-ty/src/infer.rs
Auto-merging src/tools/rust-analyzer/crates/hir-ty/src/method_resolution.rs
CONFLICT (content): Merge conflict in src/tools/rust-analyzer/crates/hir-ty/src/method_resolution.rs
Auto-merging src/tools/rust-analyzer/crates/proc-macro-srv/src/abis/abi_sysroot/ra_server.rs
Automatic merge failed; fix conflicts and then commit the result.

Submitted as #105855, feel free to pick whichever works better!

@lnicola
Copy link
Member

lnicola commented Dec 18, 2022

Before a change on the rust-analyzer side is done that changes the same code as was synced here I believe. I believe the issue is that if this sync back isn't done that the merge commit created here by git subtree will conflict with the changes you make in some cases.

But that means we have to make sure we don't merge such a change in RA before both syncs are done, right? And it can take a couple of days to get something merged to rust-lang/rust.

@bjorn3
Copy link
Member

bjorn3 commented Dec 18, 2022

Clippy syncs often get p=1. With that bors generally merges it the same day.

@fasterthanlime
Copy link
Contributor

Clippy syncs often get p=1. With that bors generally merges it the same day.

Assuming most of the actual work on RA gets done during the week, saturday could be an okay day for a ra->rust then rust->ra sync?

@arcnmx arcnmx closed this Jan 9, 2023
@arcnmx arcnmx deleted the rust-analyzer-2022-11-28 branch January 9, 2023 18:46
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2023
⬆️ rust-analyzer

Updates rust-analyzer to rust-lang/rust-analyzer@368e0bb

This is a continuation/replacement of rust-lang#105834, close that if this is chosen instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.