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.

Sign up for GitHub

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

Jump to bottom

COMP: add completion of lint attributes #5646

Merged

bors merged 1 commit into intellij-rust:master from Kobzol:comp-lint-attributes

Oct 23, 2020

Member

Kobzol commented Jun 24, 2020 •

edited

Loading

This PR adds completion of lint attributes in allow, deny, warn and forbid meta items. The lints are filtered based on their path prefix, i.e. if you write clippy::, it will only offer you lints from clippy.

Right now on the root level it offers all lints (i.e. both unused and clippy::xxx), maybe we can only offer clippy:: at the root level and only after completing that we can start completing clippy lints?

Fixes: #3720

Kobzol added the feature label

Kobzol requested a review from Undin

July 12, 2020 09:05

Kobzol assigned Undin

Undin reviewed

View reviewed changes

Member

Undin left a comment

In general looks great!

Could you rebase onto master, please?

src/main/kotlin/org/rust/lang/core/RsPsiPattern.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated

+              private sealed class Lint(val name: String) {
+                  class RustcSpecific(name: String) : Lint(name)
+                  class RustcGroup(name: String) : Lint(name)
+                  class Clippy(name: String) : Lint("clippy::$name")

Member

Undin Sep 25, 2020

Note, there are clippy groups as well according to its documentation

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated

+                              )
+                      }
+                  private fun getIcon(lint: Lint): Icon = when (lint) {

Member

Undin Sep 25, 2020

If we decide to split the current completion provider into two ones, it may be convenient to move this method into Lint itself

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated

+                      else -> RsIcons.ATTRIBUTE
+                  }
+                  private fun getPriority(lint: Lint): Double = when (lint) {

Member

Undin Sep 25, 2020

The same about moving method into Lint

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated

Comment on lines 40 to 43

+                          val name = it.name
+                          if (!name.startsWith(prefix)) return@forEach
+                          val offered = name.removePrefix(prefix)

Member

Undin Sep 25, 2020

Right now on the root level it offers all lints (i.e. both unused and clippy::xxx), maybe we can only offer clippy:: at the root level and only after completing that we can start completing clippy lints?

It looks quite hacky. I think it's definitely better to suggest complex lints like clippy::xxx in two steps.
Probably, it makes sense to split this provider into two: one for rustc lints and another one for clippy

src/main/kotlin/org/rust/lang/core/completion/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/RsPsiPattern.kt

@@ @@ -169,12 +176,17 @@ object RsPsiPattern { @@
                           .withSuperParent(2, RsOuterAttributeOwner::class.java)
                           .with("nonStdAttributeCondition") { e -> e.name !in STD_ATTRIBUTES }
+                  val lintAttributeMetaItem: PsiElementPattern.Capture<RsMetaItem> =

Member

Undin Sep 25, 2020

JFYI there is org.rust.lang.core.completion.RsPsiPatternTest to check that patterns works expected.
We usually don't use unit test and use integration instead but since patterns can be quite complex, these tests can be helpful

Member Author

Kobzol commented Sep 26, 2020

Actually, I decided to handle the automatic lint setup in this PR, it would just be a needless change if I messed with the lint data structure afterwards. I created a gradle task that parses Rustc lints from rustc -W help, however for Clippy it's a bit more complicated. There is a website with all Clippy lints, but sadly it requires Javascript to load. So possible options that I see are:

Use some headless browser to parse the lints (probably too heavyweight)
Parse the lints manually from the GitHub Clippy repository, either by HTTP requests to the individual files or by cloning the Clippy git repository.
Parse the lints by cloning the Clippy repository and running their Python helper script that can already parse them.

What should I use? I would prefer option 3), by creating a Python script in the scripts directory, which would clone the Clippy repo and use their script to get the lints.

Member

Undin commented Sep 26, 2020

@Kobzol at the first glance, option 3 looks the simplest so I'd prefer this option too

Member Author

Kobzol commented Oct 10, 2020

Ok. My notebook's disk died and I didn't backup the WIP code that I had (which was using gradle kts script to download the things), so I'll rewrite it from scratch in Python.

Kobzol force-pushed the comp-lint-attributes branch from 84f4b40 to cecc608 Compare

October 14, 2020 19:43

Member Author

Kobzol commented Oct 14, 2020

Rewrote it from scratch.

Undin reviewed

View reviewed changes

src/main/kotlin/org/rust/lang/core/completion/lint/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/lint/RsRustcLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/lint/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/lint/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

src/main/kotlin/org/rust/lang/core/completion/lint/RsLintCompletionProvider.kt Outdated Show resolved Hide resolved

scripts/fetch_lints.py Outdated Show resolved Hide resolved

scripts/fetch_lints.py Outdated Show resolved Hide resolved

build.gradle.kts Outdated Show resolved Hide resolved

build.gradle.kts Outdated Show resolved Hide resolved

build.gradle.kts Outdated Show resolved Hide resolved

Member Author

Kobzol commented Oct 21, 2020

Thanks for the review! It seems that I forgot to remove some things that weren't needed. I put the changes into a separate commit to make additional review easier.

Undin approved these changes

View reviewed changes

Member

Undin left a comment

LGTM
Thank you! It's great!
Could you do the following things before the merge, please?:

update gif image in the description since it's a little bit outdated regarding clippy lints
squash commits

Undin added this to the v134 milestone


          COMP: add completion of lint attributes

cbbd141

Kobzol force-pushed the comp-lint-attributes branch from 7f799f9 to cbbd141 Compare

October 23, 2020 07:54

Member Author

Kobzol commented Oct 23, 2020

Done and done. Btw, I think that similar completion could be useful for features (both language and compiler ones), what do you think?

Undin approved these changes

View reviewed changes

Member

Undin left a comment

bors r+

Member

Undin commented Oct 23, 2020

Btw, I think that similar completion could be useful for features (both language and compiler ones), what do you think?

Yep, it may be handy for some users. We even already have a list of compiler features in the plugin and a way to update them.
But it may be harder than completion for lints because:

it should be available only with nightly compiler otherwise you get https://doc.rust-lang.org/error-index.html#E0554. BTW this error is not handled by the plugin
there is little sense to complete already released features because it produces compiler warning (which not handled by the plugin as well)
we shouldn't suggest features that are already used in the project since it a https://doc.rust-lang.org/error-index.html#E0636 (not supported by the plugin too)

Member Author

Kobzol commented Oct 23, 2020

Good point, it should probably be introduced gradually (by solving some of the substeps first).

Contributor

bors bot commented Oct 23, 2020

Build succeeded:

check

bors bot merged commit 2e22220 into intellij-rust:master

Kobzol deleted the comp-lint-attributes branch

October 23, 2020 09:10

Kobzol mentioned this pull request

COMP: simplify script for fetching Clippy lints #6305

Merged

Undin mentioned this pull request

Undesired lint completion #6311

Closed

bors bot added a commit that referenced this pull request


          Merge #6305

4e02ba8

6305: COMP: simplify script for fetching Clippy lints r=Undin a=Kobzol

I noticed in the Rust Analyzer [changelog](https://rust-analyzer.github.io/thisweek/2020/10/26/changelog-48.html) that they also added lint completion a few days ago (what a coincidence! :D ). They use a JSON with all Clippy lints that I didn't know of. It's a much simpler solution than cloning the `clippy` repository, which is what I have done in the [original PR](#5646).

Theoretically, the Python script could now be moved back into `build.gradle.kts`, but I don't think it's worth it.

Co-authored-by: Jakub Beránek <berykubik@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels