Tidy: Allow inline tests in the compiler

[WaffleLapkin]

It seems like the original reason for this check is irrelevant now, so let's just remove it.

TL;DR: this allows using #[cfg(test)] mod tests { ... } (inline module instead of mod tests;)

[rustbot]

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

[Mark-Simulacrum]

Do you have a pointer to what fixed the reason this was done? I believe cargo without this will recompile the base library and then compile the tests even if only the tests are changed. I'm not aware of what has changed to make that not true (though would love to be pleasantly surprised!)

[WaffleLapkin]

@Mark-Simulacrum hm, I'm not sure, but I do not see how

#[cfg(test)]
mod tests { /* ... */ }

and

#[cfg(test)]
mod tests;

might be different, iirc rustc expands the latter into the former.

---

#61097 looked like it was caused by the migration from unit tests to UI tests, which I thought was complete, but now re-reading it, maybe there is a problem with incremental re-builds 🤔

[ehuss]

#[cfg(test)]
mod tests;

is different in that during a normal build, Cargo will ignore any changes to the tests.rs file. But when built as a test, then it will monitor for changes to the file. For the compiler this is important because for the inline version, if you modify a test, that would require rebuilding the entirety of the compiler and then build the crate being unittested (which can take a long time). When it is a separate file, only the crate being unittested would need to be recompiled.

[WaffleLapkin]

I see...

This is weird though, mod tests { ... } is such a common pattern everywhere in rust, shouldn't we support it better?...

[ehuss]

That seems like an extremely difficult thing to accomplish. Cargo needs some way to very quickly determine if a file has changed and should trigger a rebuild. I can only imagine that to do intra-file change detection would require parsing the entire crate and doing name resolution and expansion to determine if only content within some cfg removed section has changed (or having some kind of line-to-used-cfg mapping). I think that would have a very high complexity and performance hit.

[WaffleLapkin]

@ehuss I'd imagine that the compiler output after a change under #[cfg(false)] is not changed, so you don't need to rebuild the reverse dependencies.

From your words I assume that cargo doesn't try to hash rustc output to determine if anything actually changed and always issues reverse-dep-rebuilds... Is this a deliberate thing, or just implementation detail?

[ehuss]

It might be feasible to hash and compare the outputs, but that might require a significant rewrite of how Cargo does change tracking. It also assumes a few factors, such as reproducible builds, that the source changes don't materially change span locations, and that Cargo knows all the outputs from rustc (which it currently doesn't, it just guesses in most cases).

[Mark-Simulacrum]

Note that even if we solved the reverse dependency problem, we would still do ~2x the work since we'd compile both the cfg=test crate and the non-cfg=test crate (just to detect it hasn't changed).

[jyn514]

I'm going to close this since it would regress compile times today. Feel free to reopen if you want to add a comment documenting why this change exists. If you want to pursue changing cargo's cache invalidation algorithm, it sounds like that would need fairly close consultation with the cargo team and I think Zulip is a better place for that than this PR.