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

as_slice will be stable in 1.7 #728

Closed
mcarton opened this issue Feb 29, 2016 · 10 comments · Fixed by #795
Closed

as_slice will be stable in 1.7 #728

mcarton opened this issue Feb 29, 2016 · 10 comments · Fixed by #795

Comments

@mcarton
Copy link
Member

mcarton commented Feb 29, 2016

We need to decide what to do with the unstable_as_slice and unstable_as_mut_slice lints as these functions will be stable in 1.7 (ref. rust-lang/rust#30943). We could simply remove them or keep them and change their documentation to explain it’s better to use &v[..] (see rust-lang/rust#27729 for some arguments).
I think we can remove them as those functions are not inherently bad and can sometimes lead to slightly more readable code (.map(String::as_slice) vs. .map(|s| &*s) and y.happy().monkey().as_slice() vs. &(y.happy().monkey())[..]).

@fhartwig
Copy link
Contributor

fhartwig commented Mar 2, 2016

I agree that the lint should be removed. Warning because "using deref magic instead is nicer" is far too opinionated in my opinion.

@llogiq
Copy link
Contributor

llogiq commented Mar 2, 2016

Hmmm...I think we need to have a story for deprecating lints. I think the lints could still be useful for people that intend (for whatever reason) to deploy on an older Rust version, so I think we shouldn't throw them away yet.

@mcarton
Copy link
Member Author

mcarton commented Mar 2, 2016

In any case the documentation must be changed.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2016

I think the lints could still be useful for people that intend (for whatever reason) to deploy on an older Rust version, so I think we shouldn't throw them away yet.

If they were using the old nightly, then it's very likely that clippy won't work with it anymore. Even if they use a new nightly for clippy and an old one for their normal compilation, there are probably breaking changes between those nightlies, so the new nightly won't compile the code meant for the old one. Thus they are locked-in to an old nightly and a clippy that ran on that nightly.

@mcarton
Copy link
Member Author

mcarton commented Mar 3, 2016

By “older Rust version” he does not mean “older nightly version”, he means people compiling both without Clippy on Rust stable and with Clippy on the last nightly. For them keeping the lint might makes sense as a warning for when they use stable.
But the lint here warns about a feature that we now know will be stabilized (and is not any different in implementation in older version), so it does not make any harm to use it even on older Rust version. For me the lint was more a ”Hey, you’re using a feature which can trivially be replaced by something else and might disappear in 3 month” warning.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 3, 2016

On stable they can't use the feature...

@mcarton
Copy link
Member Author

mcarton commented Mar 3, 2016

Oh right, for some reason I thought that kind of feature could be explicitly allowed even on stable or beta 😳. You have a point then.

@llogiq
Copy link
Contributor

llogiq commented Mar 3, 2016

Perhaps we could ask on rust-users and r/rust which Rust versions people are using? If most people are on current rust anyway, we may as well remove the lint.

@Manishearth
Copy link
Member

I think to deprecate a lint, we should:

  • Immediately move it to a clippy_deprecated group and make it Allow
  • after time has passed, add a check_attribute pass that mentions that certain lints are unnecessary.
  • after time has passed, keep the lint registered, but with a dummy lintpass (to avoid unknown_lints errors)
  • remove it completely.

Perhaps we should first get the framework for more than 2 lint groups up? The idea is simple, have a custom macro instead of declare_lint, which can take the name of the lint group in as well. Make the python script work with both declare_lint and the new macro.

@mcarton
Copy link
Member Author

mcarton commented Mar 3, 2016

Perhaps we should first get the framework for more than 2 lint groups up? The idea is simple, have a custom macro instead of declare_lint, which can take the name of the lint group in as well. Make the python script work with both declare_lint and the new macro.

👍 to that. Sometimes I’d also like an ”API breaking lints”-group that I could allow because I’m using clippy on a project that does not regularly use it and I don’t want to mess with their interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants