Stream: t-compiler/rust-analyzer

Topic: Good First Issue


Chetan Khilosiya (Feb 20 2021 at 14:41, on Zulip):

Hello everyone. I am new to rust analyzer project. I would like to work on issue https://github.com/rust-analyzer/rust-analyzer/issues/7454. I wanted help to understand the requirement of this ticket.

Chetan Khilosiya (Feb 20 2021 at 14:43, on Zulip):

Is the requirement is to update documentation with the correct wording? I checked the functionality in VSCode and it shows "Expand Selection" and "Shrink Selection" as tasks.

Chetan Khilosiya (Feb 20 2021 at 14:44, on Zulip):

In documentation there is no "Shrink Selection", so I can add that also.

Laurențiu (Feb 20 2021 at 14:56, on Zulip):

Oof, sorry, that issue is a bit of a mess

Chetan Khilosiya (Feb 20 2021 at 15:00, on Zulip):

No problem. I will look into another issue.

Laurențiu (Feb 20 2021 at 15:04, on Zulip):

I've added a comment

Laurențiu (Feb 20 2021 at 15:04, on Zulip):

I think there's not much to do there except removing the feature description

Chetan Khilosiya (Feb 20 2021 at 15:39, on Zulip):

Currently when I build the plugin, it is giving me build error. The error in client.ts file. I resolved the error, should I create pull request?

Chetan Khilosiya (Feb 20 2021 at 15:40, on Zulip):

Pulled the latest code on master branch. Latest commit is 1349f6a79177c6b.

Chetan Khilosiya (Feb 20 2021 at 15:41, on Zulip):

I think the issue is due to changes in lc.LanguageClient object.

Chetan Khilosiya (Feb 20 2021 at 15:46, on Zulip):

Sorry about this. after recent pull needed to do npm install to get recent packages. Now its building properly.

Chetan Khilosiya (Feb 22 2021 at 16:34, on Zulip):

Hello everyone. I am new to this project. I was looking into the issues and can't find any bug labeled ticket. I would like to start with small issue. How can I find such small work? If anyone suggests some work directly, I would also like to work on that. Open to anything :smile:

Laurențiu (Feb 22 2021 at 16:36, on Zulip):

There are some issues labelled as such -- https://github.com/rust-analyzer/rust-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 -- but most of these don't seem particularly easy

Laurențiu (Feb 22 2021 at 16:38, on Zulip):

You can take #7526 (though it might be too easy?) It's been partly done in https://github.com/rust-analyzer/rust-analyzer/pull/7707

Laurențiu (Feb 22 2021 at 16:44, on Zulip):

There's a PR open for it, but it's not exactly right

Chetan Khilosiya (Feb 22 2021 at 16:50, on Zulip):

I did look for good first issue, but found only 1 issue that was not actionable. Thank you @Laurențiu Nicola for the issue suggestion. I will look into #7526 issue.

Laurențiu (Feb 22 2021 at 16:50, on Zulip):

Maybe https://github.com/rust-analyzer/rust-analyzer/issues/3154 too

Laurențiu (Feb 22 2021 at 16:51, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/issues/7731 if you have a bit of experience with GitHub Actions

Chetan Khilosiya (Feb 22 2021 at 17:09, on Zulip):

Hi @Laurențiu Nicola, issue #7526 is already closed and pull/7707 PR is merged. looking into #3154 issue.

Laurențiu (Feb 22 2021 at 17:11, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/issues/7526

Lukas Wirth (Feb 22 2021 at 17:11, on Zulip):

wrong repo :big_smile: https://github.com/rust-analyzer/rust-analyzer/issues/7526

Lukas Wirth (Feb 22 2021 at 17:12, on Zulip):

linking issue #numbers directly seems to be a bad idea for this stream :big_smile:

Laurențiu (Feb 22 2021 at 17:13, on Zulip):

rust-analyzer#7526 also works

Laurențiu (Feb 22 2021 at 17:13, on Zulip):

I wish Zulip had per-stream linkifier settings

Lukas Wirth (Feb 22 2021 at 17:14, on Zulip):

another rather simple issue with rough instructions is here https://github.com/rust-analyzer/rust-analyzer/issues/7693

Chetan Khilosiya (Feb 22 2021 at 18:10, on Zulip):

Hi all, while working on renaming crate assists to ide_assists. after renaming there are multiple warngings not related to renaming. (clippy warnings) what should I do?

Florian Diebold (Feb 22 2021 at 18:16, on Zulip):

You can ignore clippy warnings, we don't generally have a policy of having zero clippy warnings (https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/style.md#clippy)

Chetan Khilosiya (Feb 22 2021 at 18:17, on Zulip):

ok

Chetan Khilosiya (Feb 22 2021 at 19:39, on Zulip):

I have done the changes for #7526 and created pull request https://github.com/rust-analyzer/rust-analyzer/pull/7759. Thank you guys for the help. This is my first contribution to this project. :big_smile:

Laurențiu (Feb 23 2021 at 08:26, on Zulip):

You could also give https://github.com/rust-analyzer/rust-analyzer/issues/7708 a try

Chetan Khilosiya (Feb 23 2021 at 16:32, on Zulip):

Hi @Laurențiu Nicola , I will work on #7708.

Laurențiu (Feb 23 2021 at 16:37, on Zulip):

You mean rust-analyzer#7708 :D

Laurențiu (Feb 23 2021 at 16:38, on Zulip):

See https://github.com/rust-analyzer/rust-analyzer/tree/master/docs/dev#how-to- and https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/architecture.md

Chetan Khilosiya (Feb 23 2021 at 16:42, on Zulip):

Ohh I got it now. The #<number> tag directly links to rust project and not rust-analyzer. Thank you for the docs links.

Laurențiu (Feb 23 2021 at 16:43, on Zulip):

Yeah, I made the same mistake yesterday :-)

Chetan Khilosiya (Feb 23 2021 at 17:02, on Zulip):

my 1 observation is that https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/user/generated_assists.adoc this link is not working. same with generated_diagnostics.adoc in user manual page. Is this issue or am I missing something?

Laurențiu (Feb 23 2021 at 17:38, on Zulip):

That's https://rust-analyzer.github.io/manual.html#assists-code-actions

Laurențiu (Feb 23 2021 at 17:39, on Zulip):

The link is indeed broken (the file is in .gitignore), but don't worry about it. Once you have your assist, it's easy to generate the docs if you've followed the same convention

Chetan Khilosiya (Feb 24 2021 at 16:08, on Zulip):

ok

Chetan Khilosiya (Feb 24 2021 at 17:19, on Zulip):

I am working on adding Assist. For that I am learning the existing assists code. What is the best way to learn and debug Assists? Currently I have added the debug point, but it debugger never hits that point.

Laurențiu (Feb 24 2021 at 17:20, on Zulip):

I guess the best way to learn how to write on is reading https://github.com/rust-analyzer/rust-analyzer/pull/7535

Laurențiu (Feb 24 2021 at 17:21, on Zulip):

About the debugger, you might need a (different) extension. I think CodeLLDB works for me on Linux, Microsoft's C++ extension works on Windows and none of them on Mac :-). But I'm not sure, I find debuggers pretty painful to use in Rust, at least to some other non-native languages

Laurențiu (Feb 24 2021 at 17:22, on Zulip):

You might be better off using log::info! or log::error! to print the values you're interested in. https://tedspence.com/the-art-of-printf-debugging-7d5274d6af44 demonstrates that technique.

Chetan Khilosiya (Feb 24 2021 at 17:26, on Zulip):

Thank you @Laurențiu Nicola for that suggestion. I am using CodeLLDB but I also find that painful for rust. I am reading remove_unused_params assists currently as it is small code. I will look into extract function assist also afterwards.

Chetan Khilosiya (Feb 25 2021 at 14:40, on Zulip):

Do we have to set log file path or any default path is taken for debug build.

Laurențiu (Feb 25 2021 at 15:22, on Zulip):

If you're using Code, they'll show up in the Output window. But you might need to set RA_LOG=info or something like that

Chetan Khilosiya (Feb 27 2021 at 18:46, on Zulip):

Good morning guys. I am working on https://github.com/rust-analyzer/rust-analyzer/issues/7708 issue. I have created code for happy case scenario. Should I get that code reviewed first or implement all the checks and other cases code and then submit for reivew (via pull request)?

Laurențiu (Feb 27 2021 at 19:24, on Zulip):

You can file a draft PR. It may or may not be reviewed, depending on the availability of the reviewers :-)

Chetan Khilosiya (Feb 27 2021 at 19:30, on Zulip):

I did not consider the time and efforts required for reviewer. I will complete the work and then file PR. Thank you for the suggestion.

Laurențiu (Feb 27 2021 at 19:36, on Zulip):

It's all right either way, really, just no promises about reviewing it

Chetan Khilosiya (Feb 27 2021 at 20:52, on Zulip):

The codegen part generated wrong test case. Should I update that test case manually in generated file?

Chetan Khilosiya (Feb 27 2021 at 20:59, on Zulip):

I fixed the issue, there were many issues in documentation example code.

Chetan Khilosiya (Feb 27 2021 at 21:14, on Zulip):

I like the test suite created in this project. Checking every aspect of the code. :+1:

Lukas Wirth (Feb 27 2021 at 21:27, on Zulip):

left you some comments

Chetan Khilosiya (Mar 01 2021 at 16:11, on Zulip):

Lukas Wirth said:

left you some comments

Thank you @Lukas Wirth for the review and feedback. I will work on it.

Chetan Khilosiya (Mar 01 2021 at 19:23, on Zulip):

For Impl node in ast we don't have ast::NameOwner implemented for it. Any specific reason anyone know.

Lukas Wirth (Mar 01 2021 at 19:30, on Zulip):

Impl has no name hence it doesn't implement it

Chetan Khilosiya (Mar 01 2021 at 19:53, on Zulip):

so impl Example {} we dont consider Example as name of impl block?

Lukas Wirth (Mar 01 2021 at 19:54, on Zulip):

No, Example is a PathType in that case

Lukas Wirth (Mar 01 2021 at 19:55, on Zulip):

I gave you another comment on the PR btw, I unfortunately directed you into the wrong direction before it seems :sweat_smile:

Florian Diebold (Mar 01 2021 at 20:03, on Zulip):

the target of an impl block is a type, which can be other things than a simple name. For example, there's an impl<T> [T]

Chetan Khilosiya (Mar 01 2021 at 20:08, on Zulip):

@Lukas Wirth committed the changes. 1 more thing is pending that if Default impl is already implemented then skip the assist. I will work on that later.

Lukas Wirth (Mar 01 2021 at 20:10, on Zulip):

Alright, for that you will probably have to use the Semantics type since you will have to query whether the type already implements Default(as searching for the Default impl in the AST won't cover everything).

Chetan Khilosiya (Mar 01 2021 at 20:11, on Zulip):

thanks for the pointer. I will look into Semantics. that one I kept pending cause I wasn't sure about the logic.

Chetan Khilosiya (Mar 01 2021 at 20:15, on Zulip):

for formatting this project are we using rustfmt or rust-analyzer formatter?

Joshua Nelson (Mar 01 2021 at 20:16, on Zulip):

rust-analyzer uses rustfmt under the hood

Chetan Khilosiya (Mar 01 2021 at 20:17, on Zulip):

so they both are exactly same or rust-analyzer add some extra logic?

Lukas Wirth (Mar 01 2021 at 20:23, on Zulip):

rust-analyzer has no formatter on its own, so its just rustfmt

Lukas Wirth (Mar 01 2021 at 20:23, on Zulip):

a future rust-analyzer will hopefully have its own formatter

Joshua Nelson (Mar 01 2021 at 20:38, on Zulip):

Lukas Wirth said:

a future rust-analyzer will hopefully have its own formatter

why hopefully?

Lukas Wirth (Mar 01 2021 at 20:41, on Zulip):

So we can format our syntax trees in memory for assists and such

Lukas Wirth (Mar 01 2021 at 20:41, on Zulip):

Didn't mean to imply rustfmt being bad or anything :big_smile:

Joshua Nelson (Mar 01 2021 at 20:43, on Zulip):

nah I'm just trying to understand :)

Joshua Nelson (Mar 01 2021 at 20:43, on Zulip):

do you specifically want your own formatter? Or just a formatting library, the way chalk works?

Joshua Nelson (Mar 01 2021 at 20:43, on Zulip):

I think it's plausible for rustfmt to use syn/quote instead of rustc eventually

Lukas Wirth (Mar 01 2021 at 20:45, on Zulip):

I don't think that will help here, see https://github.com/rust-analyzer/rust-analyzer/issues/1665 and https://github.com/rust-analyzer/rust-analyzer/issues/923

Lukas Wirth (Mar 01 2021 at 20:45, on Zulip):

the goal is to be able to directly format our syntax trees so we can emit well formatted edits and such

Lukas Wirth (Mar 01 2021 at 20:45, on Zulip):

from what i remember

Joshua Nelson (Mar 01 2021 at 20:45, on Zulip):

oh man pattern-based formatting sounds amazing :D

Joshua Nelson (Mar 01 2021 at 20:45, on Zulip):

I've always wanted that in rust

Laurențiu (Mar 01 2021 at 20:46, on Zulip):

We try to build on stable, so a prerequisite for that would be rustfmt and rust-analyzer sharing the parser and maybe AST types

Chetan Khilosiya (Mar 04 2021 at 18:30, on Zulip):

Hi guys, I have updated the implementation for https://github.com/rust-analyzer/rust-analyzer/issues/7708 issue.
and completed it. Please have a look at pull request https://github.com/rust-analyzer/rust-analyzer/pull/7800 when you have free time.

Chetan Khilosiya (Mar 05 2021 at 16:56, on Zulip):

Hi @Lukas Wirth , I had tried your approach, but for me the deafault trait as well as std create comes None from FamousDefs.

Lukas Wirth (Mar 05 2021 at 16:58, on Zulip):

Oh right, that is because by default we do not test with std/core enabled, so they will always come out as None, there is a way to add them to your test fixtures though, give me a sec so I can find an example.

Lukas Wirth (Mar 05 2021 at 17:02, on Zulip):

Basically you want to wrap the check_assist function in your tests to inject the FamousDefs fixture for you.

    fn check(ra_fixture_before: &str, ra_fixture_after: &str) {
        let before = &format!(
            "//- /main.rs crate:main deps:core{}{}",
            before,
            FamousDefs::FIXTURE,
        );
        check_assist(generate_default_from_new, before, after);
    }
Lukas Wirth (Mar 05 2021 at 17:02, on Zulip):

With that you can call check instead of check-assist in your tests and it should work

Lukas Wirth (Mar 05 2021 at 17:02, on Zulip):

You can see this trick being used here as well https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide_assists/src/handlers/replace_for_loop_with_for_each.rs#L146-L154

Chetan Khilosiya (Mar 05 2021 at 17:10, on Zulip):

Thank you @Lukas Wirth I will update the code and submit for review.

Chetan Khilosiya (Mar 05 2021 at 17:48, on Zulip):

Hi @Lukas Wirth, with the fixture added in test code. all tests are failing. I will debug that and get back to you tomorrow.

Chetan Khilosiya (Mar 06 2021 at 17:39, on Zulip):

Hi all,
I have written code to check if trait is implemented for not for struct as below. But even though trait is implemented then also it is giving me false.

fn is_default_implemented(ctx: &AssistContext, impl_: &Impl) -> Option<bool> {
    let db = ctx.sema.db;
    let impl_def = ctx.sema.to_def(impl_)?;
    let ty = impl_def.target_ty(db);
    let krate = impl_def.module(db).krate();
    let default_trait = FamousDefs(&ctx.sema, Some(krate)).core_default_Default()?;
    let implements_default = ty.impls_trait(db, default_trait, &[]);
    Some(implements_default)
}

I have checked that default_trait value has data. But impls_trait method giving false after going in salsa code.

Chetan Khilosiya (Mar 06 2021 at 18:01, on Zulip):

The above code failing for below test

struct Example { _inner: () }

impl Exmaple {
    pub fn n$0ew() -> Self {
        Self { _inner: () }
    }
}

impl Default for Example {
    fn default() -> Self {
        Self::new()
    }
}

but succeeding for below code

mod test {
    struct Example { _inner: () }

    impl Example {
        pub fn n$0ew() -> Self {
            Self { _inner: () }
        }
    }

    impl Default for Example {
        fn default() -> Self {
            Self::new()
        }
    }
}
Lukas Wirth (Mar 06 2021 at 18:22, on Zulip):

You seem to have typoed Example in the first snippet(you did as well in the test) so I imagine thats the problem
you wrote impl Exmaple { instead of impl Example {

Lukas Wirth (Mar 06 2021 at 18:23, on Zulip):

If the problem still persists though I'll check your PR out locally and see whats wrong later

Chetan Khilosiya (Mar 06 2021 at 18:35, on Zulip):

That was the issue. Silly mistake. Thank you @Lukas Wirth

Chetan Khilosiya (Mar 06 2021 at 19:30, on Zulip):

Hi @Lukas Wirth, I have committed the updated code.

Chetan Khilosiya (Mar 06 2021 at 20:01, on Zulip):

Hi @Lukas Wirth, I have updated the code with your review comments.

Chetan Khilosiya (Mar 06 2021 at 20:04, on Zulip):

Thank you very much @Lukas Wirth for all the help and hand holding for my 1st task. :smiley:

Chetan Khilosiya (Mar 06 2021 at 20:06, on Zulip):

I will be starting work on https://github.com/rust-analyzer/rust-analyzer/issues/7709, which is simillar task. Hopefully I will require less help :grinning_face_with_smiling_eyes:

Chetan Khilosiya (Mar 06 2021 at 20:09, on Zulip):

1 doubt about https://github.com/rust-analyzer/rust-analyzer/issues/7709.
the generated code for is_empty function is like

pub fn is_empty() -> bool {
    self.data.len() == 0
}

instead of this can we generate

pub fn is_empty() -> bool {
    self::len() == 0
}

as the implementation in len() function can be complex.

Lukas Wirth (Mar 06 2021 at 20:10, on Zulip):

I think that's an oversight in the issue, that should definitely generate self.len() == 0,

Lukas Wirth (Mar 06 2021 at 20:11, on Zulip):

I fixed the issue code snippets

Lukas Wirth (Mar 06 2021 at 20:13, on Zulip):

You probably want to check out this https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide_assists/src/handlers/replace_for_loop_with_for_each.rs#L74-L102
since you want to check if the type already has an is_empty method or not, you can do that similar to there

Chetan Khilosiya (Mar 06 2021 at 20:14, on Zulip):

ok

Chetan Khilosiya (Mar 06 2021 at 20:21, on Zulip):

while working on other issue. should I continue with master branch (synced with upstream). or use different branch for different issue?

Lukas Wirth (Mar 06 2021 at 20:24, on Zulip):

Usually you'd want to make a branch per PR

Chetan Khilosiya (Mar 06 2021 at 20:24, on Zulip):

ok

Chetan Khilosiya (Mar 11 2021 at 18:33, on Zulip):

cargo xtask code giving error that unexpected argument: "codegen".
Is anything changed. The xtask help message also does not show codegen.

detrumi (Mar 11 2021 at 18:34, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/PSA.3A.20.60cargo.20xtask.20codegen.60.20is.20no.20more/near/229353544

detrumi (Mar 11 2021 at 18:34, on Zulip):

You don't need cargo xtask codegen any more

Chetan Khilosiya (Mar 11 2021 at 18:38, on Zulip):

my cargo test is failing. I already have 1 generated file. Should I delete that and then run cargo test?

Chetan Khilosiya (Mar 11 2021 at 18:57, on Zulip):

Fixed it.

Chetan Khilosiya (Mar 11 2021 at 19:04, on Zulip):

I have completed the work on https://github.com/rust-analyzer/rust-analyzer/issues/7709. and created PR https://github.com/rust-analyzer/rust-analyzer/pull/7977

Chetan Khilosiya (Mar 15 2021 at 17:23, on Zulip):

Hi all. I have worked on adding is_empty from len assist. Please have a look at PR https://github.com/rust-analyzer/rust-analyzer/pull/8037.

Hi @Lukas Wirth , sorry that the old PR is closed. this happened mistakenly while cleaning remote branches I deleted my own branch. I have created new PR.

Lukas Wirth (Mar 15 2021 at 17:32, on Zulip):

No worries :big_smile:

Chetan Khilosiya (Mar 15 2021 at 18:32, on Zulip):

Hi @Lukas Wirth , thank you for the review. Should I pick up the work of adding return type check for generate is_empty assits?

Lukas Wirth (Mar 15 2021 at 18:33, on Zulip):

Feel free to, it shouldn't be that much work actually as I've noticed

Lukas Wirth (Mar 15 2021 at 18:34, on Zulip):

Basically you can just add

    pub fn is_unsigned_integer(&self) -> bool {
        matches!(self.ty.value.interned(&Interner), TyKind::Scalar(Scalar::Uint(_)))
    }

to hir::Type to check if a type is an unsigned int, the rest should be simple enough as well I think

Chetan Khilosiya (Mar 15 2021 at 18:34, on Zulip):

Great. I will work on it. Thanks for the code.

Chetan Khilosiya (Mar 15 2021 at 19:05, on Zulip):

Hi @Lukas Wirth , I have few doubts.
In cargo clippy documentation they use only usize. Should we restrict type check to usize?
the unsigned int is more useful, but then should we expand that to include signed ints also?

Chetan Khilosiya (Mar 15 2021 at 19:07, on Zulip):

other places in rust like ExactSizeIterator, the len function generally have usize return type.

Lukas Wirth (Mar 15 2021 at 19:12, on Zulip):

Just usize seems fine as well, I figured it might make sense to allow all unsigned ints.

Chetan Khilosiya (Mar 15 2021 at 19:14, on Zulip):

unsigned int will certainly give more freedom to apply it.

Laurențiu (Mar 15 2021 at 19:16, on Zulip):

I wanted to say that we sometimes use u32 lengths or offsets to save memory, but those are either a struct (like TextSize) or usize. Even if you do that, there's not much reason to return anything else than usize from len(). So it's fine to only trigger it on usize.

Lukas Wirth (Mar 15 2021 at 19:17, on Zulip):

Right storing and returning are different matters I suppose, then only usize seems better

Laurențiu (Mar 15 2021 at 19:19, on Zulip):

And we can change it if someone files an issue :-)

Chetan Khilosiya (Mar 15 2021 at 19:49, on Zulip):

I have created PR https://github.com/rust-analyzer/rust-analyzer/pull/8040
@Lukas Wirth please have a look at it when you get time.

Chetan Khilosiya (Mar 17 2021 at 16:36, on Zulip):

Hi everyone. I have gone through all good first issue and I can't find the task that either other people are not working on or either not easy to low medium difficulty level. Please suggest me the issue to work on.

Chetan Khilosiya (Mar 17 2021 at 16:40, on Zulip):

I will also take any maintainance work.

Chetan Khilosiya (Mar 17 2021 at 16:43, on Zulip):

is anyone working on this issue: https://github.com/rust-analyzer/rust-analyzer/issues/4613?

Lukas Wirth (Mar 17 2021 at 16:48, on Zulip):

Nope, that should be free to work on.

Lukas Wirth (Mar 17 2021 at 16:50, on Zulip):

relevant highlighting happens here, https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide/src/syntax_highlighting/highlight.rs

Laurențiu (Mar 17 2021 at 16:53, on Zulip):

Looks like we now emit function .attribute for the attribute and attribute .attribute for the arguments

Lukas Wirth (Mar 17 2021 at 16:56, on Zulip):

Right, I'm not sure if thats too relevant now anymore actually

Lukas Wirth (Mar 17 2021 at 16:57, on Zulip):

You can try tackling this though I think https://github.com/rust-analyzer/rust-analyzer/issues/8024

Lukas Wirth (Mar 17 2021 at 16:57, on Zulip):

also semantic highlighting related

Chetan Khilosiya (Mar 17 2021 at 17:00, on Zulip):

sure, I will look into issue 8024

Chetan Khilosiya (Mar 20 2021 at 16:14, on Zulip):

About https://github.com/rust-analyzer/rust-analyzer/issues/8024. To clarify requirements, where the trait is implemented, assign trait modifier to functions and variables that is used in trait.
For this I will need to add new modifier trait right?

Lukas Wirth (Mar 20 2021 at 16:27, on Zulip):

Yes we want a new modifier for this

Lukas Wirth (Mar 20 2021 at 16:29, on Zulip):

The modifier should be applied to the Name/NameRefs of associated trait items

Lukas Wirth (Mar 20 2021 at 16:29, on Zulip):

as shown in the example in the issue

Lukas Wirth (Mar 20 2021 at 16:29, on Zulip):

So this works similar to the Associated modifier we already have

Lukas Wirth (Mar 20 2021 at 16:29, on Zulip):

just that it only applies to trait impls/defs

Chetan Khilosiya (Mar 20 2021 at 16:51, on Zulip):

ok

Ayomide Bamidele (Mar 28 2021 at 22:50, on Zulip):

Hi! I was just looking at some first issues, and this channel. I love rust-analyzer and have been looking at ways to contribute :) I'm not sure if there has been any movement on https://github.com/rust-analyzer/rust-analyzer/issues/8024 mentioned in the above messages. But I had a look as an exercise of reading the code and want to check my understanding to learn. So to implement this feature, after looking at what I think are the relevant areas you'd need to:

Lukas Wirth (Mar 28 2021 at 23:19, on Zulip):

Yep that's pretty much what it takes, this is basically just a special case for HlMod::Associated + highlighting the trait def. So the main difference is checking where the assoc item lives in.

Lukas Wirth (Mar 28 2021 at 23:25, on Zulip):

If you want I can go look for some good first issues for you

Ayomide Bamidele (Mar 28 2021 at 23:52, on Zulip):

Lukas Wirth said:

If you want I can go look for some good first issues for you

Sure! I was having a look but wasn't sure what to choose. Thanks for confirming that I was on the right track!

Lukas Wirth (Mar 28 2021 at 23:55, on Zulip):

A simple one should be https://github.com/rust-analyzer/rust-analyzer/issues/8114 I think

Lukas Wirth (Mar 28 2021 at 23:55, on Zulip):

relevant file for that is https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ide/src/folding_ranges.rs

Lukas Wirth (Mar 28 2021 at 23:56, on Zulip):

The logic should be very similar to how use statement groups are handled I believe

Lukas Wirth (Mar 28 2021 at 23:57, on Zulip):

or to comment groups as well

Ayomide Bamidele (Mar 29 2021 at 09:55, on Zulip):

Looks interesting! I'll have a go at it

Ayomide Bamidele (Mar 29 2021 at 11:47, on Zulip):

I've made a draft PR (https://github.com/rust-analyzer/rust-analyzer/pull/8231) here that has tests, but I'm running into an issue with actually trying it out for myself. I tried to use the Run Installed Extensionlaunch config, but I get an error [matklad.rust-analyzer]: 'configuration.property' must be an object that I'm not sure how to go about fixing

Laurențiu (Mar 29 2021 at 11:48, on Zulip):

You can safely ignore that one.

Ayomide Bamidele (Mar 29 2021 at 11:53, on Zulip):

Oh, then I also get a bootstrap error that stops the extension from activating, the error isn't very descriptive so not sure what to do about it EDIT: Nevermind! Got it working with the Run Debug instead. PR should be done now, that was fun!

Chetan Khilosiya (Mar 29 2021 at 16:49, on Zulip):

I have 1 question. Do rust analyzer index all used crates and its dependencies on startup?

bjorn3 (Mar 29 2021 at 17:02, on Zulip):

Yes

Chetan Khilosiya (Mar 29 2021 at 17:03, on Zulip):

why do we index dependencies of used crates also? Can we directly use dependencies in our code without adding them to cargo.toml?

Chetan Khilosiya (Mar 29 2021 at 17:17, on Zulip):

Based on the basic search, I think we can't use dependencies of creates unless they are explicitly added in cargo.toml. So we should be able to ignore the dependencies of used creates. This will speed up the startup time. but more importantly will have benefit of reduced memory usage.
Is this possible?

bjorn3 (Mar 29 2021 at 18:57, on Zulip):

We can't ignore dependencies of used crates, as those crates need the dependencies to be analyzed.

Florian Diebold (Mar 29 2021 at 19:00, on Zulip):

yes, there are two main reasons we can't just ignore transitive dependencies: 1. we need to macroexpand the direct dependencies, which means resolving macros from transitive dependencies, which means macroexpanding them in turn, ...; 2. direct dependencies might reexport things from transitive dependencies

Chetan Khilosiya (Mar 29 2021 at 19:01, on Zulip):

why we do macro expand? to do highlight or more than that?

bjorn3 (Mar 29 2021 at 19:01, on Zulip):

Macros can expand to types and functions.

Ayomide Bamidele (Mar 29 2021 at 19:05, on Zulip):

One question, in reference to this issue https://github.com/rust-analyzer/rust-analyzer/issues/6539#issuecomment-809577794, how do you build a private version of the manual so that you can preview changes to docs? I can see a preview in vscode but it doesn't have the website's css and all that stuff found it, was right on the page haha

Chetan Khilosiya (Mar 29 2021 at 19:06, on Zulip):

Ok. got the idea now. so for dependencies of crates we are limiting processing for macro expansion and re-exported things.

Laurențiu (Mar 29 2021 at 19:12, on Zulip):

I'm not sure we do that

Chetan Khilosiya (Mar 30 2021 at 20:33, on Zulip):

Hi @Lukas Wirth , can you please help me fix the macos-latest build failing?
The reason is test_format_document_unchanged is failing, but on my machine all tests are passing.

Chetan Khilosiya (Mar 30 2021 at 20:34, on Zulip):

Whenever I get the permission to merge request myself, the macos-latest build fails :sweat_smile:

Lukas Wirth (Mar 30 2021 at 20:35, on Zulip):

That seems to be just unlucky, it happens sometimes and shouldnt be caused by your changes

Lukas Wirth (Mar 30 2021 at 20:36, on Zulip):

Im unsure why it even fails sometimes in the first place though

Chetan Khilosiya (Mar 30 2021 at 20:37, on Zulip):

I am also curious about it, cause PR does not contain any platform dependent code. If it passes on 2 OS build then...

Lukas Wirth (Mar 30 2021 at 20:43, on Zulip):

Well bors merged it so there you have it :big_smile:

Lukas Wirth (Mar 30 2021 at 20:45, on Zulip):

The spurious mac-os error is quite weird though as it errors on a non-empty directory, but the function that errors is fs::remove_dir_all which to my knowledge is supposed to clear the directory prior to deleting it?

Chetan Khilosiya (Mar 30 2021 at 20:50, on Zulip):

:thinking:

bjorn3 (Mar 30 2021 at 20:51, on Zulip):

fs::remove_dir_all will fail when a new file is created after it removed all containing files, but before removing the dir itself I think.

Chetan Khilosiya (Mar 30 2021 at 20:54, on Zulip):

In PR I committed 1 new file, that I removed later as not needed. Can that cause the issue?

Lukas Wirth (Mar 30 2021 at 20:56, on Zulip):

No, your PR is not relevant for this issue, I have seen it in a few PRs already, sometimes on normal CI sometimes when bors was trying to merge

Lukas Wirth (Mar 30 2021 at 20:56, on Zulip):

What bjorn3 said seems the most likely to me, I wonder if thats why on windows we call the function serveral times :thinking:

Lukas Wirth (Mar 30 2021 at 20:57, on Zulip):

It seems weird though, judging from drop order TestDir gets dropped in the offending test last so this shouldn't be happening

Lukas Wirth (Mar 30 2021 at 20:58, on Zulip):

<https://github.com/rust-analyzer/rust-analyzer/runs/2231268344#step:9:2846> the backtrace as reference

Chetan Khilosiya (Apr 02 2021 at 18:16, on Zulip):

Can I take this issue: https://github.com/rust-analyzer/rust-analyzer/issues/8279?

Chetan Khilosiya (Apr 02 2021 at 19:07, on Zulip):

How should I categorize 54 operators?
https://doc.rust-lang.org/book/appendix-02-operators.html

Mario Carneiro (Apr 02 2021 at 19:09, on Zulip):

I like those kinds of tables to be categorized by precedence and then by function

Mario Carneiro (Apr 02 2021 at 19:10, on Zulip):

they currently appear to be ascii sorted

Chetan Khilosiya (Apr 02 2021 at 19:13, on Zulip):

Please look into issue https://github.com/rust-analyzer/rust-analyzer/issues/8279

Chetan Khilosiya (Apr 02 2021 at 19:14, on Zulip):

I need to provide different syntax highlighting to different operators. as there are many operators we need to come up with some categories

Chetan Khilosiya (Apr 02 2021 at 19:14, on Zulip):

I don't think user wants 54 different tokens for operators.

Mario Carneiro (Apr 02 2021 at 19:16, on Zulip):

One reasonably useful categorization is to separate out the binary operators from other things like , for tuples, : for struct literals, @ in patterns

Mario Carneiro (Apr 02 2021 at 19:18, on Zulip):

another would be assignments, logical operators, bitwise operators, arithmetic ops, ranges

Mario Carneiro (Apr 02 2021 at 19:19, on Zulip):

and there are a few that probably deserve their own class: & for reference, . for field/method projection, ? for try, ! for macros

Mario Carneiro (Apr 02 2021 at 19:21, on Zulip):

it's also potentially useful to separate * in *const, + in trait + trait, and -> in function types in a class separate from the normal binary op + and * for pointers

Chetan Khilosiya (Apr 03 2021 at 14:26, on Zulip):

Should we have same tag for assign shorthand as original operator?
Ex. + and += will have same tag as Arithmetic. | and |= have same tag as bitwise

Lukas Wirth (Apr 03 2021 at 14:47, on Zulip):

I'd say for the time being trat them the same yes, it might make sense to give them modifiers for assign shorthand but I wouldn't worry about that for now.

Chetan Khilosiya (Apr 03 2021 at 14:50, on Zulip):

ok

Chetan Khilosiya (Apr 05 2021 at 18:58, on Zulip):

I am implementing the semantic highlighting for operators.
For ! operator we have two cases !<bool> this will be logical operator and !<u32> this will be bitwise operator. How can I get what type of variable is in PrefixExpr?

Lukas Wirth (Apr 05 2021 at 19:00, on Zulip):

Semantics has a function to get the type of an expression, so you can do something along the lines of sema.type_of_expr(prefix_expr.expr())

Lukas Wirth (Apr 05 2021 at 19:02, on Zulip):

But rust doesn't really discern between logical not and bitwise not on a language level, so I'm not sure if it makes sense to try to differentiate them here? Especially given with user implementations, if I have some struct Foo and implement not on it, is that a logical or bitwise not then, you can't really know.

Lukas Wirth (Apr 05 2021 at 19:02, on Zulip):

And the docs also call std::ops::Not a logical not, so I would say it should always be marked as a logical not

The unary logical negation operator !.

Chetan Khilosiya (Apr 05 2021 at 19:04, on Zulip):

that makes it easy. Thank you :)

Chetan Khilosiya (Apr 08 2021 at 17:44, on Zulip):

Created PR: https://github.com/rust-analyzer/rust-analyzer/pull/8354

Chetan Khilosiya (Apr 08 2021 at 17:44, on Zulip):

Can I take this issue?

Chetan Khilosiya (Apr 08 2021 at 17:44, on Zulip):

https://github.com/rust-analyzer/rust-analyzer/issues/8425

Laurențiu (Apr 08 2021 at 17:45, on Zulip):

For that one you'd need to enable onEnter as described in the manual and check if it works for //! comments as well

Laurențiu (Apr 08 2021 at 17:45, on Zulip):

If it works, I'm inclined to close it

Chetan Khilosiya (Apr 08 2021 at 17:47, on Zulip):

checking it.

Chetan Khilosiya (Apr 08 2021 at 17:54, on Zulip):

surprisingly the the onEnter customization works for /// and //! but not for //. And in documentation the case for // is shown.

Laurențiu (Apr 08 2021 at 17:55, on Zulip):

I think that's intended, you need to end the previous line with a space (?) for it to work

Laurențiu (Apr 08 2021 at 17:56, on Zulip):

Enter in the middle or after a trailing space in // inserts //

Chetan Khilosiya (Apr 08 2021 at 18:01, on Zulip):

The enter in the middle is useful but space (?) is somewhat ugly to use. cause you need to press enter after (? this moves closing parenthesis to new line ). If you move to end of (?) the it does not add // in new line.

Chetan Khilosiya (Apr 08 2021 at 18:01, on Zulip):

Anyway all documented cases are working. Good to close the ticket.

Laurențiu (Apr 08 2021 at 18:02, on Zulip):

Sorry, it's not literally (?), it's just a trailing space -- you can almost see it in the GIF.

Laurențiu (Apr 08 2021 at 18:02, on Zulip):

I wrote "(?)" because I wasn't sure about it

Chetan Khilosiya (Apr 08 2021 at 18:03, on Zulip):

why don't we add this feature in plugin itself. Many people including myself would not know this exists. This is very good feature to have.
Edit: grammar

Chetan Khilosiya (Apr 08 2021 at 18:03, on Zulip):

ohh I got that, it makes sense now. :)

Laurențiu (Apr 08 2021 at 18:04, on Zulip):

See https://github.com/rust-analyzer/rust-analyzer/pull/6037

Laurențiu (Apr 08 2021 at 18:04, on Zulip):

Basically, it was enabled by default and a lot of people complained that it slowed their typing

Laurențiu (Apr 08 2021 at 18:05, on Zulip):

And if RA crashes or hangs, you can't press enter any more (Shift-Enter works IIRC, but most users probably don't know that)

Laurențiu (Apr 08 2021 at 18:06, on Zulip):

It's slow because the keypresses need to go through the extension host instead of being handled directly by the editor. And the extension host can be busy doing anything else (we might not be the only extension running).

Laurențiu (Apr 08 2021 at 18:07, on Zulip):

Maybe you want to take the rest of https://github.com/rust-analyzer/rust-analyzer/issues/8024#issuecomment-813236524?

Chetan Khilosiya (Apr 08 2021 at 18:10, on Zulip):

Laurențiu said:

And if RA crashes or hangs, you can't press enter any more (Shift-Enter works IIRC, but most users probably don't know that)

Hmm. It looks like this feature can be added in VSCode extension in typescript. Which would solve many (all?) the problems.

Chetan Khilosiya (Apr 08 2021 at 18:11, on Zulip):

Laurențiu said:

Maybe you want to take the rest of https://github.com/rust-analyzer/rust-analyzer/issues/8024#issuecomment-813236524?

Great, I will pickup this work.

Laurențiu (Apr 08 2021 at 18:11, on Zulip):

It can be added in package.json, but matklad is against that because it's more or less a hack (if you're in a raw string or a macro you might not want the comment continuation, but you can't check for that with a regex)

Daniel Mcnab (Apr 08 2021 at 18:31, on Zulip):

Is it possible to detect the semantic highlighting token the cursor is in?

Laurențiu (Apr 08 2021 at 18:32, on Zulip):

Programatically or for testing? Code has a command to show them.

Daniel Mcnab (Apr 08 2021 at 18:33, on Zulip):

Yeah, in the vscode client api. Because in theory, we could have a good guess regex, then check the token type if the regex match occurs.

Laurențiu (Apr 08 2021 at 18:34, on Zulip):

Maybe, but you still need to run extension code for that

Chetan Khilosiya (Apr 08 2021 at 18:36, on Zulip):

In the documentation for on enter, should we add //! also?

Daniel Mcnab (Apr 08 2021 at 18:36, on Zulip):

My point is thought that @matklad said:

I guess, in practice, maybe we want to cave in and use the simple regex handler here?

in https://github.com/rust-analyzer/rust-analyzer/pull/6037#issuecomment-698976840
Which suggests that using the regex handler would in some way be better
So my suggestion is to weed out the false positives from the 'regex handler', whatever that actually means

Laurențiu (Apr 08 2021 at 18:37, on Zulip):

The regex handler is the one configured in package.json and is run automatically by Code (outside of the extension host)

Laurențiu (Apr 08 2021 at 18:37, on Zulip):

So it's fast, but less elegant and might have some false positives

Daniel Mcnab (Apr 08 2021 at 18:38, on Zulip):

Ah, so it handles the enhanced enter itself? We should ask upstream to allow enabling it on based on semantic token types instead then?

Laurențiu (Apr 08 2021 at 18:38, on Zulip):

That might be one way to do it, AIUI

Laurențiu (Apr 08 2021 at 18:39, on Zulip):

To be honest, it wouldn't be too bad, even without taking into account semantic tokens or anything. Even if it has some false positives in a macro, it's still better than not having it at all (or randomly slowing down typing).

Daniel Mcnab (Apr 08 2021 at 18:46, on Zulip):

Laurențiu said:

The regex handler is the one configured in package.json and is run automatically by Code (outside of the extension host)

Can you link the documentation of this please?

Daniel Mcnab (Apr 08 2021 at 18:49, on Zulip):

Right, I've found this https://github.com/microsoft/vscode/blob/94c9ea46838a9a619aeafb7e8afd1170c967bb55/src/vs/workbench/contrib/codeEditor/browser/languageConfigurationExtensionPoint.ts#L701

Chetan Khilosiya (Apr 08 2021 at 19:04, on Zulip):

Created PR for updating on enter documentation with added test case https://github.com/rust-analyzer/rust-analyzer/pull/8429

Chetan Khilosiya (Apr 08 2021 at 19:41, on Zulip):

Created PR for highlighting trait associated type with modifier. https://github.com/rust-analyzer/rust-analyzer/pull/8431

Chetan Khilosiya (May 06 2021 at 15:53, on Zulip):

Good morning everyone...
Can I take this issue: https://github.com/rust-analyzer/rust-analyzer/issues/8734

Lukas Wirth (May 06 2021 at 15:59, on Zulip):

All yours

Chetan Khilosiya (May 06 2021 at 16:01, on Zulip):

Great :). I was sick so took break from project.

Chetan Khilosiya (May 08 2021 at 13:54, on Zulip):

Has anyone faced error: internal compiler error: unexpected panic recently with rustc 1.52.0?

Chetan Khilosiya (May 08 2021 at 13:58, on Zulip):

compiler_error.txt

Chetan Khilosiya (May 08 2021 at 14:08, on Zulip):

causing compiler panic with simple change
change.diff

Lukas Wirth (May 08 2021 at 14:10, on Zulip):

that's a known issue ye https://github.com/rust-lang/rust/issues/85003

Chetan Khilosiya (May 08 2021 at 18:53, on Zulip):

This is out of curiosity, regarding SmolStr implementation.
The new_inline function is written as

#[inline]
    pub const fn new_inline(text: &str) -> SmolStr {
        let mut buf = [0; INLINE_CAP];
        let mut i = 0;
        while i < text.len() {
            buf[i] = text.as_bytes()[i];
            i += 1
        }
        SmolStr(Repr::Inline {
            len: text.len() as u8,
            buf,
        })
    }

here why the text.as_bytes() function is in the while loop and not outside the loop. Does making it inline optimizes this point.
As the as_bytes() on str converts complete str to bytes in 1 go.

Laurențiu (May 08 2021 at 18:57, on Zulip):

as_bytes() doesn't do much, so I'm guessing it would make no difference: https://doc.rust-lang.org/src/core/str/mod.rs.html#224

Laurențiu (May 08 2021 at 18:57, on Zulip):

But yeah, that could maybe look cleaner

Chetan Khilosiya (May 08 2021 at 19:08, on Zulip):

mem::transmute is single memcpy under the hood. So I think it does not matter for single string (regarding of its length.) But I think for large number of strings calling memcpy 1 time or n times (max 22) will make difference.

Chetan Khilosiya (May 08 2021 at 19:09, on Zulip):

it is useful to add PR for this?

Daniel Mcnab (May 09 2021 at 07:28, on Zulip):

I'd expect it to be trivially optimised out - you could check the assembly to see though

Laurențiu (May 09 2021 at 07:54, on Zulip):

That transmute only transmutes a reference (16 bytes), not the whole string

Chetan Khilosiya (May 09 2021 at 11:54, on Zulip):

str.as_bytes() converts the whole string. And we are using this method.

Florian Diebold (May 09 2021 at 12:10, on Zulip):

there's no conversion necessary, it's just casting a pointer

Chetan Khilosiya (May 09 2021 at 12:11, on Zulip):

ohh, that makes it clear.

Chetan Khilosiya (May 09 2021 at 14:36, on Zulip):

Hi, I am working on issue: https://github.com/rust-analyzer/rust-analyzer/issues/8734
Currently the CrateDisplayName provides the name. It has crate_name and canonical_name as fields.
So do I put custom lib name in crate_name or canonical_name?

Chetan Khilosiya (May 15 2021 at 18:24, on Zulip):

Hi all. Unfortunately my laptop has stopped working. It was old so no regrets there. but I won't be able to get new one for atleast 3-4 months due to covid 19 and other conditions. So please some one take the issue 8734 as I won't be able to contribute.

Chetan Khilosiya (May 15 2021 at 18:25, on Zulip):

For compiling RA with lto enabled, is 16gb ram enough or it will require 32gb ram?

Chetan Khilosiya (May 15 2021 at 18:26, on Zulip):

any other suggestion for new laptop??

Laurențiu (May 15 2021 at 18:28, on Zulip):

16 GB should be enough. I tried to test now, but the RAM usage seemed pretty low for some reason

Chetan Khilosiya (May 15 2021 at 18:29, on Zulip):

Thank you

Laurențiu (May 15 2021 at 18:30, on Zulip):

Tuxedo Pulse 15 was a nice laptop last time I looked, but I don't know if they've refreshed it or added a new line with newer CPUs. Also sold as TongFang or Mechrevo, see a review at https://www.notion.so/Mechrevo-Code-01-TongFang-PF5NU1G-Information-8009025fdefc40118ab0ea973e7e0988

Laurențiu (May 15 2021 at 18:32, on Zulip):

I'll try tomorrow to measure the peak RSS more accurately (with cgroups, maybe?)

Chetan Khilosiya (May 15 2021 at 18:34, on Zulip):

I will check if this is available in my country (India). Due to recent conditions all delivery and shipments are stopped.

Laurențiu (May 15 2021 at 18:35, on Zulip):

Yeah, I imagine availability will be the main criteria.

Laurențiu (May 15 2021 at 18:37, on Zulip):

Stay safe, by the way. I hope things get better soon there.

Chetan Khilosiya (May 15 2021 at 18:47, on Zulip):

I hope so

Last update: Jul 29 2021 at 09:15UTC