Do not apply #[do_not_recommend] if the feature flag is not set

[weiznich]

This commit adds additional checks for the feature flag as apparently it is possible to use this on a beta compiler without feature flags. This PR might be a candidate for backporting.

Reported in the bevy issue tracker: bevyengine/bevy#14591 (comment)

r? @compiler-errors

[compiler-errors]

Hm.. This current approach has the consequence of disabling the attribute even when it's applied to upstream crates that can use the feature, like libcore..

We may need to store this info somewhere in the impl itself 🤔

[compiler-errors]

This current approach has the consequence of disabling the attribute even when it's applied to upstream crates that can use the feature, like libcore..

Or actually, let's not worry about that.

@weiznich, can you split diagnostic::do_not_recommend out into rustc_do_not_recommend and use that for the standard library, in the same way that rustc_on_unimplemented was used in the standard library? The we only need to gate diagnostic::do_not_recommend during error reporting, so we can still rustc_do_not_recommend in in the standard library.

[weiznich]

There are two other alternatives that might be possible:

• Just check at the time of using that the compiler is a nightly compiler?
• Stabilize the feature

[compiler-errors]

1. Just check at the time of using that the compiler is a nightly compiler?

I don't see how this fixes anything? Can you explain? Specifically, I don't want to regress the error messages of stable code that uses the ? operator in the standard library.

2. Stabilize the feature

This seems like a weird way to fix this regression. We almost certainly need to experiment with do_not_recommend more, so I would personally say stabilizing it in its current state is premature.

[weiznich]

I don't see how this fixes anything? Can you explain? Specifically, I don't want to regress the error messages of stable code that uses the ? operator in the standard library.

You are right that doesn't fix the problem :(
(Although I fail to see how another attribute would resolve that as well.)

[compiler-errors]

(Although I fail to see how another attribute would resolve that as well.)

Because then you can continue gating usages of diagnostic::do_not_recommend behind enablement of the tcx.features().do_not_recommend feature flag, but you don't need to gate usages of rustc_do_not_recommend since those are only going to be used in the standard library, and will result in a hard error if they're misused in non-stdlib crates.

[compiler-errors]

@rustbot author

[bors]

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

[weiznich]

Because then you can continue gating usages of diagnostic::do_not_recommend behind enablement of the tcx.features().do_not_recommend feature flag, but you don't need to gate usages of rustc_do_not_recommend since those are only going to be used in the standard library, and will result in a hard error if they're misused in non-stdlib crates.

Thanks for clarifying. That sounds like it would work on a technical level, although I very much would prefer not to do this as I fear that it makes it harder to stabilize this feature as usages in the standard library might then be counted as active usages of the corresponding unstable feature as it strictly uses different code.

That written: I'm not sure if I find the capacity to implement a different solution (or even that renaming) sometime soon.

Edit: Turns out it wasn't that hard to track the relevant information as part of the attribute, although I'm not user if I put everything in the right place.

[compiler-errors]

although I very much would prefer not to do this as I fear that it makes it harder to stabilize this feature as usages in the standard library might then be counted as active usages of the corresponding unstable feature as it strictly uses different code.

I don't agree with this concern. This was not a concern when stabilizing on_unimplemented, and I'm not sure what you mean by it using different code -- it it would just be gating whether or not to apply this attribute depending on the presence of #![feature(do_not_recommend)].

I'm not too comfortable with this approach, and I wouldn't want to beta backport it either. T-compiler has a "revert first" approach when it comes to regressions though. Until we can settle on a design, could you just please disable do_not_recommend completely? If you cannot, then I can prepare a PR instead.

[compiler-errors]

@bors try @rust-timer queue -- Just gauging the impact of this approach anyways.

[bors]

⌛ Trying commit f518ce8 with merge ce5a56a...

[compiler-errors]

Well -- I guess I'm second guessing myself whether this even warrants a beta revert or not.

This only affects the error path, and given that this doesn't affect the successful compilation of code, I'm actually not certain this warrants either a fix or a revert (especially since an erroneous do_not_recommend is likely never detrimental).

[compiler-errors]

#128912 is an alternative that encodes do_not_recommend in the impl header.

I understand that it's less flexible than a general mechanism to gate arbitrary diagnostic attributes, but it's also less invasive than having to encode the gatedness of diagnostic attrs all the way down into the AST. Ideally we could spend some time thinking of the best representation for gating arbitrary diagnostic attributes separately from fixing this specific issue.

[bors]

☀️ Try build successful - checks-actions
Build commit: ce5a56a (ce5a56a5230e65f37f846548a87f761d1d06cded)

[rust-timer]

Finished benchmarking commit (ce5a56a): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.6%]	78
Regressions ❌ (secondary)	0.4%	[0.2%, 1.1%]	28
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.6%]	78

Max RSS (memory usage)

Results (primary 2.4%, secondary 1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.5%	[2.9%, 4.2%]	3
Regressions ❌ (secondary)	3.0%	[2.0%, 3.9%]	3
Improvements ✅ (primary)	-1.0%	[-1.0%, -1.0%]	1
Improvements ✅ (secondary)	-1.2%	[-1.2%, -1.2%]	1
All ❌✅ (primary)	2.4%	[-1.0%, 4.2%]	4

Cycles

Results (primary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-2.6%, 0.8%]	2

Binary size

Results (primary 0.1%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.3%]	61
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.3%]	61

Bootstrap: 760.743s -> 762.86s (0.28%)
Artifact size: 337.11 MiB -> 337.23 MiB (0.04%)

[weiznich]

T-compiler has a "revert first" approach when it comes to regressions though. Until we can settle on a design, could you just please disable do_not_recommend completely? If you cannot, then I can prepare a PR instead.

The last version of this PR already did this. I can push that version again as new PR tomorrow or on Monday. I just read you previous comments in such a way that you don't want to do that as it would regress the one error from rusts standard library that already uses the #[do_not_recommend] attribute.

As for the performance numbers: My feeling is that the expensive part here is the additional pass over the AST of the whole crate as done by the new visitor. We should be able to easily sidestep that by just adding the relevant logic to one of the existing visitors, right? And yes, the underlying problem of how to gate new diagnostic attributes is something that might be worth to discuss unrelated from this "bugfix". I personally would rather go for a general solution as otherwise we might run into this again and again for any future new attribute.

[compiler-errors]

As for the performance numbers: My feeling is that the expensive part here is the additional pass over the AST of the whole crate as done by the new visitor. We should be able to easily sidestep that by just adding the relevant logic to one of the existing visitors, right?

I'm not certain. This may also come from making the attribute struct larger 🤷

[weiznich]

This may also come from making the attribute struct larger 🤷

Given that the size of the struct did not change according to some quick checks via mem::size_of that's very unlikely, right? Anyway it should be simple to just put that into an existing mut_visit pass and rerun perf.

[weiznich]

Also: Given that your other PR is already accepted is there even interest in fixing that or having a backport PR? If that's not the case I rather would prefer not to spend anymore time.

[compiler-errors]

Actually, most of the changed perf tests are incr-*, which are incremental. I wonder if this is due to extra work when encoding/decoding these attributes 🤔

Yeah, let's not work any more on this PR; though if you want to discuss how to make sure this doesn't happen, feel free to reach out on zulip and we can brainstorm.

I'll backport nominate #128912, but I'll also mention to the compiler team about the alternative of doing nothing, or reverting all of the do_not_recommend behavior on beta. If T-compiler decides on the last option, I can prepare a PR so as to not waste your time.

[apiraino]

Beta backport declined in favor of #128912 as per Michael's suggestion. Discussed by compiler team on Zulip.

@rustbot label -beta-nominated