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

Clippy 1.0 #1358

Open
Manishearth opened this Issue Nov 23, 2016 · 15 comments

Comments

Projects
None yet
10 participants
@Manishearth
Collaborator

Manishearth commented Nov 23, 2016

So I'm thinking of doing a clippy 1.0 soon.

The idea will be to make a users.rlo post asking for feedback that needs to get in before 1.0, after which the semantics of existing lints will be somewhat frozen (can be changed, but needs more discussion). IMO warnings don't qualify as breaking changes anyway, so this isn't a rigid restriction.

However, what I'd like to get figured out before we land:

  • Are there any bikesheds that need to be done? (#533)
  • Are there any major lints that we must have before 1.0?
  • Are there any major bugs that we must fix?

The idea of 1.0 in my mind is that clippy is "ready". Folks have been using it for more than a year now, and this is something we probably should have done ages ago. This just marks it as something we're confident in telling others to use extensively.

This bug is mostly to track progress and to decide if there are any objections to going 1.0. The points listed above will be discussed when I make the internals posts, but can be discussed here if major.

Once we release 1.0 my plan is to try pushing for it to be included as a rustup component so that it can be used on stable.

Thoughts?

cc @llogiq @mcarton @oli-obk @birkenfeld

@mcarton

This comment has been minimized.

Show comment
Hide comment
@mcarton

mcarton Nov 23, 2016

Collaborator

I have been asked Sunday why/how the hell we got to 0.0.100 😄

Once we release 1.0 my plan is to try pushing for it to be included as a rustup component so that it can be used on stable.

So clippy 1.0.* would be for rustc 1.x, clippy 1.1.* for rustc 1.(x+1), etc.?

Are there any bikesheds that need to be done?

There probably is. I would like to merge/rename some lints before we get to stable. I looked at that once, but IIRC, the interface to rename lints is not available to us.

Are there any major lints that we must have before 1.0?

Don't think so.

Are there any major bugs that we must fix?

Most likely. I did want to rewrite loop lints at some points because they have quite a lot of FP, but I haven't had much time.

Collaborator

mcarton commented Nov 23, 2016

I have been asked Sunday why/how the hell we got to 0.0.100 😄

Once we release 1.0 my plan is to try pushing for it to be included as a rustup component so that it can be used on stable.

So clippy 1.0.* would be for rustc 1.x, clippy 1.1.* for rustc 1.(x+1), etc.?

Are there any bikesheds that need to be done?

There probably is. I would like to merge/rename some lints before we get to stable. I looked at that once, but IIRC, the interface to rename lints is not available to us.

Are there any major lints that we must have before 1.0?

Don't think so.

Are there any major bugs that we must fix?

Most likely. I did want to rewrite loop lints at some points because they have quite a lot of FP, but I haven't had much time.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Nov 24, 2016

Collaborator

So clippy 1.0.* would be for rustc 1.x, clippy 1.1.* for rustc 1.(x+1), etc.?

Nah, the stuff shipped with rustc will have arbitrary versions (may not even correspond to clippy published versions due to temporary rustup fixes). I haven't yet settled on how that will work, and this involves discussion with the rustup/rustc folks too.

Collaborator

Manishearth commented Nov 24, 2016

So clippy 1.0.* would be for rustc 1.x, clippy 1.1.* for rustc 1.(x+1), etc.?

Nah, the stuff shipped with rustc will have arbitrary versions (may not even correspond to clippy published versions due to temporary rustup fixes). I haven't yet settled on how that will work, and this involves discussion with the rustup/rustc folks too.

@oli-obk

This comment has been minimized.

Show comment
Hide comment
@oli-obk

oli-obk Nov 24, 2016

Collaborator

Regarding bikesheds, I say we move all controversial lints to allow by default (goodbye cyclomatic complexity my precious 😢) and the discuss them back into warn.

Also I think that we need #1295 and #1313 before 1.0, since those seem to be a major blocker for team wide usage.

Only cargo clippy is going into rustup, right? Or will the lints be autoloaded by the distributed rustc?

Collaborator

oli-obk commented Nov 24, 2016

Regarding bikesheds, I say we move all controversial lints to allow by default (goodbye cyclomatic complexity my precious 😢) and the discuss them back into warn.

Also I think that we need #1295 and #1313 before 1.0, since those seem to be a major blocker for team wide usage.

Only cargo clippy is going into rustup, right? Or will the lints be autoloaded by the distributed rustc?

@mcarton

This comment has been minimized.

Show comment
Hide comment
@mcarton

mcarton Nov 24, 2016

Collaborator

I have already started working on #1295, I'll try to finish that soonish.

Collaborator

mcarton commented Nov 24, 2016

I have already started working on #1295, I'll try to finish that soonish.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Nov 24, 2016

Collaborator

Only cargo clippy is going into rustup, right?

Oui.

I say we move all controversial lints to allow by default

Good idea. I'd prefer to just get them discussed first, and if there are any which haven't been discussed we move them to allow.

Collaborator

Manishearth commented Nov 24, 2016

Only cargo clippy is going into rustup, right?

Oui.

I say we move all controversial lints to allow by default

Good idea. I'd prefer to just get them discussed first, and if there are any which haven't been discussed we move them to allow.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Aug 1, 2017

Collaborator

FWIW I've added a C-1.0-blocking label for things (like lint renames) that must happen before 1.0. It is not an exhaustive list right now. Feel free to apply liberally.

Collaborator

Manishearth commented Aug 1, 2017

FWIW I've added a C-1.0-blocking label for things (like lint renames) that must happen before 1.0. It is not an exhaustive list right now. Feel free to apply liberally.

@LegNeato

This comment has been minimized.

Show comment
Hide comment
@LegNeato

LegNeato Sep 13, 2018

A bikeshed on Reddit suggested changing the name from clippy to lint.

Personally I superficially agree and also note that the clippy reference is not in the current zeitgeist and has never been experienced by devs of a certain age. The name and reference will only get less and less relevant as time goes on. Then again, clippy is a fun name for those who know, refers to computing lore/history, and I don't know how much work or fallout there would be changing 😃 .

Some projects using standard "lint" naming:

Some projects referring to "linting" but using different names:

LegNeato commented Sep 13, 2018

A bikeshed on Reddit suggested changing the name from clippy to lint.

Personally I superficially agree and also note that the clippy reference is not in the current zeitgeist and has never been experienced by devs of a certain age. The name and reference will only get less and less relevant as time goes on. Then again, clippy is a fun name for those who know, refers to computing lore/history, and I don't know how much work or fallout there would be changing 😃 .

Some projects using standard "lint" naming:

Some projects referring to "linting" but using different names:

@phansch

This comment has been minimized.

Show comment
Hide comment
@phansch

phansch Sep 13, 2018

Collaborator

I'm personally not opposed to renaming but one problem that comes to mind is: If we want to rename the command to cargo lint is that other Rust projects can also implement their own lints. There's also the lints in Rust itself.

I think calling Clippy by using cargo lint could be confusing in that regard as it's not clear anymore which lints of which project (clippy, rustc or something else) are executed.

Collaborator

phansch commented Sep 13, 2018

I'm personally not opposed to renaming but one problem that comes to mind is: If we want to rename the command to cargo lint is that other Rust projects can also implement their own lints. There's also the lints in Rust itself.

I think calling Clippy by using cargo lint could be confusing in that regard as it's not clear anymore which lints of which project (clippy, rustc or something else) are executed.

@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 13, 2018

Collaborator

Yeah, the point of Clippy is specifically to house the more annoying lints (of various levels of annoyingness). I've considered cargo lint in the past and it gets confusing.

I think it's fine if folks don't get the reference, it's still a nice, short, memorable tool name.

Collaborator

Manishearth commented Sep 13, 2018

Yeah, the point of Clippy is specifically to house the more annoying lints (of various levels of annoyingness). I've considered cargo lint in the past and it gets confusing.

I think it's fine if folks don't get the reference, it's still a nice, short, memorable tool name.

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 13, 2018

I think clippy is a great name for this project and wouldn't suggest changing it, but I'm somewhat concerned about the overall Rust + cargo interface implications of what's happened here.

I know this is implemented as a custom subcommand, but if we start shipping this component by default, we've functionally added a new (official) subcommand to cargo. Is that the intention for devtools in general? - any new devtool with a CLI will just be accessed by a cargo <project-name> interface? Where is the step where we think holistically about the interface our tools present to the user?

If we are going to eventually ship clippy by default, I'd be more comfortable with some sort of unified cargo lint interface, possibly with some way to configure sources of lints, of which clippy would be a default source. Or possibly there shouldn't be a subcommand at all once its shipped by default, and you should instead just say you want these lint passes run in addition as part of cargo build.

I don't care about the interface to the clippy-preview optional subcomponent, but once we start shipping components by default I think we need to revisit how they fit into the overall Rust toolchain experience: given that they will be on equal footing with Rust and cargo at that point, there is both a need for further revision and less constraints on what kind of interface you can have (i.e. at that point you can make PRs to cargo and rustc that are necessary to get the interface you want).

withoutboats commented Sep 13, 2018

I think clippy is a great name for this project and wouldn't suggest changing it, but I'm somewhat concerned about the overall Rust + cargo interface implications of what's happened here.

I know this is implemented as a custom subcommand, but if we start shipping this component by default, we've functionally added a new (official) subcommand to cargo. Is that the intention for devtools in general? - any new devtool with a CLI will just be accessed by a cargo <project-name> interface? Where is the step where we think holistically about the interface our tools present to the user?

If we are going to eventually ship clippy by default, I'd be more comfortable with some sort of unified cargo lint interface, possibly with some way to configure sources of lints, of which clippy would be a default source. Or possibly there shouldn't be a subcommand at all once its shipped by default, and you should instead just say you want these lint passes run in addition as part of cargo build.

I don't care about the interface to the clippy-preview optional subcomponent, but once we start shipping components by default I think we need to revisit how they fit into the overall Rust toolchain experience: given that they will be on equal footing with Rust and cargo at that point, there is both a need for further revision and less constraints on what kind of interface you can have (i.e. at that point you can make PRs to cargo and rustc that are necessary to get the interface you want).

@WiSaGaN

This comment has been minimized.

Show comment
Hide comment
@WiSaGaN

WiSaGaN Sep 14, 2018

Thanks @LegNeato for investigating conventions from other languages.

As mentioned in the reddit comment quoted below:

It feels a bit unfortunate though to have clippy as the name instead of lint. All previous official cargo commands use straightforward descriptive names: init, build, check, run, test, update, etc. It seems unnecessary to break this convention..

I think the question is whether it is necessary to break the established convention of official cargo subcommand naming by using "clippy".

Following the convention of using straightforward descriptive name also makes it easier for those coming from other languages (e.g. golang), or people not knowing the Microsoft insider jokes to grok what this command does.

WiSaGaN commented Sep 14, 2018

Thanks @LegNeato for investigating conventions from other languages.

As mentioned in the reddit comment quoted below:

It feels a bit unfortunate though to have clippy as the name instead of lint. All previous official cargo commands use straightforward descriptive names: init, build, check, run, test, update, etc. It seems unnecessary to break this convention..

I think the question is whether it is necessary to break the established convention of official cargo subcommand naming by using "clippy".

Following the convention of using straightforward descriptive name also makes it easier for those coming from other languages (e.g. golang), or people not knowing the Microsoft insider jokes to grok what this command does.

@fralalonde

This comment has been minimized.

Show comment
Hide comment
@fralalonde

fralalonde Sep 14, 2018

Following on the bikeshedding path of renaming clippy to something more descriptive of its purpose (and less nostalgic) while reserving lint for a future plugin mechanism (which I support), may I suggest

cargo coach

Coach gives instant feedback before going live (submitting a PR, releasing), and teaches you best practices as you learn Rust. As you get better you need less coaching but it's always there if you need a quick opinion. It's essentially constructive, and shouldn't (I believe) block you from doing whatever, contrarily to lints which could be prescriptive and fail the build.

It's also a subtle play on the cargo "transportation" theme. And it retains the cool consonance that cargo clippy had.

fralalonde commented Sep 14, 2018

Following on the bikeshedding path of renaming clippy to something more descriptive of its purpose (and less nostalgic) while reserving lint for a future plugin mechanism (which I support), may I suggest

cargo coach

Coach gives instant feedback before going live (submitting a PR, releasing), and teaches you best practices as you learn Rust. As you get better you need less coaching but it's always there if you need a quick opinion. It's essentially constructive, and shouldn't (I believe) block you from doing whatever, contrarily to lints which could be prescriptive and fail the build.

It's also a subtle play on the cargo "transportation" theme. And it retains the cool consonance that cargo clippy had.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Sep 14, 2018

Member

@fralalonde Let's not get into name bikeshedding, please. The question is whether the name needs to become more descriptive, not whether the name should change to some other fanciful/metaphorical name.

I like @withoutboats suggestion here: let's keep cargo clippy, and consider the possibility of having a general "run multiple linters" subcommand like cargo lint.

For that matter, I wonder if we should have a (configurable) way for cargo check and cargo build to run clippy, either via cargo check --lint or some configuration to enable that.

Member

joshtriplett commented Sep 14, 2018

@fralalonde Let's not get into name bikeshedding, please. The question is whether the name needs to become more descriptive, not whether the name should change to some other fanciful/metaphorical name.

I like @withoutboats suggestion here: let's keep cargo clippy, and consider the possibility of having a general "run multiple linters" subcommand like cargo lint.

For that matter, I wonder if we should have a (configurable) way for cargo check and cargo build to run clippy, either via cargo check --lint or some configuration to enable that.

@LegNeato

This comment has been minimized.

Show comment
Hide comment
@LegNeato

LegNeato Sep 14, 2018

At the risk of sounding dense (I am not super familiar with Rust's linting though I worked on a lot of lint stuff at Facebook) why make the distinction of user-defined vs clippy lints vs rustc lints at all? I agree with @withoutboats that these should have a unified user experience. Where the lint came from and what tool is doing the linting is an implementation detail most developers do not care about or need to know.

For example, at Facebook when you run arc lint it may report some issues from pyflakes, others from clang-tidy, others from findbugs, and yet others from regex-based PHP scripts.

Everywhere I have ever worked there has been one interface to run all lints with standard ways to turn them on and off (per-line/file/project/invocation) regardless of where they originated.

So I guess concretely for 1.0...I would suggest shipping clippy by default with rust, default all checks to off, use standard generic linting attributes and config file that do not refer to clippy, etc. Basically decouple the project name from the developer UI.

LegNeato commented Sep 14, 2018

At the risk of sounding dense (I am not super familiar with Rust's linting though I worked on a lot of lint stuff at Facebook) why make the distinction of user-defined vs clippy lints vs rustc lints at all? I agree with @withoutboats that these should have a unified user experience. Where the lint came from and what tool is doing the linting is an implementation detail most developers do not care about or need to know.

For example, at Facebook when you run arc lint it may report some issues from pyflakes, others from clang-tidy, others from findbugs, and yet others from regex-based PHP scripts.

Everywhere I have ever worked there has been one interface to run all lints with standard ways to turn them on and off (per-line/file/project/invocation) regardless of where they originated.

So I guess concretely for 1.0...I would suggest shipping clippy by default with rust, default all checks to off, use standard generic linting attributes and config file that do not refer to clippy, etc. Basically decouple the project name from the developer UI.

@llogiq

This comment has been minimized.

Show comment
Hide comment
@llogiq

llogiq Sep 15, 2018

Collaborator

Since we were citing prior work, in the Java world there's FindBugs/SpotBugs, CheckStyle,PMD, SonarQube.

Collaborator

llogiq commented Sep 15, 2018

Since we were citing prior work, in the Java world there's FindBugs/SpotBugs, CheckStyle,PMD, SonarQube.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment