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

Lints should not break compilation of dependencies in cargo #1029

Closed
nikomatsakis opened this Issue Apr 2, 2015 · 7 comments

Comments

Projects
None yet
6 participants
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 2, 2015

It's very unfortunate if changing lints (e.g., adding a new lint, or making an existing lint stronger and better, or removing a lint that's not very useful) breaks compatibility guarantees. Moreover, lints are only really useful for the current crate you're working on, not for your dependencies. We need to adopt some scheme to avoid this. My rough proposal goes like this:

  • All stable code can use any lint that it wants to.
  • We should add an option to rustc that caps all lints at warn.
  • Cargo should supply this option when building dependencies or installing software

The idea here is that if I am using somebody's package, I don't want to have compilation fail because we added a new lint in the meantime. But if I am editing my own source code, I absolutely do want compilation to fail. I feel like this bridges the gap and avoids the problem of -Wall not being able to refer to "all".

@seanmonstar

This comment has been minimized.

Copy link
Contributor

seanmonstar commented Apr 2, 2015

👍 I've resorted to #[cfg_attr(test, deny(warnings))] to my crates. However, this means it is possible to accidentally let some things slip through, such as dead code which is used by tests.

@Valloric

This comment has been minimized.

Copy link

Valloric commented Apr 2, 2015

👍 Agreed, this is pretty important to have. The example with -Wall is a good reminder of this issue.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 2, 2015

triage: I-nominated

recommend P-high/1.1

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Apr 2, 2015

oops, wrong repo!

@lilyball

This comment has been minimized.

Copy link
Contributor

lilyball commented Apr 10, 2015

👍 I think this makes a lot of sense. Heck, I'm not really sure if we should even use warnings for dependencies by default. Unless I'm willing to submit a PR to my dependency, I don't care whether it triggers lints internally. This could perhaps be some sort of configuration, e.g. in ~/.cargo/config, that determines whether dependencies should be listed, with the default being allow.

@Valloric

This comment has been minimized.

Copy link

Valloric commented Apr 11, 2015

A bit of extra perspective from the C++ world (which I inhabit): it's common to include library headers with the -isystem flag instead of -I to prevent compiler warnings from third-party code breaking builds that also use -Werror. Even for builds without -Werror, nobody wants to see warnings from code that's not theirs. It's just noise.

So cargo being smart about this without extra work from the user would be seen as a really nice (albeit small) improvement over the C++ experience.

It would be great to have this sooner rather than later, because it would give a nice first impression for all the C++ devs that will descend upon Rust when it hits 1.0. I think this fits well with @brson's idea that work between beta and 1.0 should be mostly polish.

@wycats

This comment has been minimized.

Copy link
Contributor

wycats commented Apr 24, 2015

Even for builds without -Werror, nobody wants to see warnings from code that's not theirs. It's just noise

Yep. That's precisely what we did in Cargo. Would be nice for Cargo to also be able to pass a flag to rustc that says "ignore denys" for the same reason.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.