internal: Beginning of MIR #14040

HKalbasi · 2023-01-27T17:09:07Z

This pull request introduces the initial implementation of MIR lowering and interpreting in Rust Analyzer.

The implementation of MIR has potential to bring several benefits:

Executing a unit test without compiling it: This is my main goal. It can be useful for quickly testing code changes and print-debugging unit tests without the need for a full compilation (ideally in almost zero time, similar to languages like python and js). There is a probability that it goes nowhere, it might become slower than rustc, or it might need some unreasonable amount of memory, or we may fail to support a common pattern/function that make it unusable for most of the codes.
Constant evaluation: MIR allows for easier and more correct constant evaluation, on par with rustc. If r-a wants to fully support the type system, it needs full const eval, which means arbitrary code execution, which needs MIR or something similar.
Supporting more diagnostics: MIR can be used to detect errors, most famously borrow checker and lifetime errors, but also mutability errors and uninitialized variables, which can be difficult/impossible to detect in HIR.
Lowering closures: With MIR we can find out closure capture modes, which is useful in detecting if a closure implements the FnMut or Fn traits, and calculating its size and data layout.

But the current PR implements no diagnostics and doesn't support closures. About const eval, I removed the old const eval code and it now uses the mir interpreter. Everything that is supported in stable rustc is either implemented or is super easy to implement. About interpreting unit tests, I added an experimental config, disabled by default, that shows a pass or fail on hover of unit tests (ideally it should be a button similar to Run test button, but I didn't figured out how to add them). Currently, no real world test works, due to missing features including closures, heap allocation, dyn Trait and ... so at this point it is only useful for me selecting what to implement next.

The implementation of MIR is based on the design of rustc, the data structures are almost copy paste (so it should be easy to migrate it to a possible future stable-mir), but the lowering and interpreting code is from me.

crates/hir-def/src/macro_expansion_tests/builtin_fn_macro.rs

crates/hir-ty/src/display.rs

lnicola · 2023-01-27T18:34:31Z

crates/test-utils/src/fixture.rs

@@ -180,7 +180,9 @@ impl Fixture {
        let mut cfg_key_values = Vec::new();
        let mut env = FxHashMap::default();
        let mut introduce_new_source_root = None;
-        let mut target_data_layout = None;
+        let mut target_data_layout = Some(


Nit: some of the tests already have a layout annotation, should we also set it globally here?

Currently all of const eval tests, and some of tests in infer need it, so I think it make sense to have it for default. If we keep this, those annotations can be removed.

My sight preference would be to make them explicit, since it seems like something that might catch people off-guard. But it's not very important tbh.

Your point is valid, but I think there are too much tests that needs it (and there will be more) so the visual noise doesn't worth it, and also keep them all in a same place makes future changes to it easier. So I would prefer it to be present by default.

Ye I think keeping a default here is fine. No need to remove the setting of this though, maybe we do want to specifically test different layouts in some places at some point

crates/rust-analyzer/src/cli/flags.rs

flodiebold · 2023-01-29T16:11:53Z

crates/hir-ty/src/infer.rs

@@ -509,7 +509,7 @@ impl<'a> InferenceContext<'a> {
        }
    }

-    fn resolve_all(self) -> InferenceResult {
+    pub(crate) fn resolve_all(self) -> InferenceResult {


this doesn't actually get called from anywhere new, does it?

It is called in evaluating const generics. I'm probably doing something wrong here. The problem is, const eval is called inside type inference, but the constant in const generics share their body with their parent and type check at the same time, so in evaluating constants, the types inside of the const are not resolved yet. I made a clone of InferenceContext and called resolve_all on it as a hacky workaround.

Can you put a FIXME on this/the call then?

flodiebold · 2023-01-29T16:14:40Z

crates/hir-ty/src/lib.rs

+/// the neccessary bits of memory of the const eval session to keep the constant
+/// meaningful.
+#[derive(Debug, Default, Clone, PartialEq, Eq)]
+pub struct MemoryMap(pub HashMap<usize, Vec<u8>>);


should that maybe be a BTreeMap for sortedness / iteration order?

I don't think this should ever need to be iterated over.

Yes it probably doesn't need to be sorted and iterated, the only problem I can think of is Hash implementation. I currently excluded memory map field from hash implementation of the ConstScalar. If it is fine, then I think it doesn't need to be a BTreeMap.

crates/hir-ty/src/lib.rs

crates/hir-ty/src/mir/lower.rs

flodiebold · 2023-01-29T16:38:58Z

crates/hir-ty/src/mir/eval.rs

+pub fn interpret_mir(
+    db: &dyn HirDatabase,
+    body: &MirBody,
+    assert_placeholder_ty_is_unused: bool,


what's this used for?

The interpret_mir function? It is used in const eval and interpreting unit tests. If you mean the assert_placeholder_ty_is_unused, it is another hack due const generics doesn't have a separate body. Currently it generates a local for every binding in the hir body, but in interpreting const generics, we don't know all bindings types. This arguments assumes that they are unused (which is clearly wrong, we don't know if there is a type with placeholder inside of the constant body) and will use zero as size of types with placeholder, instead of failing.

Likewise, can we put a FIXME comment regarding that somewhere? (ideally linking to the issue of lowering const generics maybe)

bors · 2023-02-02T10:00:05Z

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

crates/hir-ty/src/mir/eval.rs

bors · 2023-02-06T19:58:18Z

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

bors · 2023-02-14T17:13:33Z

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

lnicola · 2023-02-16T11:55:30Z

This seems to increase the memory usage on analysis-stats self by around 2 MB, but I'm not sure if it's actually used.

HKalbasi · 2023-02-16T12:39:39Z

It is used in const eval which is used in type checking, and it caches somethings, including MIR bodies and data layout of types, but since r-a code is not const-generic heavy, I don't know if this increase is natural or indicates a problem.

lnicola · 2023-02-16T12:41:26Z

r-a and its dependencies. But yeah, the memory usage looks good for now.

Veykril · 2023-02-16T12:48:38Z

Also note that the new MIR code itself is being analyzed itself now as well

lnicola · 2023-02-16T12:49:46Z

No, I measured both for the code on this branch.

HKalbasi · 2023-02-24T19:03:52Z

If you are agree with the general direction of this, and there is no problem in changes outside of MIR parts, I think we can merge this (probably after Monday to minimize damage of possible breakages) to unblock further progress on it. The implementation details of MIR are subject to change, so reviewing that doesn't have much value, and I can apply your suggestions after merge as well. Also please tell me if the current unresolved reviews need action.

Veykril · 2023-02-27T09:13:28Z

I'm fine with merging this if you add the two FIXME comments

HKalbasi · 2023-02-28T09:12:12Z

I added the FIXME comments.

@bors r+

bors · 2023-02-28T09:12:14Z

📌 Commit cd67589 has been approved by HKalbasi

It is now in the queue for this repository.

bors · 2023-02-28T09:12:23Z

⌛ Testing commit cd67589 with merge a0be16b...

bors · 2023-02-28T09:24:44Z

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing a0be16b to master...

bors · 2023-02-28T09:24:44Z

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing a0be16b to master...

bors · 2023-02-28T09:24:47Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

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

lnicola reviewed Jan 27, 2023

View reviewed changes

crates/hir-def/src/macro_expansion_tests/builtin_fn_macro.rs Show resolved Hide resolved

lnicola reviewed Jan 27, 2023

View reviewed changes

crates/hir-ty/src/display.rs Outdated Show resolved Hide resolved

lnicola reviewed Jan 27, 2023

View reviewed changes

crates/rust-analyzer/src/cli/flags.rs Outdated Show resolved Hide resolved

HKalbasi force-pushed the mir branch from 90576d2 to fe17f77 Compare January 27, 2023 22:41

flodiebold reviewed Jan 29, 2023

View reviewed changes

HKalbasi force-pushed the mir branch 3 times, most recently from 6de59aa to 2ae1d4f Compare January 31, 2023 22:01

HKalbasi force-pushed the mir branch from 2ae1d4f to fc48c7d Compare February 2, 2023 10:16

HKalbasi commented Feb 2, 2023

View reviewed changes

crates/hir-ty/src/mir/eval.rs Show resolved Hide resolved

HKalbasi force-pushed the mir branch 3 times, most recently from ef3389b to cfdf067 Compare February 3, 2023 11:16

HKalbasi force-pushed the mir branch from cfdf067 to 6fc0c01 Compare February 7, 2023 10:42

HKalbasi force-pushed the mir branch from 6fc0c01 to 99d30dd Compare February 16, 2023 11:16

HKalbasi force-pushed the mir branch from 99d30dd to ad684ba Compare February 21, 2023 19:36

HKalbasi mentioned this pull request Feb 23, 2023

Feature request: expose type sizes #14199

Closed

beginning of MIR

cd67589

HKalbasi force-pushed the mir branch from ad684ba to cd67589 Compare February 27, 2023 20:16

bors merged commit a0be16b into rust-lang:master Feb 28, 2023

lnicola changed the title ~~Beginning of MIR~~ internal: Beginning of MIR Mar 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal: Beginning of MIR #14040

internal: Beginning of MIR #14040

HKalbasi commented Jan 27, 2023

lnicola Jan 27, 2023

HKalbasi Jan 27, 2023

lnicola Feb 24, 2023

HKalbasi Feb 24, 2023

Veykril Feb 27, 2023

flodiebold Jan 29, 2023

HKalbasi Jan 29, 2023

Veykril Feb 27, 2023

flodiebold Jan 29, 2023

bjorn3 Jan 29, 2023

HKalbasi Jan 29, 2023

flodiebold Jan 29, 2023

HKalbasi Jan 29, 2023

Veykril Feb 27, 2023

bors commented Feb 2, 2023

bors commented Feb 6, 2023

bors commented Feb 14, 2023

lnicola commented Feb 16, 2023

HKalbasi commented Feb 16, 2023

lnicola commented Feb 16, 2023

Veykril commented Feb 16, 2023

lnicola commented Feb 16, 2023 •

edited

Loading

HKalbasi commented Feb 24, 2023

Veykril commented Feb 27, 2023

HKalbasi commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

internal: Beginning of MIR #14040

internal: Beginning of MIR #14040

Conversation

HKalbasi commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Feb 2, 2023

bors commented Feb 6, 2023

bors commented Feb 14, 2023

lnicola commented Feb 16, 2023

HKalbasi commented Feb 16, 2023

lnicola commented Feb 16, 2023

Veykril commented Feb 16, 2023

lnicola commented Feb 16, 2023 • edited Loading

HKalbasi commented Feb 24, 2023

Veykril commented Feb 27, 2023

HKalbasi commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

bors commented Feb 28, 2023

lnicola commented Feb 16, 2023 •

edited

Loading