#[allow_internal_unstable] does not allow unstable library features when applied on functions

[kennytm]

The #[allow_internal_unstable] attribute can only enable compiler features. Library features cannot be enabled.

Examples:

#![feature(allow_internal_unstable)]
// #![feature(wrapping_next_power_of_two, const_transmute)]

#[allow_internal_unstable(wrapping_next_power_of_two)]
fn g(a: u64) -> u64 {
    a.wrapping_next_power_of_two()
}

#[allow_internal_unstable(const_transmute)]
const fn h(a: u64) -> i64 {
    unsafe { std::mem::transmute(a) }
}

• Expected: can be built without errors.
• Actual: still complains about missing #![feature]s.

Rustc version = 1.43.0-nightly (2020-02-22 436494b8f8008b600d64)

[eddyb]

allow_internal_unstable is a macro attribute, the only bug here is that it doesn't complain when you put it on a non-macro.

[ecstatic-morse]

@eddyb This was changed as part of #63770.

[eddyb]

Ugh, that's an unprincipled hack, and the attribute is still not checked correctly. I had this example where it doesn't do anything without errors.

I can now see how it's used in libcore, but it should be named #[rustc_allow_const_fn_unstable(...)] or something, and be properly checked.

[eddyb]

Also, do we need that anymore? Given that we're starting to add more and more const fns.

[RalfJung]

Also, do we need that anymore? Given that we're starting to add more and more const fns.

The point of this is that we don't want to stabilize const fn that use unstable features (to avoid de-facto stabilizing unstable features of the const-eval engine). I think that is still very much needed.

[kennytm]

cc @Centril @oli-obk for use of #[allow_internal_unstable] around const fn. https://github.com/rust-lang/rustc-guide/blob/master/src/stability.md#allow_internal_unstable.

[ecstatic-morse]

The fix for this is described in #69373 (comment).

[liketechnik]

I would like to tackle this issue, but I'm not 100% sure how to go forward. If I understood correctly, I would just need to follow the instructions in the above fix.
This would mean I'd add the described check at the described line, right?

[oli-obk]

I believe that should indeed suffice to fix the issue, but I may have overlooked larger things

[liketechnik]

I don't think I'll be able to fix this issue on my own, I'm afraid.

I'm not familiar enough with the internal workings and the fix linked above cannot really be used like that anymore, if I understood the code correctly.

[oli-obk]

rust/compiler/rustc_mir/src/transform/check_consts/validation.rs

Lines 802 to 828 in 62bfcfd

	if let Some(gate) = is_unstable_const_fn(tcx, callee) {
	if self.span.allows_unstable(gate) {
	return;
	}
	// Calling an unstable function *always* requires that the corresponding gate
	// be enabled, even if the function has `#[allow_internal_unstable(the_gate)]`.
	if !tcx.features().declared_lib_features.iter().any(|&(sym, _)| sym == gate) {
	self.check_op(ops::FnCallUnstable(callee, Some(gate)));
	return;
	}
	// If this crate is not using stability attributes, or the caller is not claiming to be a
	// stable `const fn`, that is all that is required.
	if !self.ccx.is_const_stable_const_fn() {
	return;
	}
	// Otherwise, we are something const-stable calling a const-unstable fn.
	if super::allow_internal_unstable(tcx, caller, gate) {
	return;
	}
	self.check_op(ops::FnCallUnstable(callee, Some(gate)));
	return;
	}

is what handles this now.

So I guess we'll need to just remove

rust/compiler/rustc_mir/src/transform/check_consts/validation.rs

Lines 807 to 812 in 62bfcfd

	// Calling an unstable function *always* requires that the corresponding gate
	// be enabled, even if the function has `#[allow_internal_unstable(the_gate)]`.
	if !tcx.features().declared_lib_features.iter().any(|&(sym, _)| sym == gate) {
	self.check_op(ops::FnCallUnstable(callee, Some(gate)));
	return;
	}

[liketechnik]

I applied that change locally and it didn't fix the issue.

I think the error lies elsewhere, because compiling the above code with -Z treat-err-as-bug=1 and RUST_BACKTRACE=full creates a backtrace where the error seems to be created in rustc_middle::middle::stability::check_optional_stability (here's a gist with the code and the backtrace).

Edit:

rust/compiler/rustc_middle/src/middle/stability.rs

Lines 409 to 428 in e1d7681

	pub fn check_optional_stability(
	self,
	def_id: DefId,
	id: Option<HirId>,
	span: Span,
	unmarked: impl FnOnce(Span, DefId) -> (),
	) {
	let soft_handler = |lint, span, msg: &_| {
	self.struct_span_lint_hir(lint, id.unwrap_or(hir::CRATE_HIR_ID), span, |lint| {
	lint.build(msg).emit()
	})
	};
	match self.eval_stability(def_id, id, span) {
	EvalResult::Allow => {}
	EvalResult::Deny { feature, reason, issue, is_soft } => {
	report_unstable(self.sess, feature, reason, issue, is_soft, span, soft_handler)
	}
	EvalResult::Unmarked => unmarked(span, def_id),
	}
	}

[oli-obk]

oh.. oof. This means it's part of

rust/compiler/rustc_middle/src/middle/stability.rs

Line 296 in d902752

pub fn eval_stability(self, def_id: DefId, id: Option<HirId>, span: Span) -> EvalResult {

which... we don't really want to touch. That would allow us to allow allow arbitrary library features. I now realize that this issue was created for non-const-fn, I always thought this was about const fn and relevant features.

Yea, I don't think we should change anything here and keep requiring the feature on the entire crate.

So the only change left from this issue that I think we should address is to rename the attribute on functions to rustc_allow_const_fn_unstable and not have the same name as the attribute for macros.

[liketechnik]

My current understanding of what needs to be done to address the renaming:

• Add rustc_allow_const_fn_unstable to the internal attribute list

  rust/compiler/rustc_feature/src/builtin_attrs.rs

  Line 174 in 9832374

  pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[

  , the list of active feature

  rust/compiler/rustc_feature/src/active.rs

  Line 95 in 9832374

  declare_features! (

  and the symbol list

  rust/compiler/rustc_span/src/symbol.rs

  Line 22 in 9832374

  symbols! {

• Include feature names from both (either) allow_internal_unstable and rustc_allow_const_fn_unstable

  rust/compiler/rustc_attr/src/builtin.rs

  Lines 1012 to 1016 in 9832374

  	pub fn allow_internal_unstable<'a>(
  	sess: &'a Session,
  	attrs: &'a [Attribute],
  	) -> Option<impl Iterator<Item = Symbol> + 'a> {
  	let attrs = sess.filter_by_name(attrs, sym::allow_internal_unstable);

• Make sure that allow_internal_unstable can only be applied to macros (which would happen in compiler/rustc_passes/src/check_attr.rs?)
• Make sure that rustc_allow_const_fn_unstable can only be applied to functions

(Note: If somebody more experienced than me should rather deal with this, please tell me. I'm really enjoying working on this so far, but I also understand if I'm too much of a burden with that many (simple) questions...)

[oli-obk]

(Note: If somebody more experienced than me should rather deal with this, please tell me. I'm really enjoying working on this so far, but I also understand if I'm too much of a burden with that many (simple) questions...)

From my end it looks like you are approaching this in a well-organized manner, and not at all a burden!

* Include feature names from both (either) `allow_internal_unstable` and `rustc_allow_const_fn_unstable`

I would make that function private and take a Symbol argument that gets checked and reported instead. Then you can add public wrappers for both allow_internal_unstable and rustc_allow_const_fn_unstable that fill in that new argument with the appropriate symbol. This way we don't mix the two functions while still deduplicating their implementation.

• Make sure that allow_internal_unstable can only be applied to macros (which would happen in compiler/rustc_passes/src/check_attr.rs?)

• Make sure that rustc_allow_const_fn_unstable can only be applied to functions

Yes, both of these can be checked similarly to
https://github.com/rust-lang/rust/blob/master/compiler/rustc_passes/src/check_attr.rs#L86-L87