Stream: t-compiler/wg-rls-2.0

Topic: Need help for a contribution in ra


Coenen Benjamin (Apr 06 2020 at 15:47, on Zulip):

Hello All ! I'm pretty new here and as I use rust-analyzer for a while now I would like to contribute to this project. In fact I detected a bug in ra and would like to solve it by myself. But the code seems pretty dense for now for me. I would like to have a start point :) . The issue is when I have an attribute in a struct covered by a feature flag. I have 2 new function to create this struct. One for feature enabled and one without. So one with the field and another one without. And rust analyzer drop me an error because it seems it didn't catch the feature flag. So the struct MissingFields is created here https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_ty/src/expr.rs#L109:L109 and should not. Can you give me the file or the place in the codebase where I can find how to not create this kind of error ?

matklad (Apr 06 2020 at 16:05, on Zulip):

yup, that might be a less-than-easy issue to fix

matklad (Apr 06 2020 at 16:06, on Zulip):

Here's how we handle cfg at the top level: https://github.com/rust-analyzer/rust-analyzer/blob/c859a6480af7baece2eec38c19f71cb714db9e3b/crates/ra_hir_def/src/nameres/collector.rs#L697

matklad (Apr 06 2020 at 16:07, on Zulip):

But for impls I htink we don't yet take that into account: https://github.com/rust-analyzer/rust-analyzer/blob/c859a6480af7baece2eec38c19f71cb714db9e3b/crates/ra_hir_def/src/data.rs#L214-L222

Coenen Benjamin (Apr 06 2020 at 16:14, on Zulip):

Ok so the idea would be to implement a kind of is_cfg_enabled on StructFieldData ?

matklad (Apr 06 2020 at 16:15, on Zulip):

yup

Coenen Benjamin (Apr 06 2020 at 16:15, on Zulip):

StructFieldData or StructData ?

matklad (Apr 06 2020 at 16:22, on Zulip):

for struct fields -- structs themsleves are handled by the code in collector.rs linked above

Coenen Benjamin (Apr 06 2020 at 16:25, on Zulip):

Ok thank you. I really want to tackle this issue. What the best way to indicate I'm working on this ? I create an issue about that ?

Coenen Benjamin (Apr 07 2020 at 09:43, on Zulip):

Hello, just a question, I'm in the crate ra_hir_ty, precisely in this function https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_ty/src/expr.rs#L64:L64 and I want to have access to data from ModCollector here https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_def/src/nameres/collector.rs#L659:L659 How can I do ? Can I use the database ?

Coenen Benjamin (Apr 07 2020 at 09:45, on Zulip):

I have the file_id, I don't know if it can help

matklad (Apr 07 2020 at 09:51, on Zulip):

So the collectors are transient data structures, which ultimatelly collect all into into a CrateDefMap. You can get hold of it by calling db.crate_def_map(file_id)

matklad (Apr 07 2020 at 09:52, on Zulip):

But I am not sure that the logic for fixing cfg-ed out fields should go to validate_record_literal. I think it already should see only the fields with active cfgs, we should rather disable extra fields when we create StructData

Coenen Benjamin (Apr 07 2020 at 09:52, on Zulip):

Hmm ok

Coenen Benjamin (Apr 07 2020 at 10:06, on Zulip):

Ok now I'm stuck hre https://github.com/rust-analyzer/rust-analyzer/blob/master/crates/ra_hir_def/src/adt.rs#L59:L59 because I only have StructId and I need CrateId to call db.crate_def_map(...). Is there any way to fetch CrateId at this point ? :)

Coenen Benjamin (Apr 07 2020 at 10:25, on Zulip):

Found a solution thank you :)

Coenen Benjamin (Apr 07 2020 at 13:59, on Zulip):

Is there a way when I'm at a struct level to get the function in which I am ? With db ?

Coenen Benjamin (Apr 07 2020 at 14:01, on Zulip):

cc @matklad as you helped me a lot :)

std::Veetaha (Apr 07 2020 at 17:48, on Zulip):

@Coenen Benjamin if you are talking about a struct litreal, you might be looking for DefWithBodyId somewhere nearby, e.g. the inference itself happens inside of a const/static initializer or a fn body. After that you will be able to query function/const/static data from the DefDatabase which is a supertrait of HirDatabase

Coenen Benjamin (Apr 07 2020 at 18:00, on Zulip):

@std::Veetaha thanks a lot ! I really need your help to let me dig and understand the code and become efficient after. I created a Draft PR about that. If you want to help me feel free https://github.com/rust-analyzer/rust-analyzer/pull/3880 I really want to involve all my free time in this project because it helps me a lot in my day to day job !

std::Veetaha (Apr 07 2020 at 20:26, on Zulip):

Sorry, can't say nothing more elaborate ) @matklad is really who can help

Coenen Benjamin (Apr 08 2020 at 13:32, on Zulip):

Hi @matklad I read your comment here https://github.com/rust-analyzer/rust-analyzer/pull/3880/files#r405451522 but I think I don't understand. In fact, I'm in the situation in which I write a crate with different feature so I write 2 methods with the same name but different behavior depending the feature. As I understand you say I have to check the cfg_options from the crate. But I want to check the syntax for both features. So cfg_options wouldn't help me to achieve that. I have to check parent attributes to know in which case I wrote this method. Am I wrong somewhere ?

matklad (Apr 08 2020 at 13:34, on Zulip):

So, the way rustc (and, currently, rust-analyzer) is implemented is that it only "sees" one of the functions anyway, the one which passes currect cfg config

matklad (Apr 08 2020 at 13:34, on Zulip):

Ideally, we should somehow check both of them, but this is not currently implemented

Coenen Benjamin (Apr 08 2020 at 13:36, on Zulip):

But in my situation there aren't any "correct" feature because it's not coming from a dependency. It's my own crate with my own feature. So ... it's impossible for me to solve this issue for now ?

matklad (Apr 08 2020 at 13:37, on Zulip):

By default, rust-analyzer uses default feature for the crates in current workspace. This is overridable via config option, sort of how you specify the features on the command line.

Coenen Benjamin (Apr 08 2020 at 13:42, on Zulip):

I was thinking about passing a kind of context with enabled configurations from parent to children and then each time checking the configuration for the node. But maybe not the best implementations. Hmmm I definitely can't solve my issue now :(

matklad (Apr 08 2020 at 13:50, on Zulip):

The problem of rust-analyzer giving a false positive is definitely solvable without doing any major refactorings, and I think you PR already goes about 60% towads the solution

Coenen Benjamin (Apr 08 2020 at 14:02, on Zulip):

hmm ok I really want to finish it and find the best workaround for now. Thanks for your help. If I understand your solution is based on usage of cfg_option at the crate level ? In this crate level the cfg_options just check the default features. If my feature is not in the default then it's disabled and I should not make any checks about these children ?

Coenen Benjamin (Apr 08 2020 at 14:04, on Zulip):

My first implementation here https://github.com/rust-analyzer/rust-analyzer/pull/3880/files#diff-df1c60833b4d654e53af1a24e549facfR226 was to check with these cfg_options at the crate_graph level. Is it the correct solution ?

matklad (Apr 08 2020 at 14:07, on Zulip):

Yes, I think so

Coenen Benjamin (Apr 08 2020 at 14:08, on Zulip):

Ok great I will do that :)

Coenen Benjamin (Apr 08 2020 at 14:47, on Zulip):

@matklad Ok I reimplemented what I did before with cfg_options from the crate. But the fact is I should not create a StructData if my functionData is not in the default feature. Could you send me a link please to find where I can basically stop a creation of a FunctionData. If I go on the query function it just returns an Arc so I can't return None or something like that. When I find this location I think I just have to use my function is_cfg_enabled with current attributes and that's all

matklad (Apr 08 2020 at 15:12, on Zulip):

https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Fwg-rls-2.2E0/topic/Need.20help.20for.20a.20contribution.20in.20ra/near/193063841

matklad (Apr 08 2020 at 15:12, on Zulip):

That I think the place where func-data is wrongly created

Coenen Benjamin (Apr 08 2020 at 15:54, on Zulip):

@matklad Yeah ! My test pass when I mock the situation. So last question :p How can I force my test to have default features. Here is what I tried:

//- Cargo.toml
        [package]
        name = "test"
        version = "0.0.1"
        [features]
        default = ["foo"]

        //- /lib.rs cfg:feature=foo
        struct MyStruct {
            my_val: usize,
            #[cfg(feature = "foo")]
            bar: bool,
        }

        impl MyStruct {
            #[cfg(feature = "foo")]
            pub(crate) fn new(my_val: usize, bar: bool) -> Self {
                Self { my_val, bar }
            }

            #[cfg(not(feature = "foo"))]
            pub(crate) fn new(my_val: usize, _bar: bool) -> Self {
                Self { my_val }
            }
        }
Coenen Benjamin (Apr 08 2020 at 15:55, on Zulip):

Without any success when I display cfg_options in fixture it's always empty :(

Coenen Benjamin (Apr 08 2020 at 16:06, on Zulip):

Ok I found the issue, the crate:foo in meta was missing

Coenen Benjamin (Apr 08 2020 at 16:27, on Zulip):

I updated my PR ! Thanks a lot for your help

Coenen Benjamin (Apr 09 2020 at 08:29, on Zulip):

Just for my culture. Does anyone know what the acronym hir means ?

detrumi (Apr 09 2020 at 08:32, on Zulip):

High-level IR, see also the glossary in rustc-dev-guide

Coenen Benjamin (Apr 09 2020 at 08:40, on Zulip):

Thanks a lot ! That's what I'm looking for :)

Coenen Benjamin (Apr 10 2020 at 13:11, on Zulip):

Is anyone has good resources about green nodes ra uses ?

Muhammad Mominul Huque (Apr 10 2020 at 13:18, on Zulip):

I think this will be helpful https://github.com/rust-analyzer/rust-analyzer/blob/master/docs/dev/syntax.md

Coenen Benjamin (Apr 10 2020 at 13:30, on Zulip):

Ok thanks there is a link inside which seems very interesting https://docs.microsoft.com/en-ie/archive/blogs/ericlippert/persistence-facades-and-roslyns-red-green-trees Have a nice day

Coenen Benjamin (Apr 11 2020 at 13:56, on Zulip):

I would like to work on this issue but don't understand the issue. Can you give me a simple example to reproduce @matklad ?

Coenen Benjamin (Apr 11 2020 at 14:37, on Zulip):

With the link to the issue https://github.com/rust-analyzer/rust-analyzer/issues/3903 it is far better

Last update: Sep 30 2020 at 15:00UTC