"conflicting implementation of trait" with enum_dispatch crate

[hahseba]

The below simple code compiles just fine on itself, but rls (used through VS Code) gives me three errors.
Code:

#[macro_use]
extern crate enum_dispatch;

#[enum_dispatch]
trait T {}

struct A {}
impl T for A {}

struct B {}
impl T for B {}

#[enum_dispatch(T)]
enum E {
    A,
    B,
}

fn main() {}

Error:

{
	"resource": "/C:/Users/hahseba/Desktop/rust/src/main.rs",
	"owner": "_generated_diagnostic_collection_name_#0",
	"code": "E0119",
	"severity": 8,
	"message": "conflicting implementations of trait `T` for type `E`:\n\nconflicting implementation for `E`",
	"source": "rustc",
	"startLineNumber": 13,
	"startColumn": 1,
	"endLineNumber": 13,
	"endColumn": 20,
	"relatedInformation": [
		{
			"startLineNumber": 4,
			"startColumn": 1,
			"endLineNumber": 4,
			"endColumn": 17,
			"message": "first implementation here",
			"resource": "/C:/rust/src/main.rs"
		},
		{
			"startLineNumber": 13,
			"startColumn": 1,
			"endLineNumber": 13,
			"endColumn": 20,
			"message": "conflicting implementation for `E`",
			"resource": "/C:/rust/src/main.rs"
		}
	]
}

[alexheretic]

I can reproduce this. I found if you disable all_targets Rls will work ok. But I don't understand the cause, and indeed cargo check --all-targets works fine.

We probably need to trace the rustc arguments and look for differences.

[hahseba]

After making a trivial change, the next rls rebuild also triggers the error even with all targets disabled. Perhaps some caching issue?

[lijinpei]

When invoking compile_with_exec() function, parameter options.filter could be
Only { all_targets: true, lib: true, bins: All, examples: All, tests: All, benches: All}
or
Default { required_features_filterable: true}.
cargo check use the 'Default' variant, cargo check --all-targets use the 'Only' variant. RLS use the 'Default' variant if all_targets is set to false (and lib is set to false, which is the default and not adjustable), otherwise the 'Only' variant.
Cargo won't emit diagnostics with either one, but RLS will if filter is set to the 'Only' variant (which I think maybe the real bug).

[lijinpei]

Some further information:
With filter set to Default, only one unit is generated(both for RLS and cargo):
Unit { pkg: Package { id: PackageId { name: "rp", version: "0.1.0", source: "/home/lijinpei/tmp/rp" }, ..: ".." }, target: Target { name: "rp", doc: true, ..: with_path("/home/lijinpei/tmp/rp/src/main.rs", Edition2018) }, profile: Profile { ..: default_dev() }, kind: Host, mode: Check { test: false } }
With filter set to Only, two units are generated(both for RLS and cargo):
Unit { pkg: Package { id: PackageId { name: "rp", version: "0.1.0", source: "/home/lijinpei/tmp/rp" }, ..: ".." }, target: Target { name: "rp", doc: true, ..: with_path("/home/lijinpei/tmp/rp/src/main.rs", Edition2018) }, profile: Profile { ..: default_dev() }, kind: Host, mode: Check { test: true } } Unit { pkg: Package { id: PackageId { name: "rp", version: "0.1.0", source: "/home/lijinpei/tmp/rp" }, ..: ".." }, target: Target { name: "rp", doc: true, ..: with_path("/home/lijinpei/tmp/rp/src/main.rs", Edition2018) }, profile: Profile { ..: default_dev() }, kind: Host, mode: Check { test: false } }

[lijinpei]

Yet some further information: RlsExecutor.exec() will be called twice, and if we:

• Only exec one job(either one), no bug
• put a mutex around the whole exec() function, bug.
• replace exec() with two line(as in the default impl) 'cargo_cmd.exec()?;Ok(())', no bug.

So, I think the caching logic is really suspicious.

[lijinpei]

After some digging around, I think perhaps the problem can be reduced to calling run_compiler() twice in the same process in rls. I have made a demo code at https://github.com/lijinpei/twice_run_compiler (my demo code to reproduce the error, just 24 lines, same error message as rls) and https://github.com/lijinpei/rp (same as hahseba's code) .You can reproduce the error as following:

1. clone the two repos, and run carg build in rp and twice_run_compiler
2. in twice_run_compiler, run something like this cargo run --bin twice -- --sysroot /home/lijinpei/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu --extern enum_dispatch=/home/lijinpei/tmp/rp/target/rls/debug/deps/libenum_dispatch-13fe138f99454ea0.so ~/tmp/rp/src/main.rs

On my machine, the second step produce the same diagnostic message as rls does(remember to modify path in the command line to suite your settings).
Besides, you can run twice_two_files for main.rs and main1.rs, interestingly, that also produces the error message, but if you run it agains main.rs and something that doesn't use enum_dispatch, there is no error message.
Note that cargo doesn't have this problem because cargo spawn new processes to run rustc.
I think if run_compiler() can be called on the same process for the same(or similar?) file multiple times, then the bug is on the rustc side, if that function cann't be called multiples in the same thread, well, rls is counting on that.
Also please point out any miss-use of run_compiler() in my demo code.

[lijinpei]

The problem is indeed caused by how enum_dispatch works. enum_dispatch will collect information about which enum(s) impl which trait(s), and generate codes along the way. That information is stored with String as hash key(not ast's ident/NodeId, or something like a scheme's syntax-object), and that information is never eliminated. So when rustc run expand_macro for the second time, residual information from the first expand_macro cause spurious code injected. I have forked a branch of enum_dispatch which shows if that information is removed timely, the spurious diagnostic message can be suppressed(it only means to show the cause of this issue, not as a remedy for enum_dispatch).
This error happens exactly as in this file and its expandsion.
I think this is an issue with Rust's macro system, not much we can do with RLS.
What do you think @alexheretic ?

[alexheretic]

Great investigation @lijinpei! So this is an effect of RLS using the compiler in-process.

This means in any proc-macro mutable statics may cause bugs like this where the author has assumed a batch lifetime, but also immutable statics become long lived and take up memory even when RLS is idling.

Basically this further fuels my feeling that RLS should use a separate process for compiling. I wanted this before for the purposes of cancelling compilations. The reason, as I understand it, that we do it in-process is to make Vfs work, ie to allow compiling with an unsaved file in the project.

I'll put an issue together so we can look at wrapping compilation in a process / Vfs usage cross process.

[Xanewok]

@alexheretic just to note, I agree that we should move compilation out of process and it'd be great to create a tracking issue of sorts, if you're already going to do something similar.

[alexheretic]

@Xanewok #1307 is up now.

[antonok-edm]

enum_dispatch author here. Looks like you all have narrowed things down, but please me know if there's anything else I can help with.

[IsaacTru]

After reading through this issue and #1307, I don't see an explicit resolution. Am I missing something? What have others done to create a work-around? Is the outcome that RLS is not usable with enum_dispatch? Thanks!

[Herschel]

Hitting this as well; will the ipc feature be enabled by default in the future? Thank you!

[rbtcollins]

I was considering using enum-dispatch, but putting it in the API of my crate would surface this bug to all my my crates users, which is probably too much.

Is there any workaround possible in the short term? @lijinpei you say that your demonstration branch of enum-dispatch isn't meant to fix the issue for enum-dispatch, but it passes the enum-dispatch test suite. Would updating it to work with master and merging it not be reasonable? After all this bug is coming up on 2 years old now, and ex-process compiles is still not a clearly-will-happen thing, let alone a thing with a defined timeframe.

@antonok-edm I think updating and merging the branch that cleans up the tracking state in the macro would be great - it passes the test suite, which seems pretty comprehensive.