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 #108260

Merged
merged 55 commits into from
Feb 20, 2023
Merged

⬆️ rust-analyzer #108260

merged 55 commits into from
Feb 20, 2023

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Feb 20, 2023

r? @ghost

albertlarsan68 and others added 30 commits January 11, 2023 09:32
Discard postfix completion if the next_non_trivia_sibling of dot_token
is an ELSE_KW.
Looks like this is a native encoding for Emacs at least!
…indivisble-expr, r=Veykril

fix: Don't trigger postfix completion in `if` block which has an `else` block

Fix rust-lang#14096
…verflow, r=Veykril

fix: Don't expand macros in the same expansion tree after overflow

This patch fixes 2 bugs:

- In `Expander::enter_expand_id()` (and in code paths it's called), we never check whether we've reached the recursion limit. Although it hasn't been reported as far as I'm aware, this may cause hangs or stack overflows if some malformed attribute macro is used on associated items.
- We keep expansion even when recursion limit is reached. Take the following for example:

  ```rust
  macro_rules! foo { () => {{ foo!(); foo!(); }} }
  fn main() { foo!(); }
  ```

  We keep expanding the first `foo!()` in each expansion and would reach the limit at some point, *after which* we would try expanding the second `foo!()` in each expansion until it hits the limit again. This will (by default) lead to ~2^128 expansions.

  This is essentially what's happening in rust-lang#14074. Unlike rustc, we don't just stop expanding macros when we fail as long as it produces some tokens so that we can provide completions and other services in incomplete macro calls.

This patch provides a method that takes care of recursion depths (`Expander::within_limit()`) and stops macro expansions in the whole macro expansion tree once it detects recursion depth overflow. To be honest, I'm not really satisfied with this fix because it can still be used in unintended ways to bypass overflow checks, and I'm still seeking ways such that misuses are caught by the compiler by leveraging types or something.

Fixes rust-lang#14074
Co-authored-by: Stig Brautaset <stig@brautaset.org>
Support UTF-32 position encoding

Looks like this is a native encoding for Emacs at least!
…, r=Veykril

fix: Search raw identifiers without prefix

When we find references/usages of a raw identifier, we should disregard `r#` prefix because there are keywords one can use without the prefix in earlier editions (see rust-lang#13034; this bug is actually fallout from the PR). `name`, the text we're searching for, has already been stripped of the prefix, but the text of nodes we compare it to hasn't been.

The second commit is strictly refactoring, I can remove it if it's not much of value.
internal: Improve parser recovery for delimited lists

Closes rust-lang/rust-analyzer#11188, rust-lang/rust-analyzer#10410, rust-lang/rust-analyzer#10173

Should probably be merged after the stable release as this might get the parser stuck if I missed something
Don't assume VSCode internal commands in the server
internal: Don't allocate the generic_args vec in `hir_def::Path` if it consists only of `None` args

Saves roughly 5mb on self
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 20, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

Some changes occurred in src/tools/rust-analyzer

cc @rust-lang/wg-rls-2

@lnicola lnicola marked this pull request as draft February 20, 2023 08:16
@lnicola
Copy link
Member Author

lnicola commented Feb 20, 2023

@fasterthanlime umm, bit of help here please, do you know why this PR (as produced by git subtree pull) contains older commits? The last one was #107992, followed by a merge to upstream in rust-lang/rust-analyzer#14143.

@fasterthanlime
Copy link
Contributor

fasterthanlime commented Feb 20, 2023

@fasterthanlime umm, bit of help here please, do you know why this PR (as produced by git subtree pull) contains older commits? The last one was #107992, followed by a merge to upstream in rust-lang/rust-analyzer#14143.

Off the top of my head (and from my phone) I'm not sure. That one is the only older commit (Jan 11), right? 3e0e51c - Everything else is from a month later at least, afaict.

It corresponds to rust-lang/rust-analyzer@3e0e51c in RA, which seems to have been introduced with rust-lang/rust-analyzer#14143 — I'm not sure how it ended up there, but it seems git-subtree is doing the right thing?

edit: I'm still confused after checking it out a little more (still on mobile). That commit is already in rust as 40ba0e8 - @eddyb is it possible that's the one commit we messed up when manually making the mega-sync?

@fasterthanlime
Copy link
Contributor

@lnicola so I can't really explain what happened there unfortunately. However, I'm curious: if you do another rust->ra sync after this PR (you can pretend to do that without actually merging the PR through bors/GitHub's UI), does the next ra->rust sync also still contain this commit?

@lnicola
Copy link
Member Author

lnicola commented Feb 20, 2023

This is how I tested:

# in the RA repo
$ git branch -D sync-from-rust

# in the Rust repo
$ git checkout rust-analyzer-2023-02-20
$ git subtree push -P src/tools/rust-analyzer rust-analyzer-local sync-from-rust
$ git checkout -b ra-new-sync
$ git subtree pull -P src/tools/rust-analyzer rust-analyzer-local sync-from-rust
$ git log --format=oneline
04c4eda971e945fa533880eb14fc6db314d8ed3e (HEAD -> ra-new-sync) Merge commit '7e711da2f07778d62f6411de5da520f1e260d761' into ra-new-sync
03288ebba35defc807952e6e55a0ab8f5f77aa83 (origin/rust-analyzer-2023-02-20, rust-analyzer-2023-02-20) :arrow_up: rust-analyzer
7e711da2f07778d62f6411de5da520f1e260d761 (rust-analyzer-local/sync-from-rust) :arrow_up: rust-analyzer
824f915cbc32c0942122389274a1b6fbe2ffc51e (upstream/master, upstream/HEAD, master) Auto merge of #108235 - tmiasko:read-buf, r=the8472
[snip]

@fasterthanlime
Copy link
Contributor

This is how I tested:

Okay well, it doesn't seem to show up again.

Here's what I don't understand about the current situation:

  • There's a commit with the same hash in both RA and this PR: having "the same commit" show up is by design, but they should affect different paths, and thus have different hashes
  • Said commit in that PR does not touch src/tools/rust-analyzer/crates, it touches crates directly, you can tell by looking at this PR's commit

These paths don't show up in the Files changed tab of the PR, so I'm really not sure what's going on.

@lnicola
Copy link
Member Author

lnicola commented Feb 20, 2023

Given that my test above went fine, should we merge it like this?

@fasterthanlime
Copy link
Contributor

Given that my test above went fine, should we merge it like this?

The test working makes me slightly more confident in this, ideally I'd love to have an extra pair of eyes on this, I've pinged a couple folks and will keep you posted

@fasterthanlime
Copy link
Contributor

The test working makes me slightly more confident in this, ideally I'd love to have an extra pair of eyes on this, I've pinged a couple folks and will keep you posted

I haven't heard back yet and don't want to block this, you got my 👍

@workingjubilee
Copy link
Member

Based on my subtree experiences, I think it's fine. Sometimes a commit gets confusingly replayed on the other side of the subtree so git can't quite be sure if it's new or not. If it's only one in a hundred commits or so, it's annoying but not unexpected.

@lnicola lnicola marked this pull request as ready for review February 20, 2023 18:49
@lnicola
Copy link
Member Author

lnicola commented Feb 20, 2023

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Feb 20, 2023

📌 Commit 03288eb has been approved by lnicola

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2023
@bors
Copy link
Contributor

bors commented Feb 20, 2023

⌛ Testing commit 03288eb with merge 5243ea5...

@bors
Copy link
Contributor

bors commented Feb 20, 2023

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing 5243ea5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2023
@bors bors merged commit 5243ea5 into rust-lang:master Feb 20, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 20, 2023
@lnicola lnicola deleted the rust-analyzer-2023-02-20 branch February 20, 2023 22:09
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5243ea5): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.2% [-1.2%, -1.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.2% [-3.5%, -2.8%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.2% [-3.5%, -2.8%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.