pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc #120039

Nadrieril · 2024-01-17T02:10:15Z

Today's pattern_analysis uses BitSet and IndexVec on the provided enum variant ids, which only makes sense if these ids count the variants from 0. In rust-analyzer, the variant ids are global interning ids, which would make BitSet and IndexVec ridiculously wasteful. In this PR I add some shims to use FxHashSet/FxHashMap instead outside of rustc.

r? @compiler-errors

rustbot · 2024-01-17T02:10:25Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

compiler-errors

r=me when green

Nadrieril · 2024-01-17T02:31:10Z

Thank you! I think bors will wait for green by itself.

@bors r=compiler-errors

bors · 2024-01-17T02:31:12Z

📌 Commit 19d6f06 has been approved by compiler-errors

It is now in the queue for this repository.

compiler-errors · 2024-01-17T02:32:17Z

Thank you! I think bors will wait for green by itself.

Huh? No it will definitely not. As soon as you r=me, it is in the queue. Please wait next time for CI to be green 😅

Mark-Simulacrum

Are IDs elsewhere still contiguous in the common case, even if not starting at zero? Maybe the caller could offset all IDs being passed in? I've wanted a "IndexVec, but skip leading zeros" type before...

Mark-Simulacrum · 2024-01-17T02:32:03Z

compiler/rustc_pattern_analysis/src/lib.rs

+    impl<T: Copy + PartialEq + Eq + std::hash::Hash> Idx for T {}
+
+    #[derive(Debug)]
+    pub struct IdxContainer<K, V>(pub rustc_hash::FxHashMap<K, V>);


Can we use the rustc_data_structures types here instead and avoid the extra crate dep? (It's still the same under the hood, but makes it easier to tweak globally).

Can we use the rustc_data_structures types here instead [...]

rustc_data_structures is not compatible with rust-analyzer due to its use of nightly features. I guess @Nadrieril could add a feature = "nightly" to that crate, but it seems unnecessary.

Are IDs elsewhere still contiguous in the common case, even if not starting at zero?

I don't believe so. As Nadrieril said above, they're global interning IDs, so they're probably really sparse.

No because rust_analyzer wants to avoid the rustc_data_structures dependency (it's really big and has lots of unstable features)

EDIT: raced by errs

Nadrieril · 2024-01-17T02:36:16Z

As soon as you r=me, it is in the queue.

It's in the queue but bors won't try to merge it until it's green, will it? There's a "mergeable" column in the queue interface, I thought that's what it was for.

Nadrieril · 2024-01-17T02:38:22Z

Are IDs elsewhere still contiguous in the common case, even if not starting at zero?

I don't think so in rust-analyzer, they're newtypes around salsa::InternId which I wouldn't expect has any contiguity guarantee.

compiler-errors · 2024-01-17T02:38:51Z

t's in the queue but bors won't try to merge it until it's green, will it?

Yes, bors will try to merge it as soon as it gets to it in the queue. I'm pretty certain "mergeable" field is totally separate from CI, and has to do with bors's "this PR is out of date and needs to be rebased" message it'll spit out when a PR can't be merged with master.

And someone can still throw it into a rollup PR.

Nadrieril · 2024-01-17T02:39:39Z

Oh, oops.

@bors r-

Nadrieril · 2024-01-17T02:41:58Z

"mergeable" field is totally separate from CI

Wait it does take CI into account: I checked the queue before the r- and this PR didn't have "mergeable=yes"

compiler-errors · 2024-01-17T03:03:22Z

@Nadrieril: I'm pretty certain that mergeability is not a property of CI running. The homu code is incomprehensible, but from what I can tell, this is just a property of whether a PR can be merged with the tip of master. Also from what I can tell, this mergeable field may only be set after bors merges the next commit in the queue (at the same time it'll comment on other PRs "☔ The latest upstream changes made this pull request unmergeable".

But whatever, CI is green now.

@bors r+ rollup

bors · 2024-01-17T03:03:24Z

📌 Commit 19d6f06 has been approved by compiler-errors

It is now in the queue for this repository.

Kobzol · 2024-01-17T09:07:56Z

@Nadrieril The point wasn't that bors would merge a PR without the full test suite passing, but rather that we shouldn't even put the PR into the queue before the basic (PR) test suite passes. Since it could unnecessarily cause failed merges.

Nadrieril · 2024-01-17T11:33:52Z

Ah, I assumed it wouldn't even try to merge. Got this idea from some blogpost of matklad's I think. It seems I was overly optimistic, sorry x)

EDIT: I understood this blog post as implying that bors doesn't have the double latency problem: https://matklad.github.io/2023/06/18/GitHub-merge-queue.html . Are y'all sure it doesn't do what I expect?

…rrors pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc Today's pattern_analysis uses `BitSet` and `IndexVec` on the provided enum variant ids, which only makes sense if these ids count the variants from 0. In rust-analyzer, the variant ids are global interning ids, which would make `BitSet` and `IndexVec` ridiculously wasteful. In this PR I add some shims to use `FxHashSet`/`FxHashMap` instead outside of rustc. r? `@compiler-errors`

…iaskrgr Rollup of 11 pull requests Successful merges: - rust-lang#115291 (Save liveness results for DestinationPropagation) - rust-lang#119855 (Enable Static Builds for FreeBSD) - rust-lang#119975 (Don't ICE if TAIT-defining fn contains a closure with `_` in return type) - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.) - rust-lang#120001 (Consistently unset RUSTC_BOOTSTRAP when compiling bootstrap) - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored) - rust-lang#120031 (Construct closure type eagerly) - rust-lang#120032 (Fix `rustc_abi` build on stable) - rust-lang#120039 (pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc) - rust-lang#120044 (Fix typo in comments (in_place_collect)) - rust-lang#120056 (Use FnOnceOutput instead of FnOnce where expected) r? `@ghost` `@rustbot` modify labels: rollup

…rrors pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc Today's pattern_analysis uses `BitSet` and `IndexVec` on the provided enum variant ids, which only makes sense if these ids count the variants from 0. In rust-analyzer, the variant ids are global interning ids, which would make `BitSet` and `IndexVec` ridiculously wasteful. In this PR I add some shims to use `FxHashSet`/`FxHashMap` instead outside of rustc. r? ``@compiler-errors``

…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#115291 (Save liveness results for DestinationPropagation) - rust-lang#119855 (Enable Static Builds for FreeBSD) - rust-lang#119975 (Don't ICE if TAIT-defining fn contains a closure with `_` in return type) - rust-lang#120001 (Consistently unset RUSTC_BOOTSTRAP when compiling bootstrap) - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored) - rust-lang#120031 (Construct closure type eagerly) - rust-lang#120032 (Fix `rustc_abi` build on stable) - rust-lang#120039 (pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc) - rust-lang#120044 (Fix typo in comments (in_place_collect)) - rust-lang#120056 (Use FnOnceOutput instead of FnOnce where expected) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#120039 - Nadrieril:remove-idx, r=compiler-errors pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc Today's pattern_analysis uses `BitSet` and `IndexVec` on the provided enum variant ids, which only makes sense if these ids count the variants from 0. In rust-analyzer, the variant ids are global interning ids, which would make `BitSet` and `IndexVec` ridiculously wasteful. In this PR I add some shims to use `FxHashSet`/`FxHashMap` instead outside of rustc. r? ```@compiler-errors```

Don't rely on contiguous VariantIds outside of rustc

19d6f06

rustbot assigned compiler-errors Jan 17, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2024

compiler-errors approved these changes Jan 17, 2024

View reviewed changes

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 Jan 17, 2024

Mark-Simulacrum reviewed Jan 17, 2024

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 17, 2024

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 17, 2024

matthiaskrgr mentioned this pull request Jan 17, 2024

Rollup of 11 pull requests #120064

Closed

compiler-errors mentioned this pull request Jan 17, 2024

Rollup of 8 pull requests #120066

Closed

matthiaskrgr mentioned this pull request Jan 17, 2024

Rollup of 10 pull requests #120068

Merged

bors merged commit 3a3242a into rust-lang:master Jan 18, 2024
11 checks passed

rustbot added this to the 1.77.0 milestone Jan 18, 2024

Nadrieril deleted the remove-idx branch January 18, 2024 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc #120039

pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc #120039

Nadrieril commented Jan 17, 2024

rustbot commented Jan 17, 2024

compiler-errors left a comment

Nadrieril commented Jan 17, 2024

bors commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

Mark-Simulacrum left a comment

Mark-Simulacrum Jan 17, 2024

compiler-errors Jan 17, 2024

Nadrieril Jan 17, 2024 •

edited

Nadrieril commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

bors commented Jan 17, 2024

Kobzol commented Jan 17, 2024

Nadrieril commented Jan 17, 2024 •

edited

pat_analysis: Don't rely on contiguous VariantIds outside of rustc #120039

pat_analysis: Don't rely on contiguous VariantIds outside of rustc #120039

Conversation

Nadrieril commented Jan 17, 2024

rustbot commented Jan 17, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Nadrieril commented Jan 17, 2024

bors commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Mark-Simulacrum Jan 17, 2024

Choose a reason for hiding this comment

compiler-errors Jan 17, 2024

Choose a reason for hiding this comment

Nadrieril Jan 17, 2024 • edited

Choose a reason for hiding this comment

Nadrieril commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

Nadrieril commented Jan 17, 2024

compiler-errors commented Jan 17, 2024

bors commented Jan 17, 2024

Kobzol commented Jan 17, 2024

Nadrieril commented Jan 17, 2024 • edited

pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc #120039

pat_analysis: Don't rely on contiguous `VariantId`s outside of rustc #120039

Nadrieril Jan 17, 2024 •

edited

Nadrieril commented Jan 17, 2024 •

edited