Stream: t-compiler/wg-rls-2.0

Topic: Adding mut/owning semantic modifiers for fns


Paul Faria (Jul 30 2020 at 14:08, on Zulip):

Hi @matklad , what are your thoughts on adding mutable and owning (Maybe a different name, but would be a new HighlightModifier) semantic modifiers to fns that take &mut self and self, respectively? I remember when I was doing rust years ago, as a beginner, I found it hard to understand how certain method calls were changing the behavior of the borrow checker. I don't propose any default syntax highlighting yet, but thought it might be useful to experiment with.

matklad (Jul 30 2020 at 14:09, on Zulip):

We already have mutable

Paul Faria (Jul 30 2020 at 14:10, on Zulip):

I meant new only for the owning one.

matklad (Jul 30 2020 at 14:10, on Zulip):

I think highlighitng functions calls that are actually mutable might be a good idea

Paul Faria (Jul 30 2020 at 14:10, on Zulip):

I wanted to distinguish between &, &mut and moving fns

matklad (Jul 30 2020 at 14:10, on Zulip):

(there's more general idea of highlighing not mutable types, but rather mutation points)

matklad (Jul 30 2020 at 14:11, on Zulip):

I think marking consumed non-Copy things might also be a good idea

matklad (Jul 30 2020 at 14:11, on Zulip):

/s we can use strikethrough, because the thing is GONE :D

Paul Faria (Jul 30 2020 at 14:11, on Zulip):

XD

Paul Faria (Jul 30 2020 at 14:13, on Zulip):

Back when I had the borrow visualizer prototype working, it was a red outline around the moved value.

Paul Faria (Jul 30 2020 at 14:13, on Zulip):

I'll play around and see what looks nice and not horribly distracting

Paul Faria (Jul 31 2020 at 13:56, on Zulip):

So I've been stuck on the Copy part of this for a while now. I (think) I have the logic down where once I have the Copy trait, I can validate whether the type impls Copy or not. However I'm stuck actually finding the Copy trait. My current attempt was to move FamousDefs from ra_assists::utils to a new module ra_hir::utils, and add a core_marker_Copy fn that looks for core:marker:Copy (very similar to the From and Option fns). However, this always fails to find core (and even std). Any thoughts on what I could do to fix this or maybe a better way to figure out whether a type is Copy or not?

Florian Diebold (Jul 31 2020 at 14:16, on Zulip):

somewhere where you have a db,

        let trait_ = match db.lang_item(krate, "copy") {
            Some(LangItemTarget::TraitId(trait_)) => Some(trait_),
            _ => None,
        };
Florian Diebold (Jul 31 2020 at 14:16, on Zulip):

that's the way to get lang items

Paul Faria (Jul 31 2020 at 16:33, on Zulip):

Well that's so much easier than what I was trying to do :sweat_smile:. I'll give that a try after work. Thanks!

Paul Faria (Aug 01 2020 at 15:00, on Zulip):

@Florian Diebold so this doesn't seem to be working for me:

            let krate = Function::from(func).krate(self.db)?;
            println!("Crate is {:?}", krate);
            let lang_item = self.db.lang_item(krate.id, SmolStr::new("copy"));
            println!("copy lang_item? {:?}", lang_item);

The above code prints these logs (where self is SemanticsImpl)

Crate is Crate { id: CrateId(0) }
copy lang_item? None
Florian Diebold (Aug 01 2020 at 15:02, on Zulip):

If that's in a unit test, keep in mind that you don't have the standard library and actually have to define it

Paul Faria (Aug 01 2020 at 15:04, on Zulip):

I was JUST about to ask that

Paul Faria (Aug 01 2020 at 15:08, on Zulip):

Ok, that's working now. Thanks!

Paul Faria (Aug 01 2020 at 15:24, on Zulip):

So I got stuck on impl Copy for MyType {} vs #[derive(Copy, Clone)] struct MyType .... Is the latter supposed to be recognized as a trait impl within a unit test?

Paul Faria (Aug 01 2020 at 15:26, on Zulip):

What I'm doing is:

            let ty = self.type_of_self(&self_param)?;
            let krate = Function::from(func).krate(self.db)?;
            let lang_item = self.db.lang_item(krate.id, SmolStr::new("copy"));
            let copy_trait = match lang_item? {
                LangItemTarget::TraitId(copy_trait) => Trait::from(copy_trait),
                _ => return None,
            };
            if ty.impls_trait(self.db, copy_trait, &[]) {
                ...
            }

The if block is entered only when I have impl Copy for MyType {}, and not with the derive syntax. Is this just a peculiarity with the test, or would this actually be an issue on real code?

Florian Diebold (Aug 01 2020 at 16:16, on Zulip):

you would need to define the built-in macro for the latter to be resolved

Florian Diebold (Aug 01 2020 at 16:17, on Zulip):

I'd just use impl Copy for MyType {}

Florian Diebold (Aug 01 2020 at 16:18, on Zulip):

i.e. you need this boilerplate so the derive can be resolved

Florian Diebold (Aug 01 2020 at 16:19, on Zulip):

... I think? :thinking: actually maybe the problem is rather that the derive expects the Copy trait to be at the path that it usually is

Paul Faria (Aug 01 2020 at 16:51, on Zulip):

Just got back to my desk. None of that worked... I think I'll just use the impl Copy ... for the test to keep it simple. Thanks so much for your help today!

Paul Faria (Aug 01 2020 at 16:54, on Zulip):

I.e. I tried

mod core {
    pub mod marker {
        #[lang = "copy"]
        pub trait Copy {}

        #[rustc_builtin_macro]
        #[allow_internal_unstable(core_intrinsics, derive_clone_copy)]
        pub macro Copy($item:item) {}
    }
}

And the derive still wasn't recognized as an impl Trait. I tried with the clone bounds as well and that didn't make a difference.

Paul Faria (Aug 01 2020 at 17:11, on Zulip):

Turns out this is all that's needed:

pub mod marker {
    #[lang = "copy"]
    pub trait Copy  {}
}

If there is no core dep, then the krate for the lang item is assumed to be just the crate keyword.

Paul Faria (Aug 01 2020 at 17:57, on Zulip):

PR up here: https://github.com/rust-analyzer/rust-analyzer/pull/5643

woody77 (Aug 03 2020 at 23:39, on Zulip):

Paul Faria said:

I'll play around and see what looks nice and not horribly distracting

I'd just ask to make it a modifier, and then theme it separately, vs. pushing textmate formatted scopes to the IDE(s). We'll need to work to get these taken advantage of in various themes (and samples for devs to add on their own, like the examples that I have put up), but I think it's a better path forward than trying to make formatting decisions in RA itself.

Paul Faria (Aug 04 2020 at 13:00, on Zulip):

That's the path I ended up on in the PR. The function.mutables get an underline since the mutable modifier adds an underline by default. I didn't add any styling to consuming (I feel like I should use a more typical rust name here like owning or moving/moved, brought up the same in the PR).

Last update: Sep 27 2020 at 13:45UTC