MIR episode 3 #14368

HKalbasi · 2023-03-16T22:02:30Z

This PR adds lowering for try operator and overloaded dereference, and adds evaluating support for function pointers and trait objects. It also adds a flag to analysis-stats to show percentage of functions that it fails to emit mir for them, which is currently 20% (which is somehow lying, as most of the supported 80% are tests). The most offenders are closure (1975 items) and overloaded index (415 items). I will try to add overloaded index before monday to have it in this PR, and tackle the closure in the next episode.

HKalbasi · 2023-03-17T08:32:19Z

~~benchmark_syntax_highlighting_parser doesn't fail on my machine, and it seems unstable (IIRC I hit it another time). Would it make sense to ignore it?~~ nevermind, it was another syntax highlighting test.

Veykril

Only reviewed the first commit (try desugaring) for now, will take another look at the rest later

Veykril · 2023-03-17T08:41:57Z

crates/hir-def/src/body/lower.rs

@@ -199,6 +204,10 @@ impl ExprCollector<'_> {
        self.source_map.pat_map.insert(src, id);
        id
    }
+    // desugared pats don't have ptr, that's wrong and should be fixed somehow.


This should be a FIXME, and ideally we should fix this before doing more desugaring as this will degrade IDE features otherwise

Added the FIXME.

I copied this comment from the existing alloc_expr_desugared. I didn't use alloc_expr_desugared and used the syntax_ptr of the root expression for generated expressions. Is it correct? And is there any feature that might not work due usage of alloc_pat_desugared in try desugaring?

So I took another look, this will only be problematic for diagnostics it seems (that is the desugared stuff causes some diagnostics to trigger, we won't show them then).

crates/hir-def/src/body/lower.rs

Veykril · 2023-03-17T08:45:44Z

crates/hir-def/src/path.rs

+pub enum Path {
+    /// A normal path
+    Normal {
+        /// Type based path like `<T>::foo`.
+        /// Note that paths like `<Type as Trait>::foo` are desugared to `Trait::<Self=Type>::foo`.
+        type_anchor: Option<Interned<TypeRef>>,
+        mod_path: Interned<ModPath>,
+        /// Invariant: the same len as `self.mod_path.segments` or `None` if all segments are `None`.
+        generic_args: Option<Box<[Option<Interned<GenericArgs>>]>>,
+    },
+    /// A link to a lang item. It is used in desugaring of things like `x?`. We can show these
+    /// links via a normal path since they might be private and not accessible in the usage place.
+    LangItem(LangItemTarget),


I am not too fond of this change, I see why the need for it is there but I wonder if we can solve this in a better way that doesn't require enum-ifying it

We can add it to PathKind which is already an enum, but it doesn't look great since segments, type anchor and generic args doesn't really mean for lang item paths.

That won't work as PathKind comes from hir-expand which does not depend on hir-def (so no LangItemTarget there)

Actually, this is fine, let's keep it this way.

crates/hir-expand/src/name.rs

Veykril · 2023-03-17T08:49:58Z

crates/hir-def/src/resolver.rs

@@ -245,8 +265,24 @@ impl Resolver {
    pub fn resolve_path_in_value_ns(
        &self,
        db: &dyn DefDatabase,
-        path: &ModPath,
+        path: &Path,


This likewise isn't too nice, the resolver should not care about generics and what not, so it now taking a Path feels a bit weird. (Though maybe its time for us to take another look at how we want to encode hir paths anyways)

OTOH it saves a bunch of .mod_path() in callers. Maybe it make sense to have another method that gets a ModPath and this function call it?

Though maybe its time for us to take another look at how we want to encode hir paths anyways

I would suggest encoding paths as resolved ids and (partially) resolving them in hir lowering. Does it have any problem that current state avoid it?

After a second thought this should be fine as well I think

I think it simplifies many things. I will try to do it in another PR.

Veykril · 2023-03-17T08:51:15Z

Given MIR will grow from the looks of it, would it make sense (and is it feasible) to split the MIR things out into its own crate?

crates/hir-ty/src/traits.rs

crates/hir-def/src/body/lower.rs

crates/hir-ty/src/infer/unify.rs

HKalbasi · 2023-03-17T22:25:16Z

I missed this one:

Given MIR will grow from the looks of it, would it make sense (and is it feasible) to split the MIR things out into its own crate?

MIR is dependent on type definitions, and type checking / lowering depends on MIR (due const evaluation), so it is possible to split the current hir-ty crate into 3 crates: hir-ty-def, mir and hir-ty-impl. Whether we should do it or not is another topic and I'm very neutral on that.

Veykril · 2023-03-20T21:15:38Z

Right, these things are very much intertwined, I don't think we gain anything from splitting these into crates then so we should just try to keep the modules cleanly apart where we can

crates/hir-ty/src/infer/expr.rs

Veykril · 2023-03-21T07:22:53Z

crates/hir-def/src/body/lower.rs

    source_map: BodySourceMap,
+    current_try_block: Option<LabelId>,


This makes me wonder if we should make label resolution be part of the body lowering (not in this PR as it is already way too big, but just a thougght)

Fully agree, we should resolve everything we can in body lowering.

Veykril · 2023-03-21T07:27:36Z

crates/hir-expand/src/name.rs

+    /// Generates a new name which is only equal to itself, by incrementing a counter. Due
+    /// its implementation, it should not be used in things that salsa considers, like
+    /// type names or field names, and it should be only used in names of local variables
+    /// and labels and similar things.
+    pub fn generate_new_name() -> Name {
+        use std::sync::atomic::{AtomicUsize, Ordering};
+        static CNT: AtomicUsize = AtomicUsize::new(0);
+        let c = CNT.fetch_add(1, Ordering::Relaxed);
+        // FIXME: Currently a `__RA_generated_name` in user code will break our analysis
+        Name::new_text(format!("__RA_geneated_name_{c}").into())
+    }


Can you put a FIXME on this that we should really move to interned symbols (with support for unique name generation)? A similar comment already exists on missing but thats also missing a FIXME. The fact that we use SmolStr for names is something I really really dislike (which becomes apparent here)

This function should be removed entirely when we resolve paths in hir lowering.

Veykril · 2023-03-21T07:31:53Z

crates/ide/src/syntax_highlighting/tests.rs

@@ -1126,5 +1126,5 @@ fn benchmark_syntax_highlighting_parser() {
            .filter(|it| it.highlight.tag == HlTag::Symbol(SymbolKind::Function))
            .count()
    };
-    assert_eq!(hash, 1608);


This seems odd, we effectively resolve less things as functions in this test now? Can you make sure that analysis-stats didn't regress?

Oh actually, this might be because this test runs without the sysroot which this PR might cause to regress some resolutions depending on things in there (which is fine, but verifying analysis-stats would be good if you haven't yet)

You are right. analysis-stats has 31 ??ty now.

Veykril · 2023-03-21T07:34:20Z

crates/test-utils/src/minicore.rs

@@ -181,7 +181,7 @@ pub mod convert {
    }
    // endregion:as_ref
    // region:infallible
-    pub enum Infallibe {}
+    pub enum Infallible {}


One thing I just realized regarding minicore, we should have a test that verifies minicore contains no unresolved things somehow to guard against accidental typos. (Again something for a follow up)

Good idea. Long term I think we also need something to make sure it is in sync with real core, or reusing the real core somehow.

HKalbasi · 2023-03-21T19:57:20Z

I will go ahead and merge this to have it in the next nightly. I will address current (and future, if any) reviews in follow up PRs.

@bors r+

bors · 2023-03-21T19:57:22Z

📌 Commit 8a3ad7c has been approved by HKalbasi

It is now in the queue for this repository.

bors · 2023-03-21T19:57:30Z

⌛ Testing commit 8a3ad7c with merge 3321799...

bors · 2023-03-21T20:15:41Z

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

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

HKalbasi force-pushed the mir branch 3 times, most recently from e5d1bd2 to b91ac56 Compare March 16, 2023 22:31

Veykril reviewed Mar 17, 2023

View reviewed changes

HKalbasi added 7 commits March 17, 2023 13:08

desugar ? operator

b7b9ae5

add mir-stats to analysis-stats

1b85b43

Support function pointer MIR lowering

a063f00

Support evaluating dyn Trait methods

7525a38

implement transmute intrinsic

513e340

Support record pattern MIR lowering

051dae2

Improve pattern matching MIR lowering

9564773

HKalbasi force-pushed the mir branch 2 times, most recently from dd46d07 to 7c9e99d Compare March 17, 2023 10:18

Support overloaded deref MIR lowering

eb4939e

HKalbasi force-pushed the mir branch from 7c9e99d to eb4939e Compare March 17, 2023 10:33

Support overloaded index MIR lowering

9ad83de

lowr reviewed Mar 17, 2023

View reviewed changes

HKalbasi force-pushed the mir branch from 558c896 to 3514577 Compare March 17, 2023 22:21

Support range MIR lowering

453ae2e

HKalbasi force-pushed the mir branch 3 times, most recently from f3bd169 to 7bc1761 Compare March 19, 2023 09:02

HKalbasi requested a review from lowr March 19, 2023 09:03

Desugar try blocks

8e73ea5

HKalbasi force-pushed the mir branch from 7bc1761 to 8e73ea5 Compare March 19, 2023 09:33

Implement some intrinsics

3303a6e

Veykril reviewed Mar 20, 2023

View reviewed changes

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

HKalbasi force-pushed the mir branch 2 times, most recently from 2064ae1 to 2514bcf Compare March 20, 2023 22:34

Support evaluating inherent associated constants with generics

8a3ad7c

HKalbasi force-pushed the mir branch from 2514bcf to 8a3ad7c Compare March 20, 2023 22:50

Veykril reviewed Mar 21, 2023

View reviewed changes

bors merged commit 3321799 into rust-lang:master Mar 21, 2023

bors mentioned this pull request Mar 21, 2023

fix: #12247 fully qualified call required to determine trait method type #13754

Closed

Veykril mentioned this pull request Mar 29, 2023

fix: Use struct_tail_without_normalization in Expectation::rvalue_hint #14434

Merged

Veykril mentioned this pull request Apr 5, 2023

Do more name resolution in body lowering #14498

Open

HKalbasi mentioned this pull request May 13, 2023

Pointer casts are not inferred #12532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR episode 3 #14368

MIR episode 3 #14368

HKalbasi commented Mar 16, 2023

HKalbasi commented Mar 17, 2023 •

edited

Loading

Veykril left a comment

Veykril Mar 17, 2023

HKalbasi Mar 17, 2023

Veykril Mar 20, 2023

Veykril Mar 17, 2023

HKalbasi Mar 17, 2023

Veykril Mar 20, 2023

Veykril Mar 20, 2023 •

edited

Loading

Veykril Mar 17, 2023

HKalbasi Mar 17, 2023

HKalbasi Mar 18, 2023

Veykril Mar 20, 2023

HKalbasi Mar 20, 2023

Veykril commented Mar 17, 2023

HKalbasi commented Mar 17, 2023

Veykril commented Mar 20, 2023

Veykril Mar 21, 2023

HKalbasi Mar 21, 2023

Veykril Mar 21, 2023

HKalbasi Mar 21, 2023

Veykril Mar 21, 2023

Veykril Mar 21, 2023

HKalbasi Mar 21, 2023

Veykril Mar 21, 2023

HKalbasi Mar 21, 2023

HKalbasi commented Mar 21, 2023

bors commented Mar 21, 2023

bors commented Mar 21, 2023

bors commented Mar 21, 2023

		source_map: BodySourceMap,
		current_try_block: Option<LabelId>,

MIR episode 3 #14368

MIR episode 3 #14368

Conversation

HKalbasi commented Mar 16, 2023

HKalbasi commented Mar 17, 2023 • edited Loading

Veykril left a 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

Veykril Mar 20, 2023 • edited Loading

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

Veykril commented Mar 17, 2023

HKalbasi commented Mar 17, 2023

Veykril commented Mar 20, 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

HKalbasi commented Mar 21, 2023

bors commented Mar 21, 2023

bors commented Mar 21, 2023

bors commented Mar 21, 2023

HKalbasi commented Mar 17, 2023 •

edited

Loading

Veykril Mar 20, 2023 •

edited

Loading