Keep rename-imported main alive in dead-code analysis under --test

[MaximilianAzendorf]

Fixes #157608.

A function used as main via a rename import was wrongly reported as dead code under --test. The dead-code pass only seeded entry_fn, which --test replaces with the test harness' main; now it also seeds the resolver's main_def.

[rustbot]

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[mu001999]

r? me

[bjorn3]

Looks like the thing keeping fn main() alive with --test is

rust/compiler/rustc_builtin_macros/src/test_harness.rs

Lines 201 to 218 in a962594

	// Remove any #[rustc_main] from the AST so it doesn't
	// clash with the one we're going to add, but mark it as
	// #[allow(dead_code)] to avoid printing warnings.
	match entry_point_type(&item, self.depth == 0) {
	EntryPointType::MainNamed | EntryPointType::RustcMainAttr => {
	let allow_dead_code = attr::mk_attr_nested_word(
	&self.sess.psess.attr_id_generator,
	ast::AttrStyle::Outer,
	ast::Safety::Default,
	sym::allow,
	sym::dead_code,
	self.def_site,
	);
	item.attrs.retain(|attr| !attr.has_name(sym::rustc_main));
	item.attrs.push(allow_dead_code);
	}
	EntryPointType::None | EntryPointType::OtherMain => {}
	};

Maybe handling OtherMain there as well would be possible. Or alternatively maybe the allow(dead_code) inserted there could be removed together with the && main_def.is_import here?

View changes since the review

[MaximilianAzendorf]

I looked a bit into OtherMain, and it's not helping us here imho. Things only get classified as OtherMain when they're literally called main while not living at the root level (e.g. mod foo { fn main() {} }, see entry_point_type). In the issue's case the relevant items are a renamed use ... as main; import and a function whose name isn't main, so both end up as None; handling OtherMain in EntryPointCleaner changes nothing for them.

And on removing the && main_def.is_import check: that breaks tests/ui/lint/dead-code/lint-dead-code-2.rs. There, an explicit #[rustc_main] demotes a separate fn main:

#[rustc_main]
fn actual_main() { 
    // ...
}

fn main() { //~ ERROR: function `main` is never used
    dead_fn();
}

main_def still resolves the name main to that demoted fn main, so without the is_import gate we'd mark it as live (and dead_fn with it), and the expected "never used" errors would disappear.

Dropping the harness #[allow(dead_code)] would break a #[rustc_main] fn foo under --test: that insertion is the only thing keeping foo alive, and dead.rs can't recover that, because the harness strips the #[rustc_main] attribute during expansion, so by the time dead-code analysis runs there's nothing left that tells us foo was an entry point (and main_def doesn't point at it).

So I think how the PR currently is the least invasive option that is producing correct behavior, but I agree that it's a bit "unelegant". I also changed the comment in the PR to make it a bit more clear.

[mu001999]

I think we can handle use_tree in fn entry_point_type, if it contains main, we can add #[allow(dead_code, unused_imports)] on it.

And also handle OtherMain, because we may have use m::*; mod m { pub fn main() {} }.

[MaximilianAzendorf]

Regarding use_tree: As I see it, it's not possible to handle the glob use case with that as we need name resolution for it.

I also tried getting use out of OtherMain and I still don't think that we can or should make use of it here. See your proposed test case: m::main is the entry point in that file and should therefore not be flagged dead, but is classified as OtherMain, regardless of if it's used outside via use m::* or not. OtherMain (and UseTree) is purely syntactic, while the question of whether a function would be the entry point in a normal (non---test) build can only be answered post name resolution. The only way I see to make use of OtherMain is to flag every OtherMain as live, which is over-suppressing the dead-code diagnostic.

I'm fairly new to the code base, so please forgive me if I'm missing something obvious here, but to make a long story short: I don't see a way other than the original approach of this PR (relying on information only available post-name-resolution, which neither use_tree nor OtherMain provide) to accommodate for all test cases.

[mu001999]

+1 to #157646 (comment)

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[MaximilianAzendorf]

@rustbot ready

[mu001999]

See #157646 (comment)

@rustbot author

[mu001999]

Also, please add tests for the following and other variants:

#![deny(dead_code)]

mod m {
    pub fn main() {}
}

use m::*;