feat#1533: verify `general` and `supplementary` registered lints

[augustin-v]

Relevant issue #1533

This test ensures that all lints defined in the "general" and "supplementary" folders are actually registered in the corresponding src/lib.rs file. The process is as follows:

1. Extract expected lints by listing the immediate subdirectories in "general" and "supplementary" (ignoring src and hidden directories) to determine the expected lint module names.
2. Read the src/lib.rs file in both folder and extract registered lints using this regex ([a-zA-Z_][a-zA-Z0-9_]*)::register_lints\s*\( which captures the module name right before the literal ::register_lints call.
3. Lastly, compare the expected and actually registered lints.

if we were to run the test with the dylint's current state we would get this output:

---- verify_registered_lints stdout ----

thread 'verify_registered_lints' panicked at cargo-dylint/tests/ci.rs:584:13:
Mismatch in /Users/user/dylint/cargo-dylint/../examples/supplementary:

Missing registered lints: ["local_ref_cell", "nonexistent_path_in_comment"]

Expected: ["commented_code", "escaping_doc_link", "inconsistent_struct_pattern", "local_ref_cell", "nonexistent_path_in_comment", "redundant_reference", "unnamed_constant", "unnecessary_borrow_mut", "unnecessary_conversion_for_trait"]
Actual: ["commented_code", "escaping_doc_link", "inconsistent_struct_pattern", "redundant_reference", "unnamed_constant", "unnecessary_borrow_mut", "unnecessary_conversion_for_trait"]
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Also seeing that ::register_lints is only used inside of the general and supplementary folders, I figured that using the examples::iter function would be too broad for our check.

This approach is a bit different from the initial that we initially discussed about, because we extract the existing lints with their folder names instead of extracting them from the cargo metadata. I'm open to feedback or alternative suggestions!

[smoelius]

I really appreciate you working on this. 🙏

Missing registered lints: ["local_ref_cell", "nonexistent_path_in_comment"]

I noticed this actually. :)

I started a PR to run the nonexistent_path_in_comment lint over the Dylint repo (#1536), and it is currently failing. I think the path regex may need to be refined.

You are welcome to ignore the failures, but if you have ideas on how to refine the regex, I would welcome them.

Please note that I already made some small changes to the lint's logic: https://github.com/trailofbits/dylint/pull/1536/files#diff-655df697dba7900d7f7a61d9548c870a5068b918eabf6381760f107b35afb3a2L133-L149

Needless to say, I really appreciate you building that lint, because I am sure it will catch mistakes.

I will give the present PR a more thorough review after #1536 is finalized and merged.

[augustin-v]

I really appreciate you working on this. 🙏

Pleasure is mine I'll be here for a while, I have a lot to learn from you😊

Not a very creative idea but maybe we could change the path regex to (?:(?!\$)(?:\./|\.\./|/|[\w/-]+/)+[\w-]+(?:\.[\w-]+)+) so that it filters out paths starting with $ and add when a comment is flagged, we would add a note to make it user friendly such as : ---- "NOTE: add $ prefix at the start of the path to ignore" ----

[smoelius]

Pleasure is mine I'll be here for a while, I have a lot to learn from you😊

You're very kind. :)

Not a very creative idea but maybe we could change the path regex to (?:(?!\$)(?:\./|\.\./|/|[\w/-]+/)+[\w-]+(?:\.[\w-]+)+) so that it filters out paths starting with $ and add when a comment is flagged, we would add a note to make it user friendly such as : ---- "NOTE: add $ prefix at the start of the path to ignore" ----

It might be worth just allowing the $DIR/... cases.

But I happen to know the lint triggers in other places. For example, it produces these warnings in the general workspace:

warning: referenced path does not exist
  --> local_ref_cell/src/lib.rs:37:44
   |
37 |     /// [`RefCell`]: https://doc.rust-lang.org/std/cell/struct.RefCell.html
   |                                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference
   = note: `#[warn(nonexistent_path_in_comment)]` on by default

warning: `local_ref_cell` (lib) generated 1 warning
warning: referenced path does not exist
  --> unnecessary_borrow_mut/src/lib.rs:54:56
   |
54 |     /// [`RefCell::borrow_mut`]: https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.borrow_mut
   |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference
   = note: `#[warn(nonexistent_path_in_comment)]` on by default

warning: referenced path does not exist
  --> unnecessary_borrow_mut/src/lib.rs:55:52
   |
55 |     /// [`RefCell::borrow`]: https://doc.rust-lang.org/std/cell/struct.RefCell.html#method.borrow
   |                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference

Another idea, besides refining the regex, could be to cast a wider net and then filter out unwanted matches afterward.

Like maybe the regex could match non-empty sequences with at least one /, and then the lint does:

if full_path.starts_with("https://") {
    continue;
}

[augustin-v]

Another idea, besides refining the regex, could be to cast a wider net and then filter out unwanted matches afterward.

Like maybe the regex could match non-empty sequences with at least one /, and then the lint does:

if full_path.starts_with("https://") {
    continue;
}

Yes this seems like the best approach to deal with the url, similar to the escaping_doc_link one I guess; looks like I did not implement the lint to ignore URLs as well as I thought 🤔

[smoelius]

Doing what I said literally produces a lot of false positives:

warning: referenced path does not exist
  --> internal/src/rustup.rs:46:41
   |
46 |     // smoelius: `path` should end with `/bin/rustc`.
   |                                         ^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference
   = note: `#[warn(nonexistent_path_in_comment)]` on by default

warning: `dylint_internal` (lib) generated 1 warning
warning: referenced path does not exist
  --> dylint/src/lib.rs:22:26
   |
22 | // smoelius: See note in dylint/src/library_packages/mod.rs.
   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference
   = note: `#[warn(nonexistent_path_in_comment)]` on by default

warning: referenced path does not exist
  --> dylint/src/driver_builder.rs:21:10
   |
21 | (https://github.com/trailofbits/dylint).
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: verify the path is correct or remove the reference

warning: referenced path does not exist
   --> dylint/src/driver_builder.rs:159:13
    |
159 |     // like `$ORIGIN/../../`... (see https://github.com/trailofbits/dylint/issues/54). The new
    |             ^^^^^^^^^^^^^^^^^^^
    |
    = help: verify the path is correct or remove the reference

I'm struggling a little bit here. I'm not sure what the best path forward is.

[smoelius]

@augustin-v Gentle ping. If you are busy, I would be happy to push this over the finish line.

[augustin-v]

@augustin-v Gentle ping. If you are busy, I would be happy to push this over the finish line.

Apologies for the very late reaction. Thank you for the ping, working on it now

[smoelius]

Apologies for the very late reaction. Thank you for the ping, working on it now

No worries at all. Thank you very much!

[augustin-v]

Hi @smoelius, I've addressed the changes please tell me if I forgot anything or anything you'd like me to change in the code, I'll fix it right away.

But even if it doesn't, could you rewrite this code so that the question marks are the outermost operators in the expressions in which they appear?

I found this very interesting! never thought about it but it makes sense for debugging and readability

[smoelius]

Thanks, @augustin-v. I will look at this within the next few days.

[smoelius]

Thanks a lot, @augustin-v.

[augustin-v]

The pleasure is mine @smoelius thanks for the help on the CI. I just got back from a long trip, looking forward to contribute again!