Don't generate minification variables if not needed

[GuillaumeGomez]

Follow up of #59158 and #59157.

r? @QuietMisdreavus

cc @jdm

[QuietMisdreavus]

I wonder if we can make a test for this. Can we use rustdoc tests to look at the search index javascript? Since the initial minification variables are always going to have the same line to initialize them, we could look for something like that. I think we can also use an auxiliary crate to test what happens when a dependency has minified the search index but a downstream crate has disabled it - from what i can tell, auxiliary doc builds use the properties from the aux file itself instead of the main test.

[GuillaumeGomez]

I could write a test which would simply load the searchIndex file and if an error occurs, it means the generation failed. However, this would require a new test-suite since we need to build using --disable-minification.

[QuietMisdreavus]

I was thinking something even simpler, doing a substring search using a @has rule in a regular rustdoc test. We can pass the --disable-minification flag in those just fine - a compile-flags:--disable-minification -Z unstable-options rule in the test should suffice.

[GuillaumeGomez]

Such a test would completely fail to trigger when a minification issue would appear. :-/

[ollie27]

Is this added complexity worth it to save a few bytes in the none minified search-index.js?

[GuillaumeGomez]

This is a simple change which generates less things. For me, it's totally worth it, although we can't really test it (or we'll need to create a whole new test suite, which I'd prefer to avoid).

[bors]

☔ The latest upstream changes (presumably #59950) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #60296) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

@QuietMisdreavus Could we get a review/decision here either way?

I'm personally also somewhat inclined to say that we shouldn't do this as the 10-20 bytes we're saving aren't really worth the complexity introduced here in my opinion.

[GuillaumeGomez]

Easy to say when you're not the one debugging the minified JS. :p

[GuillaumeGomez]

Oh also, it fixes a bug currently present in the code, it doesn't add a new feature. Therefore it seems pretty worth it (but my opinion is completely biased in here since I use this feature quite heavily).

[Mark-Simulacrum]

Ah, I was not aware that this fixes a bug -- I presumed that the variables don't hurt if they're unused, but I guess that's not true.

[GuillaumeGomez]

By default, people don't have this bug since it needs a nightly-only option to be enabled. So indeed, it doesn't hurt to have unused variables if they're really not used. However, in here in some cases, the code references to them.

[Dylan-DPC-zz]

@bors: r+

[bors]

📌 Commit 582c836 has been approved by Dylan-DPC

[ollie27]

This is dead code. The above:

if !line.starts_with(key) || line.starts_with(&format!(r#"{}["{}"]"#, key, krate)) {
    continue;
}

means that line.starts_with(MINIFIED_VARIABLES) can never be true. This means that the minification variables are missing if the last call to rustdoc used --disable-minification but previous ones didn't.

[ollie27]

@bors r-

Again I don't think this added complexity is worth it. Especially when it doesn't even work and has no tests.

[bors]

☔ The latest upstream changes (presumably #62041) made this pull request unmergeable. Please resolve the merge conflicts.

[joelpalmer]

Hi @GuillaumeGomez this is a ping from Triage. Please post an update on this PR at your earliest convenience.

[GuillaumeGomez]

@ollie27 is against it so I guess I'll just close it...