Skip to content

Commit

Permalink
Rollup merge of rust-lang#32199 - nikomatsakis:limiting-constants-in-…
Browse files Browse the repository at this point in the history
…patterns-2, r=pnkfelix

Restrict constants in patterns

This implements [RFC 1445](https://github.com/rust-lang/rfcs/blob/master/text/1445-restrict-constants-in-patterns.md). The primary change is to limit the types of constants used in patterns to those that *derive* `Eq` (note that implementing `Eq` is not sufficient). This has two main effects:

1. Floating point constants are linted, and will eventually be disallowed. This is because floating point constants do not implement `Eq` but only `PartialEq`. This check replaces the existing special case code that aimed to detect the use of `NaN`.
2. Structs and enums must derive `Eq` to be usable within a match.

This is a [breaking-change]: if you encounter a problem, you are most likely using a constant in an expression where the type of the constant is some struct that does not currently implement
`Eq`. Something like the following:

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

The easiest and most future compatible fix is to annotate the type in question with `#[derive(Eq)]` (note that merely *implementing* `Eq` is not enough, it must be *derived*):

```rust
struct SomeType { ... }
const SOME_CONST: SomeType = ...;

match foo {
    SOME_CONST => ...
}
```

Another good option is to rewrite the match arm to use an `if` condition (this is also particularly good for floating point types, which implement `PartialEq` but not `Eq`):

```rust
match foo {
    c if c == SOME_CONST => ...
}
```

Finally, a third alternative is to tag the type with `#[structural_match]`; but this is not recommended, as the attribute is never expected to be stabilized. Please see RFC rust-lang#1445 for more details.

cc rust-lang#31434

r? @pnkfelix
  • Loading branch information
Manishearth committed Mar 26, 2016
2 parents b8b17a5 + 2536ae5 commit 128b2ad
Show file tree
Hide file tree
Showing 25 changed files with 438 additions and 46 deletions.
15 changes: 15 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,19 @@ declare_lint! {
"type parameter default erroneously allowed in invalid location"
}

declare_lint! {
pub ILLEGAL_FLOATING_POINT_CONSTANT_PATTERN,
Warn,
"floating-point constants cannot be used in patterns"
}

declare_lint! {
pub ILLEGAL_STRUCT_OR_ENUM_CONSTANT_PATTERN,
Deny,
"constants of struct or enum type can only be used in a pattern if \
the struct or enum has `#[derive(PartialEq, Eq)]`"
}

declare_lint! {
pub MATCH_OF_UNIT_VARIANT_VIA_PAREN_DOTDOT,
Deny,
Expand Down Expand Up @@ -193,6 +206,8 @@ impl LintPass for HardwiredLints {
PRIVATE_IN_PUBLIC,
INACCESSIBLE_EXTERN_CRATE,
INVALID_TYPE_PARAM_DEFAULT,
ILLEGAL_FLOATING_POINT_CONSTANT_PATTERN,
ILLEGAL_STRUCT_OR_ENUM_CONSTANT_PATTERN,
MATCH_OF_UNIT_VARIANT_VIA_PAREN_DOTDOT,
CONST_ERR,
RAW_POINTER_DERIVE,
Expand Down
25 changes: 17 additions & 8 deletions src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -478,15 +478,24 @@ impl<'a, 'tcx> Folder for StaticInliner<'a, 'tcx> {
Some(Def::Const(did)) => {
let substs = Some(self.tcx.node_id_item_substs(pat.id).substs);
if let Some((const_expr, _)) = lookup_const_by_id(self.tcx, did, substs) {
const_expr_to_pat(self.tcx, const_expr, pat.span).map(|new_pat| {

if let Some(ref mut renaming_map) = self.renaming_map {
// Record any renamings we do here
record_renamings(const_expr, &pat, renaming_map);
match const_expr_to_pat(self.tcx, const_expr, pat.id, pat.span) {
Ok(new_pat) => {
if let Some(ref mut map) = self.renaming_map {
// Record any renamings we do here
record_renamings(const_expr, &pat, map);
}
new_pat
}

new_pat
})
Err(def_id) => {
self.failed = true;
self.tcx.sess.span_err(
pat.span,
&format!("constants of the type `{}` \
cannot be used in patterns",
self.tcx.item_path_str(def_id)));
pat
}
}
} else {
self.failed = true;
span_err!(self.tcx.sess, pat.span, E0158,
Expand Down
74 changes: 58 additions & 16 deletions src/librustc/middle/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use self::EvalHint::*;

use front::map as ast_map;
use front::map::blocks::FnLikeNode;
use lint;
use middle::cstore::{self, CrateStore, InlinedItem};
use middle::{infer, subst, traits};
use middle::def::Def;
Expand Down Expand Up @@ -323,10 +324,41 @@ impl ConstVal {
}
}

pub fn const_expr_to_pat(tcx: &TyCtxt, expr: &Expr, span: Span) -> P<hir::Pat> {
pub fn const_expr_to_pat(tcx: &ty::TyCtxt, expr: &Expr, pat_id: ast::NodeId, span: Span)
-> Result<P<hir::Pat>, DefId> {
let pat_ty = tcx.expr_ty(expr);
debug!("expr={:?} pat_ty={:?} pat_id={}", expr, pat_ty, pat_id);
match pat_ty.sty {
ty::TyFloat(_) => {
tcx.sess.add_lint(
lint::builtin::ILLEGAL_FLOATING_POINT_CONSTANT_PATTERN,
pat_id,
span,
format!("floating point constants cannot be used in patterns"));
}
ty::TyEnum(adt_def, _) |
ty::TyStruct(adt_def, _) => {
if !tcx.has_attr(adt_def.did, "structural_match") {
tcx.sess.add_lint(
lint::builtin::ILLEGAL_STRUCT_OR_ENUM_CONSTANT_PATTERN,
pat_id,
span,
format!("to use a constant of type `{}` \
in a pattern, \
`{}` must be annotated with `#[derive(PartialEq, Eq)]`",
tcx.item_path_str(adt_def.did),
tcx.item_path_str(adt_def.did)));
}
}
_ => { }
}

let pat = match expr.node {
hir::ExprTup(ref exprs) =>
PatKind::Tup(exprs.iter().map(|expr| const_expr_to_pat(tcx, &expr, span)).collect()),
PatKind::Tup(try!(exprs.iter()
.map(|expr| const_expr_to_pat(tcx, &expr,
pat_id, span))
.collect())),

hir::ExprCall(ref callee, ref args) => {
let def = *tcx.def_map.borrow().get(&callee.id).unwrap();
Expand All @@ -336,31 +368,41 @@ pub fn const_expr_to_pat(tcx: &TyCtxt, expr: &Expr, span: Span) -> P<hir::Pat> {
let path = match def.full_def() {
Def::Struct(def_id) => def_to_path(tcx, def_id),
Def::Variant(_, variant_did) => def_to_path(tcx, variant_did),
Def::Fn(..) => return P(hir::Pat {
Def::Fn(..) => return Ok(P(hir::Pat {
id: expr.id,
node: PatKind::Lit(P(expr.clone())),
span: span,
}),
})),
_ => unreachable!()
};
let pats = args.iter().map(|expr| const_expr_to_pat(tcx, &expr, span)).collect();
let pats = try!(args.iter()
.map(|expr| const_expr_to_pat(tcx, &**expr,
pat_id, span))
.collect());
PatKind::TupleStruct(path, Some(pats))
}

hir::ExprStruct(ref path, ref fields, None) => {
let field_pats = fields.iter().map(|field| codemap::Spanned {
span: codemap::DUMMY_SP,
node: hir::FieldPat {
name: field.name.node,
pat: const_expr_to_pat(tcx, &field.expr, span),
is_shorthand: false,
},
}).collect();
let field_pats =
try!(fields.iter()
.map(|field| Ok(codemap::Spanned {
span: codemap::DUMMY_SP,
node: hir::FieldPat {
name: field.name.node,
pat: try!(const_expr_to_pat(tcx, &field.expr,
pat_id, span)),
is_shorthand: false,
},
}))
.collect());
PatKind::Struct(path.clone(), field_pats, false)
}

hir::ExprVec(ref exprs) => {
let pats = exprs.iter().map(|expr| const_expr_to_pat(tcx, &expr, span)).collect();
let pats = try!(exprs.iter()
.map(|expr| const_expr_to_pat(tcx, &expr,
pat_id, span))
.collect());
PatKind::Vec(pats, None, hir::HirVec::new())
}

Expand All @@ -373,15 +415,15 @@ pub fn const_expr_to_pat(tcx: &TyCtxt, expr: &Expr, span: Span) -> P<hir::Pat> {
Some(Def::AssociatedConst(def_id)) => {
let substs = Some(tcx.node_id_item_substs(expr.id).substs);
let (expr, _ty) = lookup_const_by_id(tcx, def_id, substs).unwrap();
return const_expr_to_pat(tcx, expr, span);
return const_expr_to_pat(tcx, expr, pat_id, span);
},
_ => unreachable!(),
}
}

_ => PatKind::Lit(P(expr.clone()))
};
P(hir::Pat { id: expr.id, node: pat, span: span })
Ok(P(hir::Pat { id: expr.id, node: pat, span: span }))
}

pub fn eval_const_expr(tcx: &TyCtxt, e: &Expr) -> ConstVal {
Expand Down
8 changes: 7 additions & 1 deletion src/librustc/session/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,13 @@ impl Session {
let lint_id = lint::LintId::of(lint);
let mut lints = self.lints.borrow_mut();
match lints.get_mut(&id) {
Some(arr) => { arr.push((lint_id, sp, msg)); return; }
Some(arr) => {
let tuple = (lint_id, sp, msg);
if !arr.contains(&tuple) {
arr.push(tuple);
}
return;
}
None => {}
}
lints.insert(id, vec!((lint_id, sp, msg)));
Expand Down
8 changes: 8 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,14 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
id: LintId::of(OVERLAPPING_INHERENT_IMPLS),
reference: "issue #22889 <https://github.com/rust-lang/rust/issues/22889>",
},
FutureIncompatibleInfo {
id: LintId::of(ILLEGAL_FLOATING_POINT_CONSTANT_PATTERN),
reference: "RFC 1445 <https://github.com/rust-lang/rfcs/pull/1445>",
},
FutureIncompatibleInfo {
id: LintId::of(ILLEGAL_STRUCT_OR_ENUM_CONSTANT_PATTERN),
reference: "RFC 1445 <https://github.com/rust-lang/rfcs/pull/1445>",
},
]);

// We have one lint pass defined specially
Expand Down
13 changes: 10 additions & 3 deletions src/librustc_mir/hair/cx/pattern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,16 @@ impl<'patcx, 'cx, 'tcx> PatCx<'patcx, 'cx, 'tcx> {
let substs = Some(self.cx.tcx.node_id_item_substs(pat.id).substs);
match const_eval::lookup_const_by_id(self.cx.tcx, def_id, substs) {
Some((const_expr, _const_ty)) => {
let pat = const_eval::const_expr_to_pat(self.cx.tcx, const_expr,
pat.span);
return self.to_pattern(&pat);
match const_eval::const_expr_to_pat(self.cx.tcx,
const_expr,
pat.id,
pat.span) {
Ok(pat) =>
return self.to_pattern(&pat),
Err(_) =>
self.cx.tcx.sess.span_bug(
pat.span, "illegal constant"),
}
}
None => {
self.cx.tcx.sess.span_bug(
Expand Down
7 changes: 4 additions & 3 deletions src/libstd/num/f32.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1152,9 +1152,10 @@ impl f32 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn asinh(self) -> f32 {
match self {
NEG_INFINITY => NEG_INFINITY,
x => (x + ((x * x) + 1.0).sqrt()).ln(),
if self == NEG_INFINITY {
NEG_INFINITY
} else {
(self + ((self * self) + 1.0).sqrt()).ln()
}
}

Expand Down
7 changes: 4 additions & 3 deletions src/libstd/num/f64.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1023,9 +1023,10 @@ impl f64 {
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub fn asinh(self) -> f64 {
match self {
NEG_INFINITY => NEG_INFINITY,
x => (x + ((x * x) + 1.0).sqrt()).ln(),
if self == NEG_INFINITY {
NEG_INFINITY
} else {
(self + ((self * self) + 1.0).sqrt()).ln()
}
}

Expand Down
25 changes: 25 additions & 0 deletions src/libsyntax/codemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1304,6 +1304,31 @@ impl CodeMap {
return a;
}

/// Check if the backtrace `subtrace` contains `suptrace` as a prefix.
pub fn more_specific_trace(&self,
mut subtrace: ExpnId,
suptrace: ExpnId)
-> bool {
loop {
if subtrace == suptrace {
return true;
}

let stop = self.with_expn_info(subtrace, |opt_expn_info| {
if let Some(expn_info) = opt_expn_info {
subtrace = expn_info.call_site.expn_id;
false
} else {
true
}
});

if stop {
return false;
}
}
}

pub fn record_expansion(&self, expn_info: ExpnInfo) -> ExpnId {
let mut expansions = self.expansions.borrow_mut();
expansions.push(expn_info);
Expand Down
42 changes: 36 additions & 6 deletions src/libsyntax/ext/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use visit::Visitor;
use std_inject;

use std::collections::HashSet;

use std::env;

pub fn expand_expr(e: P<ast::Expr>, fld: &mut MacroExpander) -> P<ast::Expr> {
let expr_span = e.span;
Expand Down Expand Up @@ -1275,11 +1275,41 @@ impl<'a, 'b> MacroExpander<'a, 'b> {
}

fn new_span(cx: &ExtCtxt, sp: Span) -> Span {
/* this discards information in the case of macro-defining macros */
Span {
lo: sp.lo,
hi: sp.hi,
expn_id: cx.backtrace(),
debug!("new_span(sp={:?})", sp);

if cx.codemap().more_specific_trace(sp.expn_id, cx.backtrace()) {
// If the span we are looking at has a backtrace that has more
// detail than our current backtrace, then we keep that
// backtrace. Honestly, I have no idea if this makes sense,
// because I have no idea why we are stripping the backtrace
// below. But the reason I made this change is because, in
// deriving, we were generating attributes with a specific
// backtrace, which was essential for `#[structural_match]` to
// be properly supported, but these backtraces were being
// stripped and replaced with a null backtrace. Sort of
// unclear why this is the case. --nmatsakis
debug!("new_span: keeping trace from {:?} because it is more specific",
sp.expn_id);
sp
} else {
// This discards information in the case of macro-defining macros.
//
// The comment above was originally added in
// b7ec2488ff2f29681fe28691d20fd2c260a9e454 in Feb 2012. I
// *THINK* the reason we are doing this is because we want to
// replace the backtrace of the macro contents with the
// backtrace that contains the macro use. But it's pretty
// unclear to me. --nmatsakis
let sp1 = Span {
lo: sp.lo,
hi: sp.hi,
expn_id: cx.backtrace(),
};
debug!("new_span({:?}) = {:?}", sp, sp1);
if sp.expn_id.into_u32() == 0 && env::var_os("NDM").is_some() {
panic!("NDM");
}
sp1
}
}

Expand Down
9 changes: 8 additions & 1 deletion src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ const KNOWN_FEATURES: &'static [(&'static str, &'static str, Option<u32>, Status
// to bootstrap fix for #5723.
("issue_5723_bootstrap", "1.0.0", None, Accepted),

("structural_match", "1.8.0", Some(31434), Active),

// A way to temporarily opt out of opt in copy. This will *never* be accepted.
("opt_out_copy", "1.0.0", None, Removed),

Expand Down Expand Up @@ -304,6 +306,11 @@ pub const KNOWN_ATTRIBUTES: &'static [(&'static str, AttributeType, AttributeGat
("link_args", Normal, Ungated),
("macro_escape", Normal, Ungated),

// RFC #1445.
("structural_match", Whitelisted, Gated("structural_match",
"the semantics of constant patterns is \
not yet settled")),

// Not used any more, but we can't feature gate it
("no_stack_check", Normal, Ungated),

Expand Down Expand Up @@ -682,7 +689,7 @@ impl<'a> Context<'a> {
fn gate_feature(&self, feature: &str, span: Span, explain: &str) {
let has_feature = self.has_feature(feature);
debug!("gate_feature(feature = {:?}, span = {:?}); has? {}", feature, span, has_feature);
if !has_feature {
if !has_feature && !self.cm.span_allows_unstable(span) {
emit_feature_err(self.span_handler, feature, span, GateIssue::Language, explain);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/libsyntax_ext/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ crate-type = ["dylib"]

[dependencies]
fmt_macros = { path = "../libfmt_macros" }
log = { path = "../liblog" }
syntax = { path = "../libsyntax" }
Loading

0 comments on commit 128b2ad

Please sign in to comment.