Add recursion check on main function

[Urriel]

Changes:

• Add MainRecursion lint to Clippy
• Check for no-std setup

fixes #333

changelog: Add main_recursion lint

[Urriel]

Documentation on the way

[flip1995]

This LGTM overall!

Can you remove the tests/ui/crate_level_checks/no_std_main_recursion.stderr file?

[matthiaskrgr]

Should we have a testcase with the #[main] feature (mark any function as main()) here?

#![feature(main)]

#[main]
fn a() {
    println!("hello from our new main method");
}

[flip1995]

Good point. Do you know how we can query the name of the main function (if redefined) from rustc?

[matthiaskrgr]

Hmm, maybe this can help? rust-lang/rust@b7cefd0

[flip1995]

That commit is from Jan 2013, so way before Rust 1.0. I don't think any of this code still exists :D

Maybe this:
https://doc.rust-lang.org/nightly/nightly-rustc/syntax/entry/enum.EntryPointType.html

@Urriel if you want to look into that, feel free to do so. Otherwise we can leave this to future work.

[phansch]

You could try using is_entrypoint_fn from our utils:

rust-clippy/clippy_lints/src/utils/mod.rs

Line 397 in 0814207

pub fn is_entrypoint_fn(cx: &LateContext<'_, '_>, def_id: DefId) -> bool {

[matthiaskrgr]

I don't think it should say "changelog: none" here :)

[Urriel]

I don't think it should say "changelog: none" here :)

I need to add the documentation to the code too :)

[Urriel]

#![feature(main)]

#[main]

I cannot make a test compiling around this, are you sure it is not the#[start] that you are talking about?

[flip1995]

There is both start and main. Here is a working test for main: Playground

[Urriel]

"No main function was detected, so your code was compiled but not run. If you'd like to execute your code, please add a main function."

So the #[main] does not create an entrypoint, do I get this right?

[matthiaskrgr]

It seems that this works on playground once the proper run-mode is selected!
rust-lang/rust-playground#503 (comment)

[Urriel]

impl LateLintPass<'_, '_> for MainRecursion {
    fn check_expr(&mut self, cx: &LateContext<'_, '_>, expr: &hir::Expr) {
        if self.has_no_std_attr {
            return;
        }

        if_chain! {
            if let hir::ExprKind::Call(func, _) = &expr.node;
            if is_entrypoint_fn(cx, func.hir_id.owner_def_id()) || is_entrypoint_fn(cx, expr.hir_id.owner_def_id());
            then {
                span_help_and_lint(
                    cx,
                    MAIN_RECURSION,
                    expr.span,
                    "recursing into `main()`",
                    "consider using another function for this recursion"
                )
            }
        }
    }
}

The code above does not seem to find the error here:

#![feature(main)]

#[warn(clippy::main_recursion)]
#[allow(unconditional_recursion)]
#[main]
fn a() {
    a();
}

I feel like I'm spamming dumb questions since the beginning of this PR -_-

[flip1995]

Can you post the output of

dbg!(func.hir_id.owner_def_id());
dbg!(expr.hir_id.owner_def_id());

I feel like I'm spamming dumb questions since the beginning of this PR -_-

There are no dumb questions! And we all needed to learn once. So please keep asking (and contributing 😉)!

[Urriel]

  22 + │ [clippy_lints/src/main_recursion.rs:65] expr.hir_id.owner_def_id() = DefId(0:18 ~ no_std_main
       │ _recursion[317d]::main[0])
  23 + │ [clippy_lints/src/main_recursion.rs:67] func.hir_id.owner_def_id() = DefId(0:18 ~ no_std_main
       │ _recursion[317d]::main[0])
  24 + │ error: recursing into `main()`
  25 + │   --> $DIR/no_std_main_recursion.rs:18:9
  26 + │    |
  27 + │ LL |         main(argc, argv);
  28 + │    |         ^^^^
  29 + │    |
  30 + │    = note: `-D clippy::main-recursion` implied by `-D warnings`
  31 + │    = help: consider using another function for this recursion

   1   │ error: recursing into `main()`
   2   │   --> $DIR/entrypoint_recursion.rs:7:5
   3   │    |
   4   │ LL |     println!("Hello, World!");
   5   │    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
   6   │    |
   7   │    = note: `-D clippy::main-recursion` implied by `-D warnings`
   8   │    = help: consider using another function for this recursion
   9   │    = note: this error originates in a macro outside of the current crate (in Nightly builds, 
       │ run with -Z external-macro-backtrace for more info)
  10   │ 
  11   │ error: recursing into `main()`
  12   │   --> $DIR/entrypoint_recursion.rs:7:5
  13   │    |
  14   │ LL |     println!("Hello, World!");
  15   │    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
  16   │    |
  17   │    = help: consider using another function for this recursion
  18   │    = note: this error originates in a macro outside of the current crate (in Nightly builds, 
       │ run with -Z external-macro-backtrace for more info)
  19   │ 
  20   │ error: recursing into `main()`
  21   │   --> $DIR/entrypoint_recursion.rs:8:5
  22   │    |
  23   │ LL |     a();
  24   │    |     ^
  25   │    |
  26   │    = help: consider using another function for this recursion
  27   │ 
  28   │ error: aborting due to 3 previous errors

Also I found out I cannot register my lint for both early and late pass...
Testing both expr and func does not change anything in the output.
And I get a false positive for the macro println!

[flip1995]

I cannot register my lint for both early and late pass...

Yeah, a lint can only be either of them. Just register it as a LatePass 👍

Oh, I meant the debug information when you run it on this code:

#![feature(main)]

#[warn(clippy::main_recursion)]
#[allow(unconditional_recursion)]
#[main]
fn a() {
    a();
}

[Urriel]

yes I can remove the println! but if I get a false positive with that code, it means I need a check more accurate than just the entrypoint.

[flip1995]

Oh sorry, I didn't realized the println! FPs. You take the def_id of the owner of the expr. the owner, as specified by the rustc documentation is

[...] the owner, which is the DefIndex of the directly enclosing hir::Item, [...] (i.e., the closest "item-like")

The closest "item-like" is the main()/a()-function. This function is an entry point, so owner_def_id() will not give you the desired results.

What you want to do, is to get the ExprKind::Path of the func expr. This gives you a QPath, which will give you the

Path to a definition

From there you can get the def_id()

[Urriel]

Do you have a specific format for the changelog tag in this pull request?

[flip1995]

Not really. Just

changelog: short description of what was changed.

You practically already wrote it:

changelog: Add MainRecursion lint

[flip1995]

Only some minor things to fix and this should be good to go.

[flip1995]

Now you can remove the new() function and use ::default() in lib.rs

[Urriel]

Oh, I saw this one above!

[Urriel]

it seems like the code for building the no_std on linux does not work elsewhere

[flip1995]

Hmmm. Could you try adding

#![cfg(target_os = "linux")]

to the top of the file?

[flip1995]

Oh sorry, I didn't get a ping for your last change.

Sooo, adding cfg didn't work (well, that should've been obvious to me).

I took a closer look on compiletest-rs and it seems you can add

// ignore-macos
// ignore-windows

to the top of the file and the test should be ignored on these platforms.

---

Keep in mind that you have to run tests/ui/update-all-references.sh after changing the test code.

[flip1995]

It seems that github doesn't notify you on force pushes to a pull request 🤔

Reopening for travis run

[Urriel]

It seems that github doesn't notify you on force pushes to a pull request thinking

Reopening for travis run

Oh alright, let me know :)

[flip1995]

You have to run ./util/dev update_lints and while you're at it also run ./util/dev fmt to be on the save side.

[flip1995]

@bors r+

[bors]

📌 Commit a922f80 has been approved by flip1995

[bors]

⌛ Testing commit a922f80 with merge 139e49b...

[bors]

💔 Test failed - status-appveyor

[Urriel]

Oh sorry, I forgot about this, do we still need to reformat ?

[flip1995]

The test ui\crate_level_checks\no_std_main_recursion.rs has to be ignored on windows and macos

[flip1995]

@bors r+

[bors]

📌 Commit dabf599 has been approved by flip1995

[bors]

⌛ Testing commit dabf599 with merge a813c05...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing a813c05 to master...