Skip to content

de-macro-ize feature gate checking #66945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/librustc/hir/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ impl<'a> LoweringContext<'a> {
return;
}

if !self.sess.features_untracked().in_band_lifetimes {
if !self.sess.features_untracked().on(sym::in_band_lifetimes) {
return;
}

Expand Down Expand Up @@ -1394,7 +1394,7 @@ impl<'a> LoweringContext<'a> {
}
ImplTraitContext::Disallowed(pos) => {
let allowed_in = if self.sess.features_untracked()
.impl_trait_in_bindings {
.on(sym::impl_trait_in_bindings) {
"bindings or function and inherent method return types"
} else {
"function and inherent method return types"
Expand Down Expand Up @@ -2118,7 +2118,7 @@ impl<'a> LoweringContext<'a> {

fn lower_local(&mut self, l: &Local) -> (hir::Local, SmallVec<[NodeId; 1]>) {
let mut ids = SmallVec::<[NodeId; 1]>::new();
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
if let Some(ref ty) = l.ty {
let mut visitor = ImplTraitTypeIdVisitor { ids: &mut ids };
visitor.visit_ty(ty);
Expand All @@ -2130,7 +2130,7 @@ impl<'a> LoweringContext<'a> {
ty: l.ty
.as_ref()
.map(|t| self.lower_ty(t,
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
ImplTraitContext::OpaqueTy(Some(parent_def_id))
} else {
ImplTraitContext::Disallowed(ImplTraitPosition::Binding)
Expand Down
8 changes: 4 additions & 4 deletions src/librustc/hir/lowering/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,15 @@ impl LoweringContext<'_> {
ItemKind::Impl(.., None, _, _) => smallvec![i.id],
ItemKind::Static(ref ty, ..) => {
let mut ids = smallvec![i.id];
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
let mut visitor = ImplTraitTypeIdVisitor { ids: &mut ids };
visitor.visit_ty(ty);
}
ids
},
ItemKind::Const(ref ty, ..) => {
let mut ids = smallvec![i.id];
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
let mut visitor = ImplTraitTypeIdVisitor { ids: &mut ids };
visitor.visit_ty(ty);
}
Expand Down Expand Up @@ -285,7 +285,7 @@ impl LoweringContext<'_> {
hir::ItemKind::Static(
self.lower_ty(
t,
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
ImplTraitContext::OpaqueTy(None)
} else {
ImplTraitContext::Disallowed(ImplTraitPosition::Binding)
Expand All @@ -299,7 +299,7 @@ impl LoweringContext<'_> {
hir::ItemKind::Const(
self.lower_ty(
t,
if self.sess.features_untracked().impl_trait_in_bindings {
if self.sess.features_untracked().on(sym::impl_trait_in_bindings) {
ImplTraitContext::OpaqueTy(None)
} else {
ImplTraitContext::Disallowed(ImplTraitPosition::Binding)
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/infer/error_reporting/need_type_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use crate::infer::InferCtxt;
use crate::infer::type_variable::TypeVariableOriginKind;
use crate::ty::{self, Ty, Infer, TyVar};
use crate::ty::print::Print;
use syntax::source_map::DesugaringKind;
use syntax_pos::Span;
use syntax_pos::source_map::DesugaringKind;
use syntax_pos::{Span, symbol::sym};
use errors::{Applicability, DiagnosticBuilder};

use rustc_error_codes::*;
Expand Down Expand Up @@ -217,7 +217,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let is_named_and_not_impl_trait = |ty: Ty<'_>| {
&ty.to_string() != "_" &&
// FIXME: Remove this check after `impl_trait_in_bindings` is stabilized. #63527
(!ty.is_impl_trait() || self.tcx.features().impl_trait_in_bindings)
(!ty.is_impl_trait() || self.tcx.features().on(sym::impl_trait_in_bindings))
};

let ty_msg = match local_visitor.found_ty {
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/infer/opaque_types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use errors::DiagnosticBuilder;
use rustc::session::config::nightly_options;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::sync::Lrc;
use syntax_pos::Span;
use syntax_pos::{Span, symbol::sym};

use rustc_error_codes::*;

Expand Down Expand Up @@ -488,7 +488,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
conflict2: ty::Region<'tcx>,
) -> bool {
// If we have `#![feature(member_constraints)]`, no problems.
if self.tcx.features().member_constraints {
if self.tcx.features().on(sym::member_constraints) {
return false;
}

Expand Down
16 changes: 7 additions & 9 deletions src/librustc/lint/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,13 @@ impl<'a> LintLevelsBuilder<'a> {
// FIXME (#55112): issue unused-attributes lint if we thereby
// don't have any lint names (`#[level(reason = "foo")]`)
if let ast::LitKind::Str(rationale, _) = name_value.kind {
if !self.sess.features_untracked().lint_reasons {
feature_gate::feature_err(
&self.sess.parse_sess,
sym::lint_reasons,
item.span,
"lint reasons are experimental"
)
.emit();
}
feature_gate::gate_feature(
&self.sess.parse_sess,
self.sess.features_untracked(),
item.span,
sym::lint_reasons,
"lint reasons are experimental"
);
reason = Some(rationale);
} else {
bad_attr(name_value.span)
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
item_sp: Span, kind: AnnotationKind, visit_children: F)
where F: FnOnce(&mut Self)
{
if self.tcx.features().staged_api {
if self.tcx.features().on(sym::staged_api) {
// This crate explicitly wants staged API.
debug!("annotate(id = {:?}, attrs = {:?})", hir_id, attrs);
if let Some(..) = attr::find_deprecation(&self.tcx.sess.parse_sess, attrs, item_sp) {
Expand Down Expand Up @@ -393,7 +393,7 @@ impl<'tcx> Index<'tcx> {
pub fn new(tcx: TyCtxt<'tcx>) -> Index<'tcx> {
let is_staged_api =
tcx.sess.opts.debugging_opts.force_unstable_if_unmarked ||
tcx.features().staged_api;
tcx.features().on(sym::staged_api);
let mut staged_api = FxHashMap::default();
staged_api.insert(LOCAL_CRATE, is_staged_api);
let mut index = Index {
Expand Down Expand Up @@ -836,7 +836,7 @@ impl Visitor<'tcx> for Checker<'tcx> {

// There's no good place to insert stability check for non-Copy unions,
// so semi-randomly perform it here in stability.rs
hir::ItemKind::Union(..) if !self.tcx.features().untagged_unions => {
hir::ItemKind::Union(..) if !self.tcx.features().on(sym::untagged_unions) => {
let def_id = self.tcx.hir().local_def_id(item.hir_id);
let adt_def = self.tcx.adt_def(def_id);
let ty = self.tcx.type_of(def_id);
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2353,13 +2353,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
ObligationCauseCode::VariableType(_) => {
err.note("all local variables must have a statically known size");
if !self.tcx.features().unsized_locals {
if !self.tcx.features().on(sym::unsized_locals) {
err.help("unsized locals are gated as an unstable feature");
}
}
ObligationCauseCode::SizedArgumentType => {
err.note("all function arguments must have a statically known size");
if !self.tcx.features().unsized_locals {
if !self.tcx.features().on(sym::unsized_locals) {
err.help("unsized locals are gated as an unstable feature");
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2215,7 +2215,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

if let Some(principal) = data.principal() {
if !self.infcx.tcx.features().object_safe_for_dispatch {
if !self.infcx.tcx.features().on(sym::object_safe_for_dispatch) {
principal.with_self_ty(self.tcx(), self_ty)
} else if self.tcx().is_object_safe(principal.def_id()) {
principal.with_self_ty(self.tcx(), self_ty)
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::infer::{InferCtxt, InferOk};
use crate::lint;
use crate::traits::{self, coherence, FutureCompatOverlapErrorKind, ObligationCause, TraitEngine};
use rustc_data_structures::fx::FxHashSet;
use syntax_pos::DUMMY_SP;
use syntax_pos::{DUMMY_SP, symbol::sym};
use crate::traits::select::IntercrateAmbiguityCause;
use crate::ty::{self, TyCtxt, TypeFoldable};
use crate::ty::subst::{Subst, InternalSubsts, SubstsRef};
Expand Down Expand Up @@ -155,7 +155,7 @@ pub(super) fn specializes(

// The feature gate should prevent introducing new specializations, but not
// taking advantage of upstream ones.
if !tcx.features().specialization &&
if !tcx.features().on(sym::specialization) &&
(impl1_def_id.is_local() || impl2_def_id.is_local()) {
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions src/librustc/ty/constness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::ty::query::Providers;
use crate::hir::def_id::DefId;
use crate::hir;
use crate::ty::TyCtxt;
use syntax_pos::symbol::Symbol;
use syntax_pos::symbol::{sym, Symbol};
use crate::hir::map::blocks::FnLikeNode;
use syntax::attr;

Expand Down Expand Up @@ -42,7 +42,7 @@ impl<'tcx> TyCtxt<'tcx> {
return false;
}

if self.features().staged_api {
if self.features().on(sym::staged_api) {
// in order for a libstd function to be considered min_const_fn
// it needs to be stable and have no `rustc_const_unstable` attribute
match self.lookup_stability(def_id) {
Expand All @@ -56,7 +56,7 @@ impl<'tcx> TyCtxt<'tcx> {
}
} else {
// users enabling the `const_fn` feature gate can do what they want
!self.features().const_fn
!self.features().on(sym::const_fn)
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1473,7 +1473,7 @@ impl<'tcx> TyCtxt<'tcx> {
//
// * Otherwise, use the behavior requested via `-Z borrowck=...`

if self.features().nll { return BorrowckMode::Mir; }
if self.features().on(sym::nll) { return BorrowckMode::Mir; }

self.sess.opts.borrowck_mode
}
Expand Down Expand Up @@ -2437,7 +2437,7 @@ impl<'tcx> TyCtxt<'tcx> {

#[inline]
pub fn mk_diverging_default(self) -> Ty<'tcx> {
if self.features().never_type_fallback {
if self.features().on(sym::never_type_fallback) {
self.types.never
} else {
self.types.unit
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2898,7 +2898,7 @@ impl<'tcx> TyCtxt<'tcx> {
(ImplPolarity::Negative, ImplPolarity::Negative) => {}
};

let is_marker_overlap = if self.features().overlapping_marker_traits {
let is_marker_overlap = if self.features().on(sym::overlapping_marker_traits) {
let trait1_is_empty = self.impl_trait_ref(def_id1)
.map_or(false, |trait_ref| {
self.associated_item_def_ids(trait_ref.def_id).is_empty()
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/ty/wf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::ty::subst::SubstsRef;
use crate::traits::{self, AssocTypeBoundData};
use crate::ty::{self, ToPredicate, Ty, TyCtxt, TypeFoldable};
use std::iter::once;
use syntax::symbol::{kw, Ident};
use syntax::symbol::{kw, sym, Ident};
use syntax_pos::Span;
use crate::middle::lang_items;

Expand Down Expand Up @@ -538,7 +538,7 @@ impl<'a, 'tcx> WfPredicates<'a, 'tcx> {
// checking those

let defer_to_coercion =
self.infcx.tcx.features().object_safe_for_dispatch;
self.infcx.tcx.features().on(sym::object_safe_for_dispatch);

if !defer_to_coercion {
let cause = self.cause(traits::MiscObligation);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_codegen_utils/symbol_names_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ pub fn report_symbol_names(tcx: TyCtxt<'_>) {
// if the `rustc_attrs` feature is not enabled, then the
// attributes we are interested in cannot be present anyway, so
// skip the walk.
if !tcx.features().rustc_attrs {
if !tcx.features().on(sym::rustc_attrs) {
return;
}

Expand Down
69 changes: 36 additions & 33 deletions src/librustc_feature/active.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,22 @@ use syntax_pos::edition::Edition;
use syntax_pos::Span;
use syntax_pos::symbol::{Symbol, sym};

macro_rules! set {
($field: ident) => {{
fn f(features: &mut Features, _: Span) {
features.$field = true;
}
f as fn(&mut Features, Span)
}}
}

macro_rules! declare_features {
($(
$(#[doc = $doc:tt])* (active, $feature:ident, $ver:expr, $issue:expr, $edition:expr),
)+) => {
/// Represents active features that are currently being implemented or
/// currently being considered for addition/removal.
pub const ACTIVE_FEATURES:
&[Feature] =
&[$(
// (sym::$feature, $ver, $issue, $edition, set!($feature))
Feature {
state: State::Active { set: set!($feature) },
name: sym::$feature,
since: $ver,
issue: $issue,
edition: $edition,
description: concat!($($doc,)*),
}
),+];
pub const ACTIVE_FEATURES: &[Feature] = &[$(
Feature {
state: State::Active,
name: sym::$feature,
since: $ver,
issue: $issue,
edition: $edition,
description: concat!($($doc,)*),
}
),+];

/// A set of features to be used by later passes.
#[derive(Clone, Default)]
Expand All @@ -44,28 +32,43 @@ macro_rules! declare_features {
pub declared_lib_features: Vec<(Symbol, Span)>,
$(
$(#[doc = $doc])*
pub $feature: bool
$feature: bool
),+
}

impl Features {
/// Is the given feature enabled?
///
/// Panics if the symbol doesn't correspond to a declared feature.
pub fn on(&self, name: Symbol) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the field approach is rejected after all in favor of a method, could it be named enabled instead of on?
"on" looks like it's... an event handler of sorts? onKeyPress anyone?
The "on/off" association certainly didn't come immediately to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh; I went with .active(..) first, then changed to .enabled(..) and finally to .on(..) to make it shorter. 😂 .enabled(..) is fine by me.

match name {
$(sym::$feature => self.$feature,)+
Copy link
Contributor

@petrochenkov petrochenkov Dec 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

match is a pretty inefficient tool in this case (in terms of performance).
Simple linear (not even binary) search in an array could be better here.
And hash map would be both simple and fast, see how built-in attributes gating is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish we had field pointers (like &MyStruct::my_field).
They can be emulated with offset_of, but that would mean macros again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the field approach can be kept with the use of macros being minimized rather than fully eliminated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I had a HashSet but was hoping that the match would turn into a huge lookup table and be better that way. I can certainly try a HashSet again and see if it changes anything.

Otherwise, perhaps field accesses outside check.rs would be workable (ostensibly that's where the regressions are coming from in some hot loop somewhere?) -- that should still leave us with no macros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me -- why is the macro itself bad? It looks like this is still using a macro. It looks also like we're still generating the fields, so we why can't expose those is not clear to me.

_ => panic!("unknown feature `{}`", name),
}
}

/// Enable the given `feature`.
///
/// Panics if called on a non-active feature
/// or a symbol not corresponding to a declared feature.
pub fn enable(&mut self, feature: &Feature, _: Span) {
let Feature { name, .. } = feature;
match feature.state {
State::Active => match *name {
$(sym::$feature => self.$feature = true,)+
_ => panic!("unknown feature `{}`", name),
},
_ => panic!("called `set` on feature `{}` which is not `active`", name),
}
}

pub fn walk_feature_fields(&self, mut f: impl FnMut(&str, bool)) {
$(f(stringify!($feature), self.$feature);)+
}
}
};
}

impl Feature {
/// Sets this feature in `Features`. Panics if called on a non-active feature.
pub fn set(&self, features: &mut Features, span: Span) {
match self.state {
State::Active { set } => set(features, span),
_ => panic!("called `set` on feature `{}` which is not `active`", self.name)
}
}
}

// If you change this, please modify `src/doc/unstable-book` as well.
//
// Don't ever remove anything from this list; move them to `removed.rs`.
Expand Down
Loading