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

Add functionality for epoch lints; add epoch lint for dyn-trait #48461

Merged
merged 8 commits into from
Mar 1, 2018

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth commented Feb 23, 2018

This makes it possible for lints to specify an epoch after which they become Deny, as well as automatically creating lint groups for these.

This had to be done as a hardwired lint because we lose information about whether TyTraitObject nodes were from dyn trait in the HIR, and in the AST we don't know if TyPaths are type-y or trait-y.

Still need to write tests and fix up all the new warnings (though I may wait for contributors to do that -- @killercup is it possible to run rustfix on a crate-by-crate basis for rustc?). Also, #48457 needs to get some contents.

Still, worth getting review for what I have so far.

r? @nmatsakis

fixes #48447

FutureIncompatibleInfo {
id: LintId::of(lint::builtin::BARE_TRAIT_OBJECT),
reference: "issue #48457 <https://github.com/rust-lang/rust/issues/48457>",
epoch: Some(session::config::Epoch::Epoch2018),
Copy link
Member Author

Choose a reason for hiding this comment

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

we could potentially reuse the epoch on the lint

we do need something for lints which need to be removed on the next epoch (because the stuff they lint doesn't exist anymore)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to imagine when we would want to make a distinction between the "epoch on the lint" and the "epoch in the future compat info". I'm not really sure -- indeed -- what the "epoch on the lint" means, if not that it is an epoch lint of this kind. Maybe another way of saying this is that "future incompatibility" feels distinct from epochs, and might want to be a different mechanism. In particular, future incompatible changes change for everyone, whereas epochs are opt-in.

Copy link
Member Author

Choose a reason for hiding this comment

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

The lint may become Deny on a different epoch than the one it is linting for, basically.

One of the settings is "when this becomes Deny", the other is "which lint group this belongs to".

In the future we may also provide a way for the lint to go away completely.

@Manishearth
Copy link
Member Author

We also need a lint for the dyn keyword, yes? I can't write this one until we have raw identifiers.

@@ -1,4 +1,4 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// Copyright 2017 The Rust Project ~velopers. See the COPYRIGHT
Copy link
Contributor

Choose a reason for hiding this comment

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

accidental replace

}

pub fn buffer_lint_with_diagnostic<S: Into<MultiSpan>>(&self,
lint: &'static lint::Lint,
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is weird here. Maybe use rustfmt style?

@@ -74,25 +75,46 @@ pub struct Lint {
///
/// e.g. "imports that are never used"
pub desc: &'static str,

/// Deny lint after this epoch
pub epoch_deny: Option<config::Epoch>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Further down you state that the lint should be removed after the epoch, here it seems to suggest it should be deny.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is this? It used to say this, but that changed.

This specific lint becomes Deny in Rust2018. Other lints may wish to disappear entirely because they are no longer lints.

@Manishearth
Copy link
Member Author

Manishearth commented Feb 23, 2018

A bug is that it will currently suggest &dyn ::Foo for &::Foo, however dyn::Foo parses as a path. We could suggest &dyn (::Foo) (at least on the 2015 epoch) but this doesn't currently work either.

@Manishearth
Copy link
Member Author

Manishearth commented Feb 23, 2018

I've got the rustfix stuff running at #48477

@Manishearth
Copy link
Member Author

#48481 fixes the dyn (Trait) issue

@Manishearth Manishearth force-pushed the epoch-dyn-trait branch 4 times, most recently from 5484cf9 to 1c99be5 Compare February 23, 2018 21:56
@nikomatsakis nikomatsakis self-assigned this Feb 23, 2018
@pietroalbini pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This all looks quite good. I'm wondering about the idea of distinguish epochal vs non-epochal future incompatibility. Thoughts?

r=me modulo nits and this question though.

for info in lints {
self.future_incompatible.insert(info.id, info);

for epoch in config::ALL_EPOCHS {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment plz


"Create a lint group for each epoch corresponding to the future compatibility warnings targeting that epoch"


(Do you think that this ought to include the lints for "all previous epochs", since they are additive?)

(Also, are these lint groups insta-stable?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We could. I think having them separate is cleaner, Rust doesn't do well with overlapping lint groups.

Rust lint stability is a very low bar, you just need to not remove the lint (aside from register_removed_lint() or whatever it's called). I think it's fine to insta-stable lint groups.

FutureIncompatibleInfo {
id: LintId::of(lint::builtin::BARE_TRAIT_OBJECT),
reference: "issue #48457 <https://github.com/rust-lang/rust/issues/48457>",
epoch: Some(session::config::Epoch::Epoch2018),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to imagine when we would want to make a distinction between the "epoch on the lint" and the "epoch in the future compat info". I'm not really sure -- indeed -- what the "epoch on the lint" means, if not that it is an epoch lint of this kind. Maybe another way of saying this is that "future incompatibility" feels distinct from epochs, and might want to be a different mechanism. In particular, future incompatible changes change for everyone, whereas epochs are opt-in.

Err(_) => format!("dyn <type>")
BuiltinLintDiagnostics::BareTraitObject(span, is_global) => {
let sugg = match (sess.codemap().span_to_snippet(span), is_global) {
(Ok(s), true) => format!("dyn ({})", s),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would (imo) be slightly easier to read as

let sugg = match sess.codemap().span_to_snippet(span) {
    Ok(s) if is_global => ...
    Ok(s) => ...
    _
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We do that, perhaps you're reviewing an earlier commit?

Ok(s) => format!("dyn {}", s),
Err(_) => format!("dyn <type>")
};
db.span_suggestion(span, "use `dyn`", sugg);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have a test or two, particularly of the suggestions?

@Manishearth Manishearth force-pushed the epoch-dyn-trait branch 6 times, most recently from 6ac8093 to a4ebd4a Compare February 28, 2018 03:08
@Manishearth
Copy link
Member Author

@bors r=nmatsakis

(r+d over IRC)

@bors
Copy link
Contributor

bors commented Feb 28, 2018

📌 Commit 0cb3672 has been approved by nmatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2018
Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 28, 2018
bors added a commit that referenced this pull request Feb 28, 2018
Rollup of 10 pull requests

- Successful merges: #48355, #48359, #48380, #48419, #48420, #48461, #48522, #48570, #48572, #48603
- Failed merges:
@bors bors merged commit 0cb3672 into rust-lang:master Mar 1, 2018
@steveklabnik
Copy link
Member

(r+d over IRC)

@Manishearth any chance you could link the point in the log? It sounds like you two discussed some things, the justification would be nice to have saved in-context in the PR. :)

@Manishearth
Copy link
Member Author

@Manishearth Manishearth deleted the epoch-dyn-trait branch March 1, 2018 16:01
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 3, 2018
…sakis

Use `dyn trait` everywhere

Based on rust-lang#48461

do not land unless both are reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add epoch lint for dyn trait
6 participants