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

Add span to group. #14960

Merged
merged 2 commits into from Jun 10, 2023
Merged

Add span to group. #14960

merged 2 commits into from Jun 10, 2023

Conversation

jneem
Copy link
Contributor

@jneem jneem commented Jun 3, 2023

This appears to fix #14959, but I've never contributed to rust-analyzer before and there were some things that confused me:

  • I had to add the fn byte_range method to get it to build. This was added to rust in April, so I don't understand why it wasn't needed until now
  • When testing, I ran into the fact that rust recently updated its METADATA_VERSION, so I had to test this with nightly-2023-05-20. But then I noticed that rust has its own copy of rust-analyzer, and the metadata version bump has already been handled there. So I guess I don't really understand the relationship between the code there and the code here.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2023
@jneem jneem marked this pull request as draft June 3, 2023 21:29
@Veykril
Copy link
Member

Veykril commented Jun 3, 2023

This will have to wait for the the next subtree sync, that is also the reason why you ran into those weird problems

@jneem
Copy link
Contributor Author

jneem commented Jun 4, 2023

Ah, makes sense. Would it be better to just submit this in rust instead?

@Veykril
Copy link
Member

Veykril commented Jun 4, 2023

No, we want as little changes in the upstream subtree as possible. We are just lacking behind with the syncs atm because of problems we had at our end. We might be able to sync next week again, then we can merge this.

@lnicola
Copy link
Member

lnicola commented Jun 6, 2023

Subtree sync was done yesterday, can you rebase?

@Veykril
Copy link
Member

Veykril commented Jun 6, 2023

(wanna check if #14994 (comment) works now since this PR should trigger it)
@bors try

@bors
Copy link
Collaborator

bors commented Jun 6, 2023

⌛ Trying commit d013153 with merge 9e29fed...

bors added a commit that referenced this pull request Jun 6, 2023
Add span to group.

This appears to fix #14959, but I've never contributed to rust-analyzer before and there were some things that confused me:

- I had to add the `fn byte_range` method to get it to build. This was added to rust in [April](rust-lang/rust#109002), so I don't understand why it wasn't needed until now
- When testing, I ran into the fact that rust recently updated its `METADATA_VERSION`, so I had to test this with nightly-2023-05-20. But then I noticed that rust has its own copy of `rust-analyzer`, and the metadata version bump has already been [handled there](rust-lang/rust@60e95e7). So I guess I don't really understand the relationship between the code there and the code here.
@bors
Copy link
Collaborator

bors commented Jun 6, 2023

💔 Test failed - checks-actions

@jneem jneem marked this pull request as ready for review June 6, 2023 22:43
@jneem
Copy link
Contributor Author

jneem commented Jun 6, 2023

Ok, rebased and I also added a flag for overriding the proc-macro-srv binary, without which these changes are hard to test.

@Veykril
Copy link
Member

Veykril commented Jun 7, 2023

Turns out one of the tests that never got executed wasnt updated #14995

@jneem
Copy link
Contributor Author

jneem commented Jun 7, 2023

It looks like the CI failure is caused by nightly distributing a broken rust-analyzer-proc-macro-srv. On my system, I get

> rustup update nightly
...
> RUST_ANALYZER_INTERNALS_DO_NOT_USE='this is unstable' /home/jneeman/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/libexec/rust-analyzer-proc-macro-srv
thread 'main' panicked at 'proc-macro-srv-cli requires the `sysroot-abi` feature to be enabled', crates/proc-macro-srv-cli/src/main.rs:23:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@lnicola
Copy link
Member

lnicola commented Jun 7, 2023

#14991

@Veykril
Copy link
Member

Veykril commented Jun 9, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 9, 2023

📌 Commit ad2a0d1 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 9, 2023

⌛ Testing commit ad2a0d1 with merge fb238ef...

bors added a commit that referenced this pull request Jun 9, 2023
Add span to group.

This appears to fix #14959, but I've never contributed to rust-analyzer before and there were some things that confused me:

- I had to add the `fn byte_range` method to get it to build. This was added to rust in [April](rust-lang/rust#109002), so I don't understand why it wasn't needed until now
- When testing, I ran into the fact that rust recently updated its `METADATA_VERSION`, so I had to test this with nightly-2023-05-20. But then I noticed that rust has its own copy of `rust-analyzer`, and the metadata version bump has already been [handled there](rust-lang/rust@60e95e7). So I guess I don't really understand the relationship between the code there and the code here.
@bors
Copy link
Collaborator

bors commented Jun 9, 2023

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Jun 9, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 9, 2023

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Collaborator

bors commented Jun 9, 2023

📌 Commit ad2a0d1 has been approved by Veykril

It is now in the queue for this repository.

bors added a commit that referenced this pull request Jun 9, 2023
Add span to group.

This appears to fix #14959, but I've never contributed to rust-analyzer before and there were some things that confused me:

- I had to add the `fn byte_range` method to get it to build. This was added to rust in [April](rust-lang/rust#109002), so I don't understand why it wasn't needed until now
- When testing, I ran into the fact that rust recently updated its `METADATA_VERSION`, so I had to test this with nightly-2023-05-20. But then I noticed that rust has its own copy of `rust-analyzer`, and the metadata version bump has already been [handled there](rust-lang/rust@60e95e7). So I guess I don't really understand the relationship between the code there and the code here.
@bors
Copy link
Collaborator

bors commented Jun 9, 2023

⌛ Testing commit ad2a0d1 with merge 8ecb6df...

@bors
Copy link
Collaborator

bors commented Jun 9, 2023

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Jun 10, 2023

(sorry for the spam, but this PR is special in that it triggers a nightly build which we are having problmes with right now)
@bors retry

@bors
Copy link
Collaborator

bors commented Jun 10, 2023

⌛ Testing commit ad2a0d1 with merge b33d860...

bors added a commit that referenced this pull request Jun 10, 2023
Add span to group.

This appears to fix #14959, but I've never contributed to rust-analyzer before and there were some things that confused me:

- I had to add the `fn byte_range` method to get it to build. This was added to rust in [April](rust-lang/rust#109002), so I don't understand why it wasn't needed until now
- When testing, I ran into the fact that rust recently updated its `METADATA_VERSION`, so I had to test this with nightly-2023-05-20. But then I noticed that rust has its own copy of `rust-analyzer`, and the metadata version bump has already been [handled there](rust-lang/rust@60e95e7). So I guess I don't really understand the relationship between the code there and the code here.
@bors
Copy link
Collaborator

bors commented Jun 10, 2023

💔 Test failed - checks-actions

@Veykril
Copy link
Member

Veykril commented Jun 10, 2023

@bors retry

@bors
Copy link
Collaborator

bors commented Jun 10, 2023

⌛ Testing commit ad2a0d1 with merge 489eeab...

@bors
Copy link
Collaborator

bors commented Jun 10, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 489eeab to master...

@bors bors merged commit 489eeab into rust-lang:master Jun 10, 2023
7 of 10 checks passed
bors added a commit that referenced this pull request Aug 1, 2023
… r=Veykril

Don't provide `add_missing_match_arms` assist when upmapping match arm list failed

Fixes #15310

We shouldn't provide the assist when we fail to find the original match arm list.

Note that this PR will temporarily make the assist not applicable when attribute macro operates on the match expression in question, just like the case in #15310, for most of the current stable toolchain users. This is because the sysroot-abi proc-macro-srv on the current stable [discards] spans for `Group` delimiters in some code paths, which the popular `proc-macro2` crate almost always calls, and it makes the identity of match arm list's brackets lost, leading to the upmapping failure. This has been fixed by #14960, which will land in the next stable, 1.71.

[discards]: https://github.com/rust-lang/rust/blob/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/src/tools/rust-analyzer/crates/proc-macro-srv/src/abis/abi_sysroot/ra_server.rs#L231
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.

Group spans don't round-trip when run under rust-analyzer
5 participants