update_lints rewrite: Add structure and --print-only #2985

phansch · 2018-07-31T19:14:08Z

Part of #2882

This adds a clippy_dev project that currently uses clap, regex and lazy_static to print a list of lints. For now it only replaces the lint printing in update_lints.py.

Running util/dev update_lints --print-only from scratch takes 45s on my machine.

I could not make it use a stable toolchain because somehow running cargo test in the subdirectory parses the Clippy Cargo.toml, too:

$ cd clippy_dev && cargo run -- update_lints --print-only
error: failed to parse manifest at `/home/phansch/code/rust-clippy/Cargo.toml`

Caused by:
  the cargo feature `edition` requires a nightly version of Cargo, but this is the `stable` channel

phansch · 2018-07-31T19:16:29Z

clippy_dev/src/lib.rs

+use std::io::prelude::*;
+
+lazy_static! {
+    static ref DEC_CLIPPY_LINT_RE: Regex = Regex::new(r#"declare_clippy_lint!\s*[\{(]\s*pub\s+(?P<name>[A-Z_][A-Z_0-9]*)\s*,\s*(?P<cat>[a-z_]+)\s*,\s*"(?P<desc>(?:[^"\\]+|\\(?s).(?-s))*)"\s*[})]"#).unwrap();


Is there a way to split these up over multiple lines like in the python script? That would make them much more readable.
https://github.com/rust-lang-nursery/rust-clippy/blob/97840090b78ce1f370c3380e8a703da420fe3a98/util/update_lints.py#L11-L15

phansch · 2018-07-31T19:18:05Z

clippy_dev/src/main.rs

+        "nursery",
+        "restriction"
+    ];
+    // We could use itertools' group_by to make this much more concise:


Just saw that we already use itertools in Clippy, so I will use that in here, too.

ghost · 2018-08-03T04:50:32Z

clippy_dev/src/lib.rs

+
+pub fn collect_all() -> Vec<Lint> {
+    let mut lints = vec![];
+    for direntry in lint_files() {


dir_entry (instead of direntry) is easier on my eyes

phansch · 2018-08-16T08:12:42Z

(will get back to this and the other PR and the end of the month)

phansch · 2018-09-05T07:36:46Z

I think this is ready for a final review

oli-obk · 2018-09-05T08:42:20Z

I could not make it use a stable toolchain because somehow running cargo test in the subdirectory parses the Clippy Cargo.toml, too:

I think this is because it thinks it's part of the workspace. Not sure if that is resolveable

oli-obk · 2018-09-05T08:43:38Z

clippy_dev/.gitignore

@@ -0,0 +1,38 @@
+# Used by Travis to be able to push:
+/.github/deploy_key


Is it possible to just update our main .gitignore?

clippy_dev/src/main.rs

oli-obk · 2018-09-05T08:52:56Z

clippy_dev/src/lib.rs

+        }
+    }
+
+    pub fn active_lints(lints: &[Lint]) -> Vec<Lint> {


I kind of feel like all the Vec returning functions here should be returning impl Iterator<Item=Lint>. That does require some string allocations, but should generally be possible.

Not a blocker for this PR, just a nice to have. You can also open an issue and leave it as a "good first issue"

This makes the API of `lib.rs` a bit more flexible.

oli-obk · 2018-09-05T16:05:02Z

clippy_dev/src/lib.rs

@@ -56,22 +57,22 @@ impl Lint {
    }
 }

-pub fn collect_all() -> Vec<Lint> {
+pub fn gather_all() -> impl Iterator<Item=Lint> {


this entire function can probably be lint_files().flat_map(identity)

This is my favorite refactoring in Ruby and I'm SO happy it's possible in Rust 🎉

oli-obk · 2018-09-05T16:06:00Z

clippy_dev/src/lib.rs

    let mut file = fs::File::open(dir_entry.path()).unwrap();
    let mut content = String::new();
    file.read_to_string(&mut content).unwrap();
    parse_contents(&content, dir_entry.path().file_stem().unwrap().to_str().unwrap())
 }

-fn parse_contents(content: &str, filename: &str) -> Vec<Lint> {
+fn parse_contents(content: &str, filename: &str) -> impl Iterator<Item=Lint> {
    let mut lints: Vec<Lint> = DEC_CLIPPY_LINT_RE


chain the two iterators and remove the collect&append&into_iter

If I don't collect the map result, I run into a lot of lifetime issues, because contents and filename are used in the iterator and would outlive the function. I will push the changes in a bit with a comment.

oli-obk · 2018-09-05T16:06:19Z

clippy_dev/src/lib.rs

 }

 /// Collects all .rs files in the `clippy_lints/src` directory
-fn lint_files() -> Vec<fs::DirEntry> {
+fn lint_files() -> impl Iterator<Item=fs::DirEntry> {
    let paths = fs::read_dir("../clippy_lints/src").unwrap();


remove the intermedate paths variable

phansch · 2018-09-05T18:37:00Z

clippy_dev/src/lib.rs

+        .captures_iter(content)
+        .map(|m| Lint::new( &m["name"], "Deprecated", &m["desc"], Some(&m["desc"]), filename));
+    // Removing the `.collect::<Vec<Lint>>().into_iter()` causes some lifetime issues due to the map
+    lints.chain(deprecated).collect::<Vec<Lint>>().into_iter()


If I remove the collect().into_iter() here, I get lifetime 'static' required for content and filename, which makes sense because they are used in the iterator and consumed outside of this function. I feel like making them static doesn't make sense here and would make the code more complicated to read.

phansch commented Jul 31, 2018

View reviewed changes

ghost reviewed Aug 3, 2018

View reviewed changes

phansch added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 29, 2018

phansch force-pushed the riir_update_lints branch 3 times, most recently from 77fa255 to cf89c58 Compare September 2, 2018 08:00

phansch removed the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 2, 2018

phansch force-pushed the riir_update_lints branch from 9e674eb to fe7e76a Compare September 3, 2018 05:48

phansch added 6 commits September 3, 2018 22:18

update_lints rewrite: Add structure and --print-only

35f100b

cargo update in clippy_dev

502357d

Use insignificant whitespace mode for nice regex

7031243

s/direntry/dir_entry

78d358b

Refactor to use into_group_map from Itertools

586ef4e

Run clippy on clippy_dev, too

be995dc

phansch force-pushed the riir_update_lints branch from fe7e76a to be995dc Compare September 3, 2018 20:20

phansch changed the title ~~WIP: update_lints rewrite: Add structure and --print-only~~ update_lints rewrite: Add structure and --print-only Sep 5, 2018

phansch added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 5, 2018

oli-obk reviewed Sep 5, 2018

View reviewed changes

clippy_dev/src/main.rs Show resolved Hide resolved

oli-obk reviewed Sep 5, 2018

View reviewed changes

phansch added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 5, 2018

phansch added 2 commits September 5, 2018 13:34

Return impl Iterator instead of Vec<Lint>

79bec03

This makes the API of `lib.rs` a bit more flexible.

Remove duplicated .gitignore

20836d3

oli-obk suggested changes Sep 5, 2018

View reviewed changes

phansch commented Sep 5, 2018

View reviewed changes

More refactoring

de36d42

phansch force-pushed the riir_update_lints branch from dedfb35 to de36d42 Compare September 6, 2018 05:15

Pass by ref instead of value

3bdc691

oli-obk merged commit dc164f4 into rust-lang:master Sep 6, 2018

phansch deleted the riir_update_lints branch September 6, 2018 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update_lints rewrite: Add structure and --print-only #2985

update_lints rewrite: Add structure and --print-only #2985

phansch commented Jul 31, 2018 •

edited

Loading

phansch Jul 31, 2018

killercup Jul 31, 2018

phansch Jul 31, 2018

ghost Aug 3, 2018

phansch commented Aug 16, 2018

phansch commented Sep 5, 2018

oli-obk commented Sep 5, 2018

oli-obk Sep 5, 2018

oli-obk Sep 5, 2018

oli-obk Sep 5, 2018

phansch Sep 6, 2018 •

edited

Loading

oli-obk Sep 5, 2018

phansch Sep 5, 2018

oli-obk Sep 5, 2018

phansch Sep 5, 2018

		@@ -0,0 +1,38 @@
		# Used by Travis to be able to push:
		/.github/deploy_key

update_lints rewrite: Add structure and --print-only #2985

update_lints rewrite: Add structure and --print-only #2985

Conversation

phansch commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansch commented Aug 16, 2018

phansch commented Sep 5, 2018

oli-obk commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansch Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansch commented Jul 31, 2018 •

edited

Loading

phansch Sep 6, 2018 •

edited

Loading