Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add resolve diagnostics #3569

Closed

Conversation

SomeoneToIgnore
Copy link
Contributor

Adds a set of diagnostics to highlight unresolved methods and paths.

The implementation idea is to use the part of the auto_import assist that goes before the actual import, since this part is responsible for resolution checks.
This reuse may also come in handy if we decide to generate import assists for this diagnostics.

I've turned off method resolution by default, since it gives a lot of false-positives.
The path resolution is way better (yet still has some glitches), so I've left it enabled.

Comment on lines +353 to +358
"rust-analyzer.diagnosticsOptions.checkPathsResolution": {
"type": "boolean",
"default": true,
"description": "Highlight unresolved paths"
},
"rust-analyzer.diagnosticsOptions.checkMethodsResolution": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"rust-analyzer.diagnosticsOptions.checkPathsResolution": {
"type": "boolean",
"default": true,
"description": "Highlight unresolved paths"
},
"rust-analyzer.diagnosticsOptions.checkMethodsResolution": {
"rust-analyzer.diagnostics.checkPathsResolution": {
"type": "boolean",
"default": true,
"description": "Highlight unresolved paths"
},
"rust-analyzer.diagnostics.checkMethodsResolution": {

Comment on lines +181 to +182
checkPathsResolution: this.cfg.get("diagnosticsOptions.checkPathsResolution") as boolean,
checkMethodsResolution: this.cfg.get("diagnosticsOptions.checkMethodsResolution") as boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
checkPathsResolution: this.cfg.get("diagnosticsOptions.checkPathsResolution") as boolean,
checkMethodsResolution: this.cfg.get("diagnosticsOptions.checkMethodsResolution") as boolean,
checkPathsResolution: this.cfg.get("diagnostics.checkPathsResolution") as boolean,
checkMethodsResolution: this.cfg.get("diagnostics.checkMethodsResolution") as boolean,

@@ -38,6 +38,7 @@ pub struct Options {
pub inlay_hints: InlayHintsOptions,
pub rustfmt_args: Vec<String>,
pub cargo_watch: CheckOptions,
pub diagnostics_config: DiagnosticsOptions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub diagnostics_config: DiagnosticsOptions,
pub diagnostics: DiagnosticsOptions,

I suggest to follow the pattern we have (i.e. not adding config suffix to the properties of this struct)

@bjorn3
Copy link
Member

bjorn3 commented Mar 13, 2020

If there is an extern crate that is unresolved, can all resolve erros involving that crate be suppresed? Otherwise development on crates using rustc private crates will get more annoying as the rust-src component only provides the source for some of those crates. I am not sure if rust-analyzer even resolves those crates that do have their source.

@SomeoneToIgnore
Copy link
Contributor Author

can all resolve erros involving that crate be suppresed?

To implement that, we need some kind of infra built up for "project settings" that remember the crates excluded per project, save those somewhere and restore them on reload.
Afaik currently, all functionality in RA is very simple and just has an on/off switch for the entire feature due to the in-memory approach.

@bjorn3
Copy link
Member

bjorn3 commented Mar 13, 2020

What I propose is that when you write extern crate unknown_crate;, that item will give an error, but any unknown_crate::my_fancy_function will get their resolve error suppressed. This means that there is still a way to know that the crate couldn't be found, but you don't get an error at every place you use the crate.

@flodiebold
Copy link
Member

I think this should be possible, especially if there's an explicit extern crate declaration. AFAIK, we still create a name in the module scope for any unresolved import; so we should be able to see that a certain import didn't resolve because it referred to another import that didn't resolve.

OTOH, @bjorn3 can you not use the workaround mentioned in #3517?

Relatedly: This isn't really how we want to be doing these diagnostics. They should be created by the HIR crates during analysis, not as a separate pass in ra_ide; that would also make it more possible to implement the above-mentioned logic. (I assume matklad will have an opinion as to whether we should merge this anyway.)

@bjorn3
Copy link
Member

bjorn3 commented Mar 13, 2020

OTOH, @bjorn3 can you not use the workaround mentioned in #3517?

No, that would cause the respective crates to be recompiled for cg_clif. As cg_clif is loaded into a pre-compiled rustc instance, that would result in a mismatch between the librustc used by rustc and cg_clif, thus likely causing corruption the worst case, or just missing TLS data for cg_clif in the best case.

@flodiebold
Copy link
Member

Ok, good to know 🤔

@matklad
Copy link
Member

matklad commented Mar 13, 2020

@bjorn3 there's also a nuclear option of using rust-project.json, but it probably isn't convenient....

@bjorn3
Copy link
Member

bjorn3 commented Mar 13, 2020

Maybe add a way to generate a rust-project.json file for the current project based on Cargo.toml? Or add a way to merge the Cargo.toml based results and rust-project.json?

@SomeoneToIgnore
Copy link
Contributor Author

So, did I get it right, we need to move this into the depths of the ra_hir and add a bit cleverer diagnostics (such as that extern crate case)?

I will close this PR then, since it's a totally different way.

In that case, any ideas on how should we be able to disable the diagnostics?
We need to pass the settings all the way up from rust-analyzer and somehow pass this config to all pieces of the code that perform the diagnostics.
Yet the only common structure tossed aroud that code is a DiagnosticSink and the rest of the interface seems to be different for every case.

@flodiebold
Copy link
Member

In that case, any ideas on how should we be able to disable the diagnostics?

I imagine, by filtering them according to the settings on the ra_ide level.

@SomeoneToIgnore
Copy link
Contributor Author

Yeah, the question is how to filter them uniformly across the crates before the diagnostics are computed.
I have thought that you know anything :)

But never mind, I'll fiddle with it more and come up with something.

@flodiebold
Copy link
Member

flodiebold commented Mar 13, 2020

I don't think it's necessary to filter them before they are computed 🙂

(It occurs to me I might have been misunderstood: I meant that the filtering should happen in ra_ide.)

@SomeoneToIgnore SomeoneToIgnore deleted the resolve-diagnostics branch March 15, 2020 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants