feat: MIR episode 2 #14232

HKalbasi · 2023-03-02T10:24:54Z

This PR adds:

need-mut and unused-mut diagnostics
View mir command which shows MIR for the body under cursor, useful for debugging
MIR lowering for or-patterns and for-loops

HKalbasi · 2023-03-03T09:11:36Z

When running rust-analyzer diagnostics . I see some errors that don't exist in the original file, for example cannot mutate immutable variable None. I guess it is because std is not ready at that time so None pattern would be treated like an identifier. How I can force dependencies to be resolved in rust-analyzer diagnostic .? Or is there another way (like a vscode command) to see all diagnostics for a crate?

flodiebold · 2023-03-03T09:18:34Z

I'm ... pretty sure std is resolved in rust-analyzer diagnostics .? Unless you get a "sysroot can't be resolved" error.

HKalbasi · 2023-03-03T09:27:41Z

I'm not getting a sysroot can't be resolved error, but there are 22k diagnostics for the r-a repo, mostly unresolved-macro-call and unresolved-import.

Veykril · 2023-03-03T09:46:13Z

Can confirm that I am getting nresolved macros as well for current r-a, that's odd ...

Veykril · 2023-03-03T09:47:27Z

Oh I know why :) Diagnostics is still using the default cargo config without the

cargo_config.sysroot = match self.no_sysroot {
    true => None,
    false => Some(RustcSource::Discover),
};

part that is now necessary.
That is diagnostics is currently opting out of sysroot loading, hence no error on that either.
#14239 fixes this

bors · 2023-03-03T10:53:36Z

☔ The latest upstream changes (presumably #14222) made this pull request unmergeable. Please resolve the merge conflicts.

Veykril

Loving the introduction of bindings, handling bindings through binding patterns was always a pain

Veykril · 2023-03-04T10:12:37Z

crates/hir-ty/src/infer.rs

+    /// Type of the result of `.into_iter()` on the for. `ExprId` is the one of the whole for loop.
+    pub type_of_for_iterator: ArenaMap<ExprId, Ty>,


This feels odd to have, though I gues we don't really have a lot of options here since we don't desugar so there is nothing to refer to for the resolver into iter type?

Though we definitely don't want this to be an ArenaMap, as that will allocate a lot due to it being backed by a Vec. That is if we were to insert the expr id 152, we allocate a vec of length 152 where the first 151 elements are unmapped. So this should be an FxHashMap.

Ah yes that's wrong. Between FxHashMap and NoHashHashMap which one would be better here?

About desugaring, it would also help in simplifing MIR lowering which currently is ugly (and this field is used there) and type inference. Currently both need to find IntoIterator related things via lang items, but with desugaring it would be unified in one place. But it probably has it's own cons too.

Since this is very rarely populated with keys I'd say NoHashHashMap.

Though I am wondering whether we should get rid of that hashmap type again. I assumed that hashbrown has no problems with sequential hashes but I believe that was wrong (forgot where I saw that), so that type most likely has no perf gain for our usual use cases with IDs. Would need to investigate.

I was wondering about HIR desugaring in r-a, there is almost (if any at all) no desugaring happening and I am unsure whether there is a reason for that aside from the usual "there was no need for it so far".

NoHashHashMap didn't work for some reason, so I used FxHashMap.

Experimenting with HIR desugaring can be started with ? operator, as it would solve real problems like #12247

Veykril · 2023-03-04T10:22:13Z

crates/hir/src/lib.rs

-        match &body[self.pat_id] {
-            Pat::Bind { name, .. } => name.clone(),
-            _ => {
-                stdx::never!("hir::Local is missing a name!");
-                Name::missing()
-            }
-        }


Finally this is gone :)

HKalbasi · 2023-03-04T20:26:37Z

There was a false positive for need-mut in r-a repo, and I fixed it so there is nothing now. It isn't surprising, as most of the codes has something unsupported in MIR lowering (closures, ?, overloaded operators, ...) and there would be no error in them (neither true nor false positive). So I think we can enable it by default if inconsistency of emitting errors isn't a problem.

For unused-mut, there is a more fundamental problem that we don't respect #[allow(unused_mut)], and since attributes on expressions are not supported currently it isn't easy to solve completely. Should it become experimental?

Veykril · 2023-03-06T10:43:25Z

need-mut being enabled by default sounds good to me, I am a bit worried about unused mut though, I think that one should go into the experimental bucket.

bors · 2023-03-06T11:58:01Z

☔ The latest upstream changes (presumably #14261) made this pull request unmergeable. Please resolve the merge conflicts.

flodiebold · 2023-03-06T12:13:31Z

IMO we should only start out with a diagnostic being non-experimental of we are really sure that it's not going to have false positives. (And if we're going to have more false positives once more of MIR gets implemented, that's also not good.)

Veykril · 2023-03-06T12:57:30Z

Yes I agree, I assumed the need-mut diagnostic has none now (only false negatives), but if I am wrong with that it should also be set to experimental.

HKalbasi · 2023-03-06T18:31:27Z

I made unused_mut experimental. About need_mut, I'm not super confident that it would never cause a single false positive. What I'm pretty sure about is that there would be very few false positives (way lower than some of the current non experimental diagnostics), so it wouldn't become an annoying diagnostic, and there is no false positive in r-a and some other rust projects I had in my disk, and that I can patch every bug report about it (in the worst case by unsupporting a feature in MIR). I will try to maintain this zero false positive in future changes in MIR, but frequent changes will increase the bug probability. By merging these MIR PRs in Mondays and maybe checking the false positive in CI or metrics, I think we can mitigate the possible damage.

So I think it doesn't need to be experimental, but if you think it needs, I can change it.

HKalbasi · 2023-03-06T20:29:47Z

I also have some questions about performance, things about the query system is not clear to me. What would become invalidated (hir, hir source map, infer result, mir, diagnostics) in these changes?

A single character inside the function
A single character above the function, changing the ranges of the elements
A single character in a struct or enum (in another file), potentially changing it's definition.

And how much time we are allowed to put on the diagnostics, what will happen if we spend more?

Another question is that I saw some profiling infrastructure on the inference code, should I add them to the MIR and borrow checking as well? And where is the result of that profiling available?

Veykril · 2023-03-06T20:55:12Z

A single character inside the function

An item is pretty much the smallest unit we have in a sense, so changing a character in a function should invalidate the body, meaning hir gets redone for it and all that follows from that afaik. (diagnostics aren't even a query I think so thats just recalculated on every change). This mainly goes for the function body, if the function signature is changed, this will have wider implications obviously, as the signature can be observed by other things.

A single character above the function, changing the ranges of the elements

This I am not actually sure about, we assign ids to nodes in breadth first order which allows us to not having to recalculate anything item related when changing things inside of an item, but adding a single character between items is parsed as a macro call I believe which should invaldiated all items below if I am not mistaken?

A single character in a struct or enum (in another file), potentially changing it's definition.

Anything that depends on those definition (transitively).

In short, salsa will check dependency queries for whether they are still fresh or not. A query might have to be re-evaluated because its dependencies changed, but if the result of the query is equal to the previous result it will be marked as not having changed I think stopping the "dirtyness" in propagating (not 100% sure if this always works that way though).

HKalbasi · 2023-03-06T21:04:22Z

Anything that depends on those definition (transitively).

This is not very clear to me. For example:

struct Foe(u8); // change Foe to Foo
fn x() {
    let y = Foo(2);
}

x needs to be invalidated, but how salsa knows that it depends on Foe?

flodiebold · 2023-03-06T21:06:25Z

Changing the name of an item changes the crate def map, which invalidates everything that depended on it including all type check results in the crate.

flodiebold · 2023-03-06T21:11:46Z

On the other hand, changing struct Foo(u8) to struct Foo(u32) should (I think) not affect the def map, only the struct_data query -> the field_types query -> type check results for any function that depended on that.

HKalbasi · 2023-03-06T21:18:23Z

Hmm, so adding an item will also invalidate everything, right? That's more invalidating than what I initially thought. So the performance of MIR lowering can become important. Do you think it needs some action?

Veykril · 2023-03-06T21:37:48Z

Adding items should be rare enough for it not to matter, though now I wonder how use items affect this whole picture since those tend to change quite a bit ...

flodiebold · 2023-03-06T21:47:32Z

Use items will also invalidate the def map, I think. At least pub ones have to anyway.

HKalbasi · 2023-03-07T09:38:52Z

I added some profiling. The result from RA_PROFILE='*>10' ./target/release/rust-analyzer diagnostics . showed that the amount of time spent in mir is epsilon relative to the inference and hir body query (probably due not supporting most of the cases) so at least for now I think there is no concern.

Anything else to do on this before merge?

Veykril · 2023-03-07T09:49:32Z

It's also worth noting that (these kinds of) diagnostics are only ever calculated for the open buffers in the editor and not project-wide so its unlikely to be problematic for now.

Let's get this merged now then since its still early in the week.
@bors r+

bors · 2023-03-07T09:49:34Z

📌 Commit bcd7ecb has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-03-07T09:49:53Z

⌛ Testing commit bcd7ecb with merge 44ff3c4...

bors · 2023-03-07T10:04:29Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 44ff3c4 to master...

lnicola · 2023-03-13T06:20:22Z

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2023

Veykril reviewed Mar 4, 2023

View reviewed changes

HKalbasi force-pushed the mir branch from 8e889cb to f0ca929 Compare March 4, 2023 20:08

HKalbasi added 8 commits March 6, 2023 21:09

Add BindingId

61ad6a9

Support "or patterns" MIR lowering

c0a0664

Add need-mut and unused-mut diagnostics

a25710b

Add View Mir command and fix some bugs

ac04bfd

Support "for loop" MIR lowering

6377d50

Fix overloaded deref unused mut false positive

bf0f99f

Partially support "overloaded deref" MIR lowering

af90ec8

Make unused_mut diagnostic experimental

2cce9dc

HKalbasi force-pushed the mir branch from f0ca929 to 2cce9dc Compare March 6, 2023 17:40

Add profiling to mir lower and borrowck query

bcd7ecb

HKalbasi force-pushed the mir branch from d7d0066 to bcd7ecb Compare March 7, 2023 09:13

bors merged commit 44ff3c4 into rust-lang:master Mar 7, 2023

Veykril mentioned this pull request Mar 10, 2023

thread 'Worker' has overflowed its stack when go-to-def on #![derive(Clone)] #14310

Closed

lnicola changed the title ~~MIR episode 2~~ feat: MIR episode 2 Mar 11, 2023

HKalbasi mentioned this pull request Mar 20, 2023

Rename symbol - No file available to rename #14379

Closed

This was referenced Jun 4, 2023

Add "make mutable" diagnostic / quick fix for local variables #8449

Closed

Improve usability for "make variable mut" suggestion #12618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: MIR episode 2 #14232

feat: MIR episode 2 #14232

HKalbasi commented Mar 2, 2023

HKalbasi commented Mar 3, 2023

flodiebold commented Mar 3, 2023

HKalbasi commented Mar 3, 2023

Veykril commented Mar 3, 2023

Veykril commented Mar 3, 2023 •

edited

Loading

bors commented Mar 3, 2023

Veykril left a comment

Veykril Mar 4, 2023

Veykril Mar 4, 2023

HKalbasi Mar 4, 2023

Veykril Mar 4, 2023

HKalbasi Mar 4, 2023

Veykril Mar 4, 2023

HKalbasi commented Mar 4, 2023

Veykril commented Mar 6, 2023

bors commented Mar 6, 2023

flodiebold commented Mar 6, 2023 •

edited

Loading

Veykril commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

Veykril commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

flodiebold commented Mar 6, 2023

flodiebold commented Mar 6, 2023

HKalbasi commented Mar 6, 2023 •

edited

Loading

Veykril commented Mar 6, 2023

flodiebold commented Mar 6, 2023

HKalbasi commented Mar 7, 2023

Veykril commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

lnicola commented Mar 13, 2023

		/// Type of the result of `.into_iter()` on the for. `ExprId` is the one of the whole for loop.
		pub type_of_for_iterator: ArenaMap<ExprId, Ty>,

feat: MIR episode 2 #14232

feat: MIR episode 2 #14232

Conversation

HKalbasi commented Mar 2, 2023

HKalbasi commented Mar 3, 2023

flodiebold commented Mar 3, 2023

HKalbasi commented Mar 3, 2023

Veykril commented Mar 3, 2023

Veykril commented Mar 3, 2023 • edited Loading

bors commented Mar 3, 2023

Veykril left a comment

Choose a reason for hiding this comment

Veykril Mar 4, 2023

Choose a reason for hiding this comment

Veykril Mar 4, 2023

Choose a reason for hiding this comment

HKalbasi Mar 4, 2023

Choose a reason for hiding this comment

Veykril Mar 4, 2023

Choose a reason for hiding this comment

HKalbasi Mar 4, 2023

Choose a reason for hiding this comment

Veykril Mar 4, 2023

Choose a reason for hiding this comment

HKalbasi commented Mar 4, 2023

Veykril commented Mar 6, 2023

bors commented Mar 6, 2023

flodiebold commented Mar 6, 2023 • edited Loading

Veykril commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

Veykril commented Mar 6, 2023

HKalbasi commented Mar 6, 2023

flodiebold commented Mar 6, 2023

flodiebold commented Mar 6, 2023

HKalbasi commented Mar 6, 2023 • edited Loading

Veykril commented Mar 6, 2023

flodiebold commented Mar 6, 2023

HKalbasi commented Mar 7, 2023

Veykril commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 7, 2023

lnicola commented Mar 13, 2023

Veykril commented Mar 3, 2023 •

edited

Loading

flodiebold commented Mar 6, 2023 •

edited

Loading

HKalbasi commented Mar 6, 2023 •

edited

Loading