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

Subtree update of rust-analyzer #120447

Merged
merged 38 commits into from
Feb 2, 2024
Merged

Subtree update of rust-analyzer #120447

merged 38 commits into from
Feb 2, 2024

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jan 28, 2024

r? ghost

nnethercote and others added 30 commits January 12, 2024 16:19
By making it an `EscapeError` instead of a `LitError`. This makes it
like the other errors produced when checking string literals contents,
e.g. for invalid escape sequences or bare CR chars.

NOTE: this means these errors are issued earlier, before expansion,
which changes behaviour. It will be possible to move the check back to
the later point if desired. If that happens, it's likely that all the
string literal contents checks will be delayed together.

One nice thing about this: the old approach had some code in
`report_lit_error` to calculate the span of the nul char from a range.
This code used a hardwired `+2` to account for the `c"` at the start of
a C string literal, but this should have changed to a `+3` for raw C
string literals to account for the `cr"`, which meant that the caret in
`cr"` nul error messages was one short of where it should have been. The
new approach doesn't need any of this and avoids the off-by-one error.
Starting from cargo#13311, Cargo's compiler artifact message
uses Package ID specification as package's identifier format.
…etrochenkov

Detect `NulInCStr` error earlier.

By making it an `EscapeError` instead of a `LitError`. This makes it like the other errors produced when checking string literals contents, e.g. for invalid escape sequences or bare CR chars.

NOTE: this means these errors are issued earlier, before expansion, which changes behaviour. It will be possible to move the check back to the later point if desired. If that happens, it's likely that all the string literal contents checks will be delayed together.

One nice thing about this: the old approach had some code in `report_lit_error` to calculate the span of the nul char from a range. This code used a hardwired `+2` to account for the `c"` at the start of a C string literal, but this should have changed to a `+3` for raw C string literals to account for the `cr"`, which meant that the caret in `cr"` nul error messages was one short of where it should have been. The new approach doesn't need any of this and avoids the off-by-one error.

r? ```@fee1-dead```
…acrum

fix(rust-analyzer): use new pkgid spec to compare

Starting from rust-lang/cargo#13311, Cargo's compiler artifact message
uses Package ID specification as package's identifier format.

Zulip topic: https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/proc-macro-test.20bootstrap.20and.20pkgid.20JSON

cc `@ehuss`
Currently there is no proper way to get a target FileRange for a given
Definition.
…is, r=Veykril

Use upstream exhaustiveness checker!

Because it has been librarified!

The extra `Apache-2.0 WITH LLVM-exception` license is for `rustc_apfloat`. Also this duplicates `rustc_index` because the other upstream deps are still on an earlier version. They should be bumpable now though. Good thing is that we don't need this new crate to be synchronized with the others, which will make our lives easier.
minor: Bump rustc crates

This also reverts the earlier revert.
…Veykril

internal: Make TryToNav trait public

Currently there is no proper way to get a target FileRange for a given Definition.
…acro, r=Veykril

Replaced adjusted_display_range with adjusted_display_range_new in mismatched_arg_count

For detailed description - see related issue.

Fixes: rust-lang#16407
@lnicola
Copy link
Member Author

lnicola commented Feb 1, 2024

There's nobody assigned to the PR (since you requested a "ghost" review) so nobody else will look at it either.

Oh, I thought the CCed people would look at it.

FWIW, in my experience doing these syncs for Miri, it helps a lot to automate as much as possible and do them more often.

We do it every week(-ish) as well, but recently ran into a lot of bootstrap and lint related issues, like the "unknown cfg" lint, and stuff related to linking to rustc_driver; some of them wouldn't even show up on the PR CI, just on bors.

The other thing is that git subtree seems to report more conflicts than expected, even ones I've fixed before. There was one that showed up on every sync for I don't know, 6 months or maybe a year. It was auto-fixable, fortunately, and I think it went away recently when Veykril removed the code.

We were also stopping merges on the rust-analyzer side until the sync back (rust -> rust-analyzer), because if the rust-analyzer repository changed, we'd get more conflicts. What helped here was doing the sync back only up to the release commit, not up to master. So instead of:

  1. merge rust-analyzer release into rustc master
  2. fetch rustc master
  3. subtree rustc master into a rust-analyzer branch
  4. merge rust-analyzer master into the new branch
  5. merge the new branch into rust-analyzer

This works much better:

  1. merge rust-analyzer release into rustc master
  2. fetch rustc master
  3. subtree rustc master into a rust-analyzer branch
  4. merge rust-analyzer release into the new branch
  5. merge rust-analyzer master into the new branch
  6. merge the new branch into rust-analyzer

For the other direction we have a CI cronjob that creates a PR automatically whenever Miri no longer builds against rustc master.

We always sync both ways, not only when it stops building, because of rust-lang/rust-analyzer#13676.

I think you're using josh now, maybe that one is more resilient, I haven't tried it yet.

@RalfJung
Copy link
Member

RalfJung commented Feb 1, 2024

Ouch. :( Okay that sounds quite painful. Not sure why it's so much worse for RA than for Miri. Do you have a lot of conflicts? We do syncs in the two directions largely independent of each other and that works fine; that seems to be a subtree vs josh-proxy issue. But if the "sync both ways" can be automated that should not in itself be such a big deal I would expect. But maybe I am underestimating this, having to coordinate multiple branches does sound like significant overhead.

The git master version of josh is pretty reliable, yeah. (The latest released version still has an issue that can sometimes be blocking.) Happy to talk more if you want to explore switching RA to josh (but probably best to start a new thread e.g. on Zulip of that rather than discussing it here).

@lnicola
Copy link
Member Author

lnicola commented Feb 1, 2024

Do you have a lot of conflicts?

We used to freeze master during the syncs, but that wasn't strictly enforced and some of them took 2-3 days because of other PRs in the queue (that is, ignoring the CI/bors failures). So not a lot of conflicts in practice, but before my two-step merge trick, every "real" conflict would show up again on every further change to the same file up to master.

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Feb 1, 2024

⌛ Testing commit 7159d88 with merge bf3c6c5...

@bors
Copy link
Contributor

bors commented Feb 2, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2024
@bors bors merged commit bf3c6c5 into rust-lang:master Feb 2, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Feb 2, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bf3c6c5): 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)
-2.7% [-2.7%, -2.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.7% [-2.7%, -2.7%] 1

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)
2.2% [1.4%, 3.9%] 14
Regressions ❌
(secondary)
2.7% [0.8%, 4.1%] 33
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.2% [1.4%, 3.9%] 14

Cycles

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)
8.3% [6.7%, 9.3%] 3
Improvements ✅
(primary)
-2.6% [-2.6%, -2.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.6% [-2.6%, -2.6%] 1

Binary size

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

Bootstrap: 662.648s -> 662.752s (0.02%)
Artifact size: 308.09 MiB -> 308.09 MiB (-0.00%)

@lnicola lnicola deleted the sync-from-ra branch February 2, 2024 06:49
@lnicola
Copy link
Member Author

lnicola commented Feb 2, 2024

@mu001999 was this in time for you?

Guess not, sorry 😔.

@mu001999
Copy link
Contributor

mu001999 commented Feb 2, 2024

@mu001999 was this in time for you?

Guess not, sorry 😔.

Never mind ;) seems the CI stuck again unexpectedly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc 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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.