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
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
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
16 changes: 16 additions & 0 deletions src/test/ui/dyn-keyword/dyn-2018-edition-lint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// edition:2018
#[deny(bare_trait_objects)]

fn function(x: &SomeTrait, y: Box<SomeTrait>) {
//~^ ERROR trait objects without an explicit `dyn` are deprecated
//~| WARN this was previously accepted
//~| ERROR trait objects without an explicit `dyn` are deprecated
//~| WARN this was previously accepted
let _x: &SomeTrait = todo!();
//~^ ERROR trait objects without an explicit `dyn` are deprecated
//~| WARN this was previously accepted
}

trait SomeTrait {}

fn main() {}
34 changes: 34 additions & 0 deletions src/test/ui/dyn-keyword/dyn-2018-edition-lint.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
error: trait objects without an explicit `dyn` are deprecated
--> $DIR/dyn-2018-edition-lint.rs:4:17
|
LL | fn function(x: &SomeTrait, y: Box<SomeTrait>) {
| ^^^^^^^^^ help: use `dyn`: `dyn SomeTrait`
|
note: the lint level is defined here
--> $DIR/dyn-2018-edition-lint.rs:2:8
|
LL | #[deny(bare_trait_objects)]
| ^^^^^^^^^^^^^^^^^^
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
= note: for more information, see issue #80165 <https://github.com/rust-lang/rust/issues/80165>

error: trait objects without an explicit `dyn` are deprecated
--> $DIR/dyn-2018-edition-lint.rs:4:35
|
LL | fn function(x: &SomeTrait, y: Box<SomeTrait>) {
| ^^^^^^^^^ help: use `dyn`: `dyn SomeTrait`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
= note: for more information, see issue #80165 <https://github.com/rust-lang/rust/issues/80165>

error: trait objects without an explicit `dyn` are deprecated
--> $DIR/dyn-2018-edition-lint.rs:9:14
|
LL | let _x: &SomeTrait = todo!();
| ^^^^^^^^^ help: use `dyn`: `dyn SomeTrait`
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in the 2021 edition!
= note: for more information, see issue #80165 <https://github.com/rust-lang/rust/issues/80165>

error: aborting due to 3 previous errors

12 changes: 12 additions & 0 deletions src/test/ui/dyn-keyword/dyn-2021-edition-error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// edition:2021
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests for this for edition 2018? Just want to make sure we test for the applicable suggestions not to break. Also, I don't think we've had a discussion around test coverage for people updating from editions earlier than CURRENT - 1. Should we be having that conversation?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that that's simply not supported, you have to upgrade editions one by one.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, we are currently testing Rust2018 though virtue of the compiler being on Rust 2018 itself. However, once it moves to Rust 2021 (no idea when that will happen), the tests that currently only assume the current edition (like this one) will need to be updated.

We could explicitly make a test right now, making sure that the lint triggers in 2018 which will be a bit of a duplicate test but will ensure we still test this code path when the compiler is updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in favor of making an explicit test for code written in Rust 2018 as part of this PR. We want to check that it is still doing the expecting thing on rust 2018 code.


fn function(x: &SomeTrait, y: Box<SomeTrait>) {
//~^ ERROR trait objects must include the `dyn` keyword
//~| ERROR trait objects must include the `dyn` keyword
let _x: &SomeTrait = todo!();
//~^ ERROR trait objects must include the `dyn` keyword
}

trait SomeTrait {}

fn main() {}
36 changes: 36 additions & 0 deletions src/test/ui/dyn-keyword/dyn-2021-edition-error.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
error[E0782]: trait objects must include the `dyn` keyword
--> $DIR/dyn-2021-edition-error.rs:6:14
|
LL | let _x: &SomeTrait = todo!();
| ^^^^^^^^^
|
help: add `dyn` keyword before this trait
|
LL | let _x: &dyn SomeTrait = todo!();
| ^^^

error[E0782]: trait objects must include the `dyn` keyword
--> $DIR/dyn-2021-edition-error.rs:3:17
|
LL | fn function(x: &SomeTrait, y: Box<SomeTrait>) {
| ^^^^^^^^^
|
help: add `dyn` keyword before this trait
|
LL | fn function(x: &dyn SomeTrait, y: Box<SomeTrait>) {
| ^^^

error[E0782]: trait objects must include the `dyn` keyword
--> $DIR/dyn-2021-edition-error.rs:3:35
|
LL | fn function(x: &SomeTrait, y: Box<SomeTrait>) {
| ^^^^^^^^^
|
help: add `dyn` keyword before this trait
|
LL | fn function(x: &SomeTrait, y: Box<dyn SomeTrait>) {
| ^^^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0782`.
Loading