Skip to content
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

Stability annotations on generic parameters (take 2) #72314

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions src/librustc_metadata/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1612,13 +1612,17 @@ impl EncodeContext<'tcx> {
EntryKind::TypeParam,
default.is_some(),
);
if default.is_some() {
self.encode_stability(def_id.to_def_id());
}
}
GenericParamKind::Const { .. } => {
self.encode_info_for_generic_param(
def_id.to_def_id(),
EntryKind::ConstParam,
true,
);
// FIXME(const_generics:defaults)
varkor marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
40 changes: 32 additions & 8 deletions src/librustc_middle/middle/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,9 +280,15 @@ impl<'tcx> TyCtxt<'tcx> {
/// If `id` is `Some(_)`, this function will also check if the item at `def_id` has been
/// deprecated. If the item is indeed deprecated, we will emit a deprecation lint attached to
/// `id`.
pub fn eval_stability(self, def_id: DefId, id: Option<HirId>, span: Span) -> EvalResult {
pub fn eval_stability(
self,
def_id: DefId,
id: Option<HirId>,
span: Span,
check_deprecation: bool,
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved
) -> EvalResult {
// Deprecated attributes apply in-crate and cross-crate.
if let Some(id) = id {
if let (Some(id), true) = (id, check_deprecation) {
if let Some(depr_entry) = self.lookup_deprecation_entry(def_id) {
let parent_def_id = self.hir().local_def_id(self.hir().get_parent_item(id));
let skip = self
Expand Down Expand Up @@ -377,21 +383,39 @@ impl<'tcx> TyCtxt<'tcx> {
/// Additionally, this function will also check if the item is deprecated. If so, and `id` is
/// not `None`, a deprecated lint attached to `id` will be emitted.
pub fn check_stability(self, def_id: DefId, id: Option<HirId>, span: Span) {
self.check_stability_internal(def_id, id, span, true, |span, def_id| {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this extra method necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR: many unfixable errors are produced if we call self.sess.delay_span_bug(span, &format!("encountered unmarked API: {:?}", def_id)); on generic parameters.

Generic parameters are not always marked stable.
This occurs when a generic from a dependency is used in a staged_api crate.
I believe it's caused by the parameter being instantiated in their users/callers crate rather than at the definition site.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so the issue is that generic parameters are the only language feature that has optional stability: everything else is either not a staged_api item, or is either explicitly marked stable or unstable. Maybe we could call this function check_optional_stability or something like that, and give the reason why we want it (i.e. specifically mention the case of generic parameters).

// The API could be uncallable for other reasons, for example when a private module
// was referenced.
self.sess.delay_span_bug(span, &format!("encountered unmarked API: {:?}", def_id));
})
}

/// Checks if an item is stable or error out.
///
/// If the item defined by `def_id` is unstable and the corresponding `#![feature]` does not
/// exist, emits an error.
///
/// Additionally when `inherit_dep` is `true`, this function will also check if the item is deprecated. If so, and `id` is
/// not `None`, a deprecated lint attached to `id` will be emitted.
pub fn check_stability_internal(
self,
def_id: DefId,
id: Option<HirId>,
span: Span,
check_deprecation: bool,
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) {
match self.eval_stability(def_id, id, span, check_deprecation) {
EvalResult::Allow => {}
EvalResult::Deny { feature, reason, issue, is_soft } => {
report_unstable(self.sess, feature, reason, issue, is_soft, span, soft_handler)
}
EvalResult::Unmarked => {
// The API could be uncallable for other reasons, for example when a private module
// was referenced.
self.sess.delay_span_bug(span, &format!("encountered unmarked API: {:?}", def_id));
}
EvalResult::Unmarked => unmarked(span, def_id),
}
}

Expand Down
73 changes: 59 additions & 14 deletions src/librustc_passes/stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,20 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
attrs: &[Attribute],
item_sp: Span,
kind: AnnotationKind,
inherit_deprecation: bool,
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved
visit_children: F,
) where
F: FnOnce(&mut Self),
{
if !self.tcx.features().staged_api {
self.forbid_staged_api_attrs(hir_id, attrs, item_sp, kind, visit_children);
self.forbid_staged_api_attrs(
hir_id,
attrs,
item_sp,
kind,
inherit_deprecation,
visit_children,
);
return;
}

Expand Down Expand Up @@ -106,8 +114,11 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
// If parent is deprecated and we're not, inherit this by merging
// deprecated_since and its reason.
if let Some(parent_stab) = self.parent_stab {
if parent_stab.rustc_depr.is_some() && stab.rustc_depr.is_none() {
stab.rustc_depr = parent_stab.rustc_depr
if inherit_deprecation
&& parent_stab.rustc_depr.is_some()
&& stab.rustc_depr.is_none()
{
stab.rustc_depr = parent_stab.rustc_depr.clone()
}
}

Expand Down Expand Up @@ -157,7 +168,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
if stab.is_none() {
debug!("annotate: stab not found, parent = {:?}", self.parent_stab);
if let Some(stab) = self.parent_stab {
if stab.level.is_unstable() {
if inherit_deprecation && stab.level.is_unstable() {
self.index.stab_map.insert(hir_id, stab);
}
}
Expand Down Expand Up @@ -200,6 +211,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
attrs: &[Attribute],
item_sp: Span,
kind: AnnotationKind,
inherit_deprecation: bool,
visit_children: impl FnOnce(&mut Self),
) {
// Emit errors for non-staged-api crates.
Expand Down Expand Up @@ -227,7 +239,7 @@ impl<'a, 'tcx> Annotator<'a, 'tcx> {
// Propagate unstability. This can happen even for non-staged-api crates in case
// -Zforce-unstable-if-unmarked is set.
if let Some(stab) = self.parent_stab {
if stab.level.is_unstable() {
if inherit_deprecation && stab.level.is_unstable() {
self.index.stab_map.insert(hir_id, stab);
}
}
Expand Down Expand Up @@ -280,54 +292,82 @@ impl<'a, 'tcx> Visitor<'tcx> for Annotator<'a, 'tcx> {
}
hir::ItemKind::Struct(ref sd, _) => {
if let Some(ctor_hir_id) = sd.ctor_hir_id() {
self.annotate(ctor_hir_id, &i.attrs, i.span, AnnotationKind::Required, |_| {})
self.annotate(
ctor_hir_id,
&i.attrs,
i.span,
AnnotationKind::Required,
true,
|_| {},
)
}
}
_ => {}
}

self.annotate(i.hir_id, &i.attrs, i.span, kind, |v| intravisit::walk_item(v, i));
self.annotate(i.hir_id, &i.attrs, i.span, kind, true, |v| intravisit::walk_item(v, i));
self.in_trait_impl = orig_in_trait_impl;
}

fn visit_trait_item(&mut self, ti: &'tcx hir::TraitItem<'tcx>) {
self.annotate(ti.hir_id, &ti.attrs, ti.span, AnnotationKind::Required, |v| {
self.annotate(ti.hir_id, &ti.attrs, ti.span, AnnotationKind::Required, true, |v| {
intravisit::walk_trait_item(v, ti);
});
}

fn visit_impl_item(&mut self, ii: &'tcx hir::ImplItem<'tcx>) {
let kind =
if self.in_trait_impl { AnnotationKind::Prohibited } else { AnnotationKind::Required };
self.annotate(ii.hir_id, &ii.attrs, ii.span, kind, |v| {
self.annotate(ii.hir_id, &ii.attrs, ii.span, kind, true, |v| {
intravisit::walk_impl_item(v, ii);
});
}

fn visit_variant(&mut self, var: &'tcx Variant<'tcx>, g: &'tcx Generics<'tcx>, item_id: HirId) {
self.annotate(var.id, &var.attrs, var.span, AnnotationKind::Required, |v| {
self.annotate(var.id, &var.attrs, var.span, AnnotationKind::Required, true, |v| {
if let Some(ctor_hir_id) = var.data.ctor_hir_id() {
v.annotate(ctor_hir_id, &var.attrs, var.span, AnnotationKind::Required, |_| {});
v.annotate(
ctor_hir_id,
&var.attrs,
var.span,
AnnotationKind::Required,
true,
|_| {},
);
}

intravisit::walk_variant(v, var, g, item_id)
})
}

fn visit_struct_field(&mut self, s: &'tcx StructField<'tcx>) {
self.annotate(s.hir_id, &s.attrs, s.span, AnnotationKind::Required, |v| {
self.annotate(s.hir_id, &s.attrs, s.span, AnnotationKind::Required, true, |v| {
intravisit::walk_struct_field(v, s);
});
}

fn visit_foreign_item(&mut self, i: &'tcx hir::ForeignItem<'tcx>) {
self.annotate(i.hir_id, &i.attrs, i.span, AnnotationKind::Required, |v| {
self.annotate(i.hir_id, &i.attrs, i.span, AnnotationKind::Required, true, |v| {
intravisit::walk_foreign_item(v, i);
});
}

fn visit_macro_def(&mut self, md: &'tcx hir::MacroDef<'tcx>) {
self.annotate(md.hir_id, &md.attrs, md.span, AnnotationKind::Required, |_| {});
self.annotate(md.hir_id, &md.attrs, md.span, AnnotationKind::Required, true, |_| {});
}

fn visit_generic_param(&mut self, p: &'tcx hir::GenericParam<'tcx>) {
let kind = match &p.kind {
// FIXME(const_generics:defaults)
varkor marked this conversation as resolved.
Show resolved Hide resolved
hir::GenericParamKind::Type { default, .. } if default.is_some() => {
AnnotationKind::Container
}
_ => AnnotationKind::Prohibited,
};

self.annotate(p.hir_id, &p.attrs, p.span, kind, false, |v| {
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved
intravisit::walk_generic_param(v, p);
});
}
}

Expand Down Expand Up @@ -401,6 +441,10 @@ impl<'a, 'tcx> Visitor<'tcx> for MissingStabilityAnnotations<'a, 'tcx> {
fn visit_macro_def(&mut self, md: &'tcx hir::MacroDef<'tcx>) {
self.check_missing_stability(md.hir_id, md.span);
}

// Note that we don't need to `check_missing_stability` for default generic parameters,
// as we assume that any default generic parameters without attributes are automatically
// stable (assuming they have not inherited instability from their parent).
}

fn new_index(tcx: TyCtxt<'tcx>) -> Index<'tcx> {
Expand Down Expand Up @@ -464,6 +508,7 @@ fn new_index(tcx: TyCtxt<'tcx>) -> Index<'tcx> {
&krate.item.attrs,
krate.item.span,
AnnotationKind::Required,
true,
|v| intravisit::walk_crate(v, krate),
);
}
Expand Down
11 changes: 10 additions & 1 deletion src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,16 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
(GenericParamDefKind::Lifetime, GenericArg::Lifetime(lt)) => {
self.ast_region_to_region(&lt, Some(param)).into()
}
(GenericParamDefKind::Type { .. }, GenericArg::Type(ty)) => {
(GenericParamDefKind::Type { has_default, .. }, GenericArg::Type(ty)) => {
if *has_default {
tcx.check_stability_internal(
param.def_id,
Some(arg.id()),
arg.span(),
false,
|_, _| (),
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved
)
}
if let (hir::TyKind::Infer, false) = (&ty.kind, self.allow_ty_infer()) {
inferred_params.push(ty.span);
tcx.types.err.into()
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1220,7 +1220,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
if let Some(uc) = unstable_candidates {
applicable_candidates.retain(|&(p, _)| {
if let stability::EvalResult::Deny { feature, .. } =
self.tcx.eval_stability(p.item.def_id, None, self.span)
self.tcx.eval_stability(p.item.def_id, None, self.span, true)
{
uc.push((p, feature));
return false;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#![crate_type = "lib"]
#![feature(staged_api)]

#![stable(feature = "stable_test_feature", since = "1.0.0")]

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub trait Trait1<#[unstable(feature = "unstable_default", issue = "none")] T = ()> {
#[stable(feature = "stable_test_feature", since = "1.0.0")]
fn foo() -> T;
}

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub trait Trait2<#[unstable(feature = "unstable_default", issue = "none")] T = usize> {
#[stable(feature = "stable_test_feature", since = "1.0.0")]
fn foo() -> T;
}

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub trait Trait3<T = ()> {
#[stable(feature = "stable_test_feature", since = "1.0.0")]
fn foo() -> T;
}

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub struct Struct1<#[unstable(feature = "unstable_default", issue = "none")] T = usize> {
#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub field: T,
}

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub struct Struct2<T = usize> {
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved
#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub field: T,
}


#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub const STRUCT1: Struct1 = Struct1 { field: 1 };

#[stable(feature = "stable_test_feature", since = "1.0.0")]
pub const STRUCT2: Struct2 = Struct2 { field: 1 };
59 changes: 59 additions & 0 deletions src/test/ui/stability-attribute/generics-default-stability.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// aux-build:unstable_generic_param.rs
Avi-D-coder marked this conversation as resolved.
Show resolved Hide resolved

extern crate unstable_generic_param;

use unstable_generic_param::*;

struct R;

impl Trait1 for S {
fn foo() -> () { () } // ok
}

struct S;

impl Trait1<usize> for S { //~ ERROR use of unstable library feature 'unstable_default'
fn foo() -> usize { 0 }
}

impl Trait1<isize> for S { //~ ERROR use of unstable library feature 'unstable_default'
fn foo() -> isize { 0 }
}

impl Trait2<usize> for S { //~ ERROR use of unstable library feature 'unstable_default'
Aaron1011 marked this conversation as resolved.
Show resolved Hide resolved
fn foo() -> usize { 0 }
}

impl Trait3<usize> for S {
fn foo() -> usize { 0 } // ok
}

fn main() {
// let _ = S;

// let _ = Struct1 { field: 1 }; //~ ERROR use of unstable library feature 'unstable_default'
// let _: Struct1 = Struct1 { field: 1 }; //~ ERROR use of unstable library feature 'unstable_default'
// let _: Struct1<isize> = Struct1 { field: 1 }; //~ ERROR use of unstable library feature 'unstable_default'

// let _ = STRUCT1; // ok
// let _: Struct1 = STRUCT1; // ok
// let _: Struct1<usize> = STRUCT1; //~ ERROR use of unstable library feature 'unstable_default'
// let _: Struct1<usize> = STRUCT1; //~ ERROR use of unstable library feature 'unstable_default'
// let _ = STRUCT1.field; // ok
// let _: usize = STRUCT1.field; //~ ERROR use of unstable library feature 'unstable_default'
// let _ = STRUCT1.field + 1; //~ ERROR use of unstable library feature 'unstable_default'
// let _ = STRUCT1.field + 1usize; //~ ERROR use of unstable library feature 'unstable_default'

// let _ = Struct2 { field: 1 }; // ok
// let _: Struct2 = Struct2 { field: 1 }; // ok
// let _: Struct2<usize> = Struct2 { field: 1 }; // ok

// let _ = STRUCT2;
// let _: Struct2 = STRUCT2; // ok
// let _: Struct2<usize> = STRUCT2; // ok
// let _: Struct2<usize> = STRUCT2; // ok
// let _ = STRUCT2.field; // ok
// let _: usize = STRUCT2.field; // ok
// let _ = STRUCT2.field + 1; // ok
// let _ = STRUCT2.field + 1usize; // ok
}
Loading