Skip to content

Fix detection of main function if there are expressions around it #140220

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

Merged
merged 5 commits into from
Apr 28, 2025
Merged

Fix detection of main function if there are expressions around it #140220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3536324
Fix detection of `main` function if there are expressions around it
GuillaumeGomez Apr 23, 2025
81438c0
Add regression ui test for #140162 and for #139651
GuillaumeGomez Apr 23, 2025
3ededc1
Improve code
GuillaumeGomez Apr 25, 2025
3ef98a5
If there is a `;` alone, we consider that the doctest needs to be put…
GuillaumeGomez Apr 25, 2025
aa69e3a
Fix bad handling of macros if there is already a `main` function
GuillaumeGomez Apr 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/librustdoc/doctest/make.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,15 +407,19 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result<ParseSourceIn
push_to_s(&mut info.crate_attrs, source, attr.span, &mut prev_span_hi);
}
}
let mut has_non_module_items = false;
for stmt in &body.stmts {
let mut is_extern_crate = false;
match stmt.kind {
StmtKind::Item(ref item) => {
is_extern_crate = check_item(&item, &mut info, crate_name);
}
StmtKind::Expr(ref expr) if matches!(expr.kind, ast::ExprKind::Err(_)) => {
reset_error_count(&psess);
return Err(());
StmtKind::Expr(ref expr) => {
if matches!(expr.kind, ast::ExprKind::Err(_)) {
reset_error_count(&psess);
return Err(());
}
has_non_module_items = true;
}
StmtKind::MacCall(ref mac_call) if !info.has_main_fn => {
Copy link
Member

@jyn514 jyn514 Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i think you need to also assume that macros can expand to expressions, not just items. in fact i think you need to do this for any statement other than an item.

Copy link
Member Author

Choose a reason for hiding this comment

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

I voluntarily ignored it for the simple fact that trying to match on a macro call is pretty much useless since we have no clue how it will be expanded. The fact that we match on fn main is already a pretty bad idea. If I start saying "everything that doesn't match the first condition means it's an expression", then this code would fail to compile:

include!("auxiliary/empty.rs");
fn main() {}

Because "auxiliary/empty.rs" would be considered an expression, meaning the whole code would be wrapped, and the compiler would be unhappy:

error: non-statement macro in statement position: include
  |
3 | include!("auxiliary/empty.rs");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So sadly, I think the best course of action here is to keep this ugly hack...

Copy link
Member

@fmease fmease Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While Jyn is obviously right regarding macro expansion, older versions of rustdoc (e.g., 1.86, 1.75) don't handle those top-level macros calls correctly either, I mean ones which expand to exprs. At least from what I've gathered. I tested:

//! ```
//! macro_rules! mk { () => { if true { return; } } }
//! mk!();
//! fn main() {}
//! ```

1.88, 1.86, 1.75: macro expansion ignores keyword `if` and any tokens following // the usage of `mk!` is likely invalid in item context.

So it's fine to keep treating macro calls as item macro calls. All of this is a heuristic anyway and I'm so glad GuillaumeGomez threw out the janky partially string-based heuristics in #138104 which also used to reparse the input string multiple times.

As it's the case with all heuristics, there will always be false positives and negatives.

Copy link
Member

@fmease fmease Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Guillaume, could you add a short comment above the MacroCall arm mentioning that we assume that the macro calls expand to item(s) even though they could expand to stmts and exprs, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

let mut iter = mac_call.mac.args.tokens.iter();
Expand All @@ -437,7 +441,11 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result<ParseSourceIn
}
}
}
_ => {}
// We do nothing in this case. Not marking it as `non_module_items` either.
StmtKind::Empty => {}
Copy link
Member

Choose a reason for hiding this comment

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

No, we should set has_non_module_items = true when encountering StmtKind::Empty since it represents lone ;s which are not valid items.

Consider:

//! ```
//! fn main() {};
//! ```

or

//! ```
//! ;fn main() {}
//! ```

which lead to expected item, found `;` on master.

Copy link
Member

Choose a reason for hiding this comment

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

However as a matter of fact, <=1.86 did not handle this 'correctly' either.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Apr 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

EDIT: This comment is completely wrong.

Weeeeell... Let me raise to you:

struct s{};

fn main() {}

:D

There is even tests/rustdoc-ui/doctest/failed-doctest-extra-semicolon-on-item.rs to check it. So sorry but I'll leave it as is for backward compatibility. :')

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, ignore my previous comment, it's not allowed to have a rogue semi-colon in code like that. I'll change it then.

_ => {
has_non_module_items = true;
}
}

// Weirdly enough, the `Stmt` span doesn't include its attributes, so we need to
Expand All @@ -462,6 +470,11 @@ fn parse_source(source: &str, crate_name: &Option<&str>) -> Result<ParseSourceIn
push_to_s(&mut info.crates, source, span, &mut prev_span_hi);
}
}
if has_non_module_items {
// FIXME: if `info.has_main_fn` is `true`, emit a warning here to mention that
Copy link
Member

Choose a reason for hiding this comment

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

👍 Could you open an issue for that unless we already have one for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it is: #140310

// this code will not be called.
info.has_main_fn = false;
}
Ok(info)
}
Err(e) => {
Expand Down
22 changes: 22 additions & 0 deletions tests/rustdoc-ui/doctest/test-main-alongside-exprs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// This test ensures that if there is an expression alongside a `main`
// function, it will not consider the entire code to be part of the `main`
// function and will generate its own function to wrap everything.
//
// This is a regression test for:
// * <https://github.com/rust-lang/rust/issues/140162>
// * <https://github.com/rust-lang/rust/issues/139651>
//@ compile-flags:--test
//@ normalize-stdout: "tests/rustdoc-ui/doctest" -> "$$DIR"
//@ normalize-stdout: "finished in \d+\.\d+s" -> "finished in $$TIME"
//@ check-pass

#![crate_name = "foo"]

//! ```
//! # if cfg!(miri) { return; }
//! use std::ops::Deref;
//!
//! fn main() {
//! println!("Hi!");
//! }
//! ```
6 changes: 6 additions & 0 deletions tests/rustdoc-ui/doctest/test-main-alongside-exprs.stdout
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@

running 1 test
test $DIR/test-main-alongside-exprs.rs - (line 15) ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME

Loading