Stream: t-compiler/wg-rls-2.0

Topic: Yet another auto import bug


Kirill Bulatov (Feb 04 2020 at 21:01, on Zulip):

I've spotted another auto import bug, but fail to reproduce it with the test fixture.

The bug: create a project with a name-that-contains-dashes.
Have a main.rs and a struct defined in lib.rs. Try to auto import the struct into the main.rs.
Expected: use name_that_contains_dashes::Foo
Actual: use name-that-contains-dashes::Foo // this won't compile

When I try to add a test fixture in the find_path.rs test module, something like:

#[test]
fn zzz() {
    let code = r#"
        //- /main.rs crate:main deps:std-with-dashes
        <|>
        //- /std.rs crate:std-with-dashes
        pub struct S;
    "#;
    check_found_path(code, "std-with-dashes::S");
}

it panics despite the import form I use.

I've failed to create a snippet without deps due to the same reason and don't really want to dive into the code of the fixtures yet.
Do you have any ideas on how can I create a proper test case for the bug?

Kirill Bulatov (Feb 04 2020 at 21:05, on Zulip):

If I use deps:std_with_dashes and the corresponding crate name everywhere in the fixture, it works so I suspect that it's a bit too late to use the dashes form in those tests and I should create this test elsewhere.

Florian Diebold (Feb 04 2020 at 22:18, on Zulip):

does our name resolution actually work with that name_that_contains_dashes? I suspect we just don't handle that. The autoimport algorithm just uses the name from the extern prelude; that should already be a valid identifier

Kirill Bulatov (Feb 04 2020 at 22:37, on Zulip):

I don't get any completion for those structs but can navigate to them.
Can also find them with Cmd + T shortcut, so some part of the functionality is there.

matklad (Feb 05 2020 at 00:01, on Zulip):

I think we should add assert to CrateGraph that crate names do not have dashes

matklad (Feb 05 2020 at 00:01, on Zulip):

Or, more specifically, that they are lexed as valid rust identifiers

Kirill Bulatov (Feb 05 2020 at 08:22, on Zulip):

So it looks like we should adjust the names somewhere before storing? I wonder where is the place in the code, same CrageGraph?

matklad (Feb 05 2020 at 08:28, on Zulip):

I think we should get names with underscroes from cargo metadata, so we don't need to adjust them. Rather, it's a precondition

matklad (Feb 05 2020 at 08:29, on Zulip):

However, adding struct CrateName(SmolStr) which asserts the precondition in ctor would be useful

Kirill Bulatov (Feb 05 2020 at 08:39, on Zulip):

I think we should get names with underscroes from cargo metadata, so we don't need to adjust them. Rather, it's a precondition

Looks like it's not true, for the project itself at least:

{"packages":[{"name":"test-the-dashes","version":"0.1.0","id":"test-the-dashes 0.1.0 (path+file:///Users/someonetoignore/Downloads/tmp_dir/test-the-dashes)","license":null,"license_file":null,"description":null,"source":null,"dependencies":[],"targets":[{"kind":["lib"],"crate_types":["lib"],"name":"test-the-dashes","src_path":"/Users/someonetoignore/Downloads/tmp_dir/test-the-dashes/src/lib.rs","edition":"2018","doctest":true},{"kind":["bin"],"crate_types":["bin"],"name":"test-the-dashes","src_path":"/Users/someonetoignore/Downloads/tmp_dir/test-the-dashes/src/main.rs","edition":"2018","doctest":false}],"features":{},"manifest_path":"/Users/someonetoignore/Downloads/tmp_dir/test-the-dashes/Cargo.toml","metadata":null,"publish":null,"authors":["Kirill Bulatov <mail4score@gmail.com>"],"categories":[],"keywords":[],"readme":null,"repository":null,"edition":"2018","links":null}],"workspace_members":["test-the-dashes 0.1.0 (path+file:///Users/someonetoignore/Downloads/tmp_dir/test-the-dashes)"],"resolve":{"nodes":[{"id":"test-the-dashes 0.1.0 (path+file:///Users/someonetoignore/Downloads/tmp_dir/test-the-dashes)","dependencies":[],"deps":[],"features":[]}],"root":"test-the-dashes 0.1.0 (path+file:///Users/someonetoignore/Downloads/tmp_dir/test-the-dashes)"},"target_directory":"/Users/someonetoignore/Downloads/tmp_dir/test-the-dashes/target","version":1,"workspace_root":"/Users/someonetoignore/Downloads/tmp_dir/test-the-dashes"}
Kirill Bulatov (Feb 05 2020 at 08:43, on Zulip):

It is indeed named this way in the Cargo.toml, with the dashes:

[package]
name = "test-the-dashes"
# ... snip
matklad (Feb 05 2020 at 08:43, on Zulip):

We should be using the name from the dep node:

                "deps": [
                    {
                        "name": "cfg_if",
                        "pkg": "cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)",
                        "dep_kinds": [
                            {
                                "kind": null,
                                "target": null
                            }
                        ]
                    },
matklad (Feb 05 2020 at 08:43, on Zulip):

The name in package is just a display name for the user, it's not the name the compiler sees, because it doesn't take underscores and foo = { package = "bar"} feature of Cargo.toml.

matklad (Feb 05 2020 at 08:44, on Zulip):

into account

Kirill Bulatov (Feb 05 2020 at 08:45, on Zulip):

Ah, maybe I was not explicit enough: there are no deps, this is the project itself that has the dashes and we try to import a struct from the project's lib.rs to the project's main.rs.

So the issue I'm talking about has nothing to do with the side crates at all.

matklad (Feb 05 2020 at 08:46, on Zulip):

oh wow

matklad (Feb 05 2020 at 08:47, on Zulip):

that's actually an interesting conceptual bug in cargo....

matklad (Feb 05 2020 at 08:47, on Zulip):

Like, ideally it should record crate dependency on itself, and do renaming, but it doesnt

matklad (Feb 05 2020 at 08:48, on Zulip):

And that's because cargo metadata is higher level than crate_graph, which creates all sorts of problems...

Kirill Bulatov (Feb 05 2020 at 08:48, on Zulip):

Yes, we've stumbled onto something interesting here apparently.

I have no idea how to make it proper alas, but ready to help with some guidance.

matklad (Feb 05 2020 at 08:49, on Zulip):

yeah, let me check how we actually create dependency from a test to a lib.rs ...

matklad (Feb 05 2020 at 08:57, on Zulip):

fond it ra_project_model/src/lib.rs:280, pkg.name(&cargo).into() should do - -> _ conversion

matklad (Feb 05 2020 at 08:58, on Zulip):

Or, rather,

pub fn add_dep(
        &mut self,
        from: CrateId,
        name: CrateName,
        to: CrateId,
    ) -> Result<(), CyclicDependenciesError> {

Should be changed to

pub fn add_dep(
        &mut self,
        from: CrateId,
        name: CrateName,
        to: CrateId,
    ) -> Result<(), CyclicDependenciesError> {
Kirill Bulatov (Feb 05 2020 at 09:11, on Zulip):

Thanks, I'll look at it.

Kirill Bulatov (Feb 05 2020 at 09:12, on Zulip):

(I think you've inserted the same code snippet twice btw :D)

Kirill Bulatov (Feb 05 2020 at 09:36, on Zulip):

After I've applied the fix, not only imports but autocompletion also started to work, magic.

matklad (Feb 05 2020 at 10:39, on Zulip):

After I've applied the fix, not only imports but autocompletion also started to work, magic.

It's actually interesting how complexity and synergy emerges over time in an IDE. It's typical that fixing one things can accidentally make many different things better. Or how you can sometimes compile many small features into a complex and powerful feature. Love when it happens. Definitely prefer this to futzing with rollup :D

std::Veetaha (Feb 05 2020 at 10:43, on Zulip):

;( yeah, damn rollup, hate it

Last update: Sep 22 2020 at 01:00UTC