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

Update BARE_TRAIT_OBJECT and ELLIPSIS_INCLUSIVE_RANGE_PATTERNS to errors in Rust 2021 #83213

Merged
merged 14 commits into from
May 4, 2021
Merged
30 changes: 22 additions & 8 deletions compiler/rustc_ast_lowering/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ use rustc_ast_pretty::pprust;
use rustc_data_structures::captures::Captures;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::sync::Lrc;
use rustc_errors::struct_span_err;
use rustc_errors::{struct_span_err, Applicability};
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Namespace, PartialRes, PerNS, Res};
use rustc_hir::def_id::{DefId, DefIdMap, LocalDefId, CRATE_DEF_ID};
Expand All @@ -58,6 +58,7 @@ use rustc_session::lint::builtin::{BARE_TRAIT_OBJECTS, MISSING_ABI};
use rustc_session::lint::{BuiltinLintDiagnostics, LintBuffer};
use rustc_session::parse::ParseSess;
use rustc_session::Session;
use rustc_span::edition::Edition;
use rustc_span::hygiene::ExpnId;
use rustc_span::source_map::{respan, DesugaringKind};
use rustc_span::symbol::{kw, sym, Ident, Symbol};
Expand Down Expand Up @@ -2774,13 +2775,26 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
.map(|snippet| snippet.starts_with("#["))
.unwrap_or(true);
if !is_macro_callsite {
self.resolver.lint_buffer().buffer_lint_with_diagnostic(
BARE_TRAIT_OBJECTS,
id,
span,
"trait objects without an explicit `dyn` are deprecated",
BuiltinLintDiagnostics::BareTraitObject(span, is_global),
)
if span.edition() < Edition::Edition2021 {
self.resolver.lint_buffer().buffer_lint_with_diagnostic(
BARE_TRAIT_OBJECTS,
id,
span,
"trait objects without an explicit `dyn` are deprecated",
BuiltinLintDiagnostics::BareTraitObject(span, is_global),
)
} else {
let msg = "trait objects must include the `dyn` keyword";
let label = "add `dyn` keyword before this trait";
let mut err = struct_span_err!(self.sess, span, E0782, "{}", msg,);
err.span_suggestion_verbose(
span.shrink_to_lo(),
label,
String::from("dyn "),
Applicability::MachineApplicable,
);
err.emit();
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_error_codes/src/error_codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,8 @@ E0778: include_str!("./error_codes/E0778.md"),
E0779: include_str!("./error_codes/E0779.md"),
E0780: include_str!("./error_codes/E0780.md"),
E0781: include_str!("./error_codes/E0781.md"),
E0782: include_str!("./error_codes/E0782.md"),
E0783: include_str!("./error_codes/E0783.md"),
;
// E0006, // merged with E0005
// E0008, // cannot bind by-move into a pattern guard
Expand Down
26 changes: 26 additions & 0 deletions compiler/rustc_error_codes/src/error_codes/E0782.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
Trait objects must include the `dyn` keyword.
rylev marked this conversation as resolved.
Show resolved Hide resolved

Erroneous code example:

```edition2021,compile_fail,E0782
trait Foo {}
fn test(arg: Box<Foo>) {} // error!
```

Trait objects are a way to call methods on types that are not known until
runtime but conform to some trait.

Trait objects should be formed with `Box<dyn Foo>`, but in the code above
`dyn` is left off.

This makes it harder to see that `arg` is a trait object and not a
simply a heap allocated type called `Foo`.

To fix this issue, add `dyn` before the trait name.

```edition2021
trait Foo {}
Copy link
Member

Choose a reason for hiding this comment

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

I think you still need to specify the edition here (as least to be coherent with the failing code example).

fn test(arg: Box<dyn Foo>) {} // ok!
```

This used to be allowed before edition 2021, but is now an error.
22 changes: 22 additions & 0 deletions compiler/rustc_error_codes/src/error_codes/E0783.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
The range pattern `...` is no longer allowed.
rylev marked this conversation as resolved.
Show resolved Hide resolved

Erroneous code example:

```edition2021,compile_fail,E0783
match 2u8 {
0...9 => println!("Got a number less than 10"), // error!
_ => println!("Got a number 10 or more"),
}
```

Older Rust code using previous editions allowed `...` to stand for exclusive
ranges which are now signified using `..=`.

To make this code compile replace the `...` with `..=`.

```edition2021
match 2u8 {
0..=9 => println!("Got a number less than 10"), // ok!
_ => println!("Got a number 10 or more"),
}
```
81 changes: 55 additions & 26 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1655,7 +1655,11 @@ declare_lint! {
/// [`..` range expression]: https://doc.rust-lang.org/reference/expressions/range-expr.html
pub ELLIPSIS_INCLUSIVE_RANGE_PATTERNS,
Warn,
"`...` range patterns are deprecated"
"`...` range patterns are deprecated",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #80165 <https://github.com/rust-lang/rust/issues/80165>",
edition: Some(Edition::Edition2021),
};
}

#[derive(Default)]
Expand Down Expand Up @@ -1699,32 +1703,57 @@ impl EarlyLintPass for EllipsisInclusiveRangePatterns {
let suggestion = "use `..=` for an inclusive range";
if parenthesise {
self.node_id = Some(pat.id);
cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, pat.span, |lint| {
let end = expr_to_string(&end);
let replace = match start {
Some(start) => format!("&({}..={})", expr_to_string(&start), end),
None => format!("&(..={})", end),
};
lint.build(msg)
.span_suggestion(
pat.span,
suggestion,
replace,
Applicability::MachineApplicable,
)
.emit();
});
let end = expr_to_string(&end);
let replace = match start {
Some(start) => format!("&({}..={})", expr_to_string(&start), end),
None => format!("&(..={})", end),
};
if join.edition() >= Edition::Edition2021 {
let mut err =
rustc_errors::struct_span_err!(cx.sess, pat.span, E0783, "{}", msg,);
err.span_suggestion(
pat.span,
suggestion,
replace,
Applicability::MachineApplicable,
)
.emit();
} else {
cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, pat.span, |lint| {
lint.build(msg)
.span_suggestion(
pat.span,
suggestion,
replace,
Applicability::MachineApplicable,
)
.emit();
});
}
} else {
cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, join, |lint| {
lint.build(msg)
.span_suggestion_short(
join,
suggestion,
"..=".to_owned(),
Applicability::MachineApplicable,
)
.emit();
});
let replace = "..=".to_owned();
if join.edition() >= Edition::Edition2021 {
let mut err =
rustc_errors::struct_span_err!(cx.sess, pat.span, E0783, "{}", msg,);
Copy link
Member

Choose a reason for hiding this comment

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

Note: lint passes typically do not have side effects, if a lint is disabled there should ideally be no behavior differences between running the pass or not (and that's a potential optimization to do in the future).

I'm not sure how strictly the compiler team follows this; it's possible that when we do such optimizations we'll have to go back and audit our lints (which is fine!), but I just want to flag this.
I'm not sure how str

Copy link
Contributor

Choose a reason for hiding this comment

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

The only concern is that performing this check in the appropriate place as if we had no editions could cause divergence in the behavior. Emitting the error in the same place as we currently emit the lint should make the transition less painful than if we start denying in places where we currently don't even lint. That being said, this could be accepted as acceptable breakage to bestow on the ecosystem in the name of codebase cleanliness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I missed this discussion the first time around. A few thoughts:

  • Maybe we want some way to have the lint pass indicate that it is required? I'm imagining adding a fn required(&self) -> bool { false } to the trait, and this lint could return true. This might be convenient if we commonly wind up with things that are errors in one edition and lints in another.
  • Alternatively, we could maybe add a group of lints, not 'late lints' but 'required late lints' and put the lint into that group, something like that.

Moving the logic out from the "lint helper" just seems like a bit of extra work, I'm not sure what would be a nice place to put it, we'd have to reproduce the "traverse the tree" code. That said, I don't object to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@estebank I don't quite understand how behavior would change -- do you just mean altering the order of errors that get emitted?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not quite following. That said, I don't think we hold ourselves to a sufficiently high standard here today that I would be comfortable applying a "no pass" optimization if lints were disabled. We also merge all built-in lint passes into a few groups (by when they need to run), so there's not really an actual "per-lint" full traversal of any tree, so it's not obvious a major win is to be had. (But I don't think we've profiled much).

I think this seems OK for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as we have tests for the hard error, I'm comfortable with it.

err.span_suggestion_short(
join,
suggestion,
replace,
Applicability::MachineApplicable,
)
.emit();
} else {
cx.struct_span_lint(ELLIPSIS_INCLUSIVE_RANGE_PATTERNS, join, |lint| {
lint.build(msg)
.span_suggestion_short(
join,
suggestion,
replace,
Applicability::MachineApplicable,
)
.emit();
});
}
};
}
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1618,7 +1618,11 @@ declare_lint! {
/// [`impl Trait`]: https://doc.rust-lang.org/book/ch10-02-traits.html#traits-as-parameters
pub BARE_TRAIT_OBJECTS,
Warn,
"suggest using `dyn Trait` for trait objects"
"suggest using `dyn Trait` for trait objects",
@future_incompatible = FutureIncompatibleInfo {
reference: "issue #80165 <https://github.com/rust-lang/rust/issues/80165>",
edition: Some(Edition::Edition2021),
};
}

declare_lint! {
Expand Down
12 changes: 7 additions & 5 deletions compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,11 +272,13 @@ pub fn struct_lint_level<'s, 'd>(
// emit shouldn't be automatically fixed by rustfix.
err.allow_suggestions(false);

// If this is a future incompatible lint it'll become a hard error, so
// we have to emit *something*. Also, if this lint occurs in the
// expansion of a macro from an external crate, allow individual lints
// to opt-out from being reported.
if future_incompatible.is_none() && !lint.report_in_external_macro {
// If this is a future incompatible that is not an edition fixing lint
// it'll become a hard error, so we have to emit *something*. Also,
// if this lint occurs in the expansion of a macro from an external crate,
// allow individual lints to opt-out from being reported.
let not_future_incompatible =
Mark-Simulacrum marked this conversation as resolved.
Show resolved Hide resolved
future_incompatible.map(|f| f.edition.is_some()).unwrap_or(true);
if not_future_incompatible && !lint.report_in_external_macro {
err.cancel();
// Don't continue further, since we don't want to have
// `diag_span_note_once` called for a diagnostic that isn't emitted.
Expand Down
51 changes: 37 additions & 14 deletions compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use rustc_middle::ty::{
use rustc_session::lint;
use rustc_session::lint::builtin::BARE_TRAIT_OBJECTS;
use rustc_session::parse::feature_err;
use rustc_span::edition::Edition;
use rustc_span::source_map::{original_sp, DUMMY_SP};
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::{self, BytePos, MultiSpan, Span};
Expand Down Expand Up @@ -969,20 +970,42 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let TyKind::TraitObject([poly_trait_ref, ..], _, TraitObjectSyntax::None) =
self_ty.kind
{
self.tcx.struct_span_lint_hir(BARE_TRAIT_OBJECTS, hir_id, self_ty.span, |lint| {
let mut db = lint
.build(&format!("trait objects without an explicit `dyn` are deprecated"));
let (sugg, app) = match self.tcx.sess.source_map().span_to_snippet(self_ty.span)
{
Ok(s) if poly_trait_ref.trait_ref.path.is_global() => {
(format!("<dyn ({})>", s), Applicability::MachineApplicable)
}
Ok(s) => (format!("<dyn {}>", s), Applicability::MachineApplicable),
Err(_) => ("<dyn <type>>".to_string(), Applicability::HasPlaceholders),
};
db.span_suggestion(self_ty.span, "use `dyn`", sugg, app);
db.emit()
});
let msg = "trait objects without an explicit `dyn` are deprecated";
let (sugg, app) = match self.tcx.sess.source_map().span_to_snippet(self_ty.span) {
Ok(s) if poly_trait_ref.trait_ref.path.is_global() => {
(format!("<dyn ({})>", s), Applicability::MachineApplicable)
}
Ok(s) => (format!("<dyn {}>", s), Applicability::MachineApplicable),
Err(_) => ("<dyn <type>>".to_string(), Applicability::HasPlaceholders),
};
let replace = String::from("use `dyn`");
if self.sess().edition() >= Edition::Edition2021 {
let mut err = rustc_errors::struct_span_err!(
Copy link
Contributor

Choose a reason for hiding this comment

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

I do wish we could make this more unified. I feel like the struct_span_lint_hir ought to check the edition info and convert it to a hard error automatically. Probably better to just open an issue for this and leave it for future cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #84625

self.sess(),
self_ty.span,
E0783,
"{}",
msg,
);
err.span_suggestion(
self_ty.span,
&sugg,
replace,
Applicability::MachineApplicable,
)
.emit();
} else {
self.tcx.struct_span_lint_hir(
BARE_TRAIT_OBJECTS,
hir_id,
self_ty.span,
|lint| {
let mut db = lint.build(msg);
db.span_suggestion(self_ty.span, &replace, sugg, app);
db.emit()
},
);
}
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1687,6 +1687,9 @@ fn markdown_test(builder: &Builder<'_>, compiler: Compiler, markdown: &Path) ->
builder.info(&format!("doc tests for: {}", markdown.display()));
let mut cmd = builder.rustdoc_cmd(compiler);
builder.add_rust_test_threads(&mut cmd);
// allow for unstable options such as new editions
cmd.arg("-Z");
cmd.arg("unstable-options");
cmd.arg("--test");
cmd.arg(markdown);
cmd.env("RUSTC_BOOTSTRAP", "1");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ fn b() {
//~| ERROR expected trait, found constant `BAR`
//~| ERROR type provided when a constant was expected
//~| WARN trait objects without an explicit `dyn` are deprecated
//~| WARN this was previously accepted by the compiler
}
fn c() {
foo::<3 + 3>(); //~ ERROR expressions must be enclosed in braces
Expand Down
Loading