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

Don't enforce snake_case for binary names #45127

Closed
Majora320 opened this issue Oct 9, 2017 · 23 comments · Fixed by #121749
Closed

Don't enforce snake_case for binary names #45127

Majora320 opened this issue Oct 9, 2017 · 23 comments · Fixed by #121749
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@Majora320
Copy link

Currently, if you have a Cargo.toml like this:

[package]
name = "some_package"
version = "0.0.0"

[[bin]]
name = "somePackage"
path = "src/main.rs"

rustc will complain:

warning: crate `somePackage` should have a snake case name such as `some_package`
  |
  = note: #[warn(non_snake_case)] on by default

I understand why this is reasonable for library crates, to enforce a common naming convention for rust libraries and source code. But it doesn't make much sense to enforce this for binaries as well, as there is no real advantage to doing so (note that the crate name isn't camelCase, only the binary name is).


$ rustc --version
rustc 1.22.0-nightly (05f8ddc46 2017-10-07)
@TimNN TimNN added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Oct 10, 2017
@MCluck90
Copy link

MCluck90 commented Oct 18, 2017

I've actually been digging in to this one and would be happy to take it on, if the Rust team agrees it should happen.

Right now, building a binary passes [[bin]]::name as the --crate-name argument. I've thought of two ways of tackling this.

  1. Don't enforce the snake_case rule when building a binary
  2. Add an optional --bin-name flag which can be used for naming the resulting binary. Defaults back to --crate-name. This would require coordination with cargo.

Personally, I prefer 2.

@jethrogb
Copy link
Contributor

jethrogb commented Dec 8, 2017

Policy should apply to executables and cdylib's alike.

It would also be great if you could turn off #[warn(non_snake_case)] for the crate name only. If I use #![allow(non_snake_case)] at the top-level, that disables the lint for my entire crate.

rcsv referenced this issue in rcsv/rcsvRogue Mar 14, 2019
change rcsvRogue -> rcsv_rogue to fix warning
@pipehappy1
Copy link

Policy should apply to executables and cdylib's alike.

It would also be great if you could turn off #[warn(non_snake_case)] for the crate name only. If I use #![allow(non_snake_case)] at the top-level, that disables the lint for my entire crate.

Agree, is there way to disable this warning for crate name while cargo build?

ximon18 added a commit to NLnetLabs/ROVer that referenced this issue Apr 8, 2021
oowekyala added a commit to lf-lang/lingua-franca that referenced this issue Jul 15, 2021
@Zingam
Copy link

Zingam commented Nov 20, 2021

This would be super simple to fix yet it is still a problem.

For example on Windows it is normal to have application names with capital letters and white spaces. We shouldn't need to manually rename the build artifact, so it would be nice to have an option to set a customized artifact name for exes and dlls.

@NickLarsenNZ
Copy link

Oh man, need this right now. Kustomize plugins look for a TitleCaseFileName.

To save having to do an extra rename, I'll just live with the warnings.

@GilShoshan94
Copy link
Contributor

Hi @oli-obk and @Manishearth,

You might be the relevant people to ask for guidance (or at least to redirect to other people).
This issue is very old and I didn't see any response from a Rust member.

Is it possible to avoid the non_snake_case warning for the build artifacts without using an #![allow(non_snake_case)] at the top-level, that disables the lint for the entire crate (which is really not desirable) ?

By build artifacts, I mean for crate-type = bin | cdylib | staticlib.
So for executable, static and dynamic system library.

(To me, the most important is the binary case)

@Manishearth
Copy link
Member

The relevant code is

let crate_ident = if let Some(name) = &cx.tcx.sess.opts.crate_name {

I think it makes sense to avoid for bin. It's not as clear for the other two and I do not want to be the one to make that call. Make a PR, I guess?

@GilShoshan94
Copy link
Contributor

@Manishearth,

Thank you for the pointer!
I will try to make the PR soon.

@Manishearth
Copy link
Member

Also FWIW this is just a suggestion, I do not know what the opinion of the relevant teams (the compiler team) is. So they may see the PR and decide that it is not the right route.

If you want to ask for a proper decision I suggest asking on the compiler team's channel on Zulip.

@GilShoshan94
Copy link
Contributor

GilShoshan94 commented May 2, 2023

I looked at the code and it's more involved than what I expected...
Not sure I can do a good PR without extensively learn the way it is built.
It still seems doable quiet without too many trouble to someone that know the code base, especially for the bin case.

I am taking your advice and will reach out to the compiler team on Zulip (here the thread).

@est31
Copy link
Member

est31 commented May 3, 2023

Over in #111130, @GilShoshan94 made a very good point about bad user experience when they want to create camelCase binary names but keep the lint for Rust identifiers internally.

Here the main issue is that the only solution left for the people requiring to name their binary differently is to use #![allow(non_snake_case)] at the top-level, that disables the lint for the entire crate, which is not desirable at all.

Basically the only thing you can do right now is to disable the lint at the top module, and then re-enable it at the top of each child module. This is obviously very sub-optimal UX and doesn't cover the identifiers created at the top level.

Even if we want to lint on non snake case binary names, it should be possible to disable the lint for the binary name only and not have the lint for normal rust identifiers be affected. This means that even if #111130 PR isn't chosen, one should at least address the precision issue. For example, this could happen by moving the linting into a new non_snake_crate_name lint or such.

In general I'm in favour of #111130 though, as on windows, it makes little sense to lint, and platform specific lints aren't really nice.

@est31
Copy link
Member

est31 commented May 3, 2023

This means that even if #111130 PR isn't chosen, one should at least address the precision issue. For example, this could happen by moving the linting into a new non_snake_crate_name lint or such.

Wanting to add: this probably will be a good idea anyways because #111130 only turns off the lint for executables, and if someone wants to disable the lint for non executable crate names, they would be in the same situation. So yeah, we should split it up in any case.


@workingjubilee to reply to your comment, these are the numbers of characters used in identifers in linux binary names on my local machine:

$ compgen -c | wc -l
2280
$ compgen -c | egrep "-" | wc -l
395
$ compgen -c | egrep "_" | wc -l
129
$ compgen -c | egrep "[A-Z]" | wc -l
12

It seems to me there is a fair usage of -, around 10% of identifiers. This is all non-scientific though, and _ is also used but note that about half of the identifiers with _ are built-in functions of my shell that start with _.

@GilShoshan94
Copy link
Contributor

@est31 I agree with you. We should have a way to turn off or on the lint only for the crate name without affecting the top level.

Also we should have better default according to the different crate_types.

From the available options for crate_type: bin, lib, rlib, dylib, cdylib, staticlib, and proc-macro,

Only bin (runnable executable) , cdylib (dynamic system library) and staticlib (static system library) are the build artifact that are exported outside of Rust ecosystem and should not be forced to be linted by default I believe (certain for bin, not 100% sure for cdylib and staticlib).

The others: lib (Rust library), dylib (dynamic Rust library), rlib (static Rust library) and proc-macro are to be consumed by the Rust ecosystem and thus should be linted by default with the Rust convention in my opinion.

@est31
Copy link
Member

est31 commented May 3, 2023

@GilShoshan94 agreed. I think splitting up the lint and making a lint that is for the crate name only is the best approach.

@GilShoshan94
Copy link
Contributor

Hi @oli-obk,

You are assigned to review my PR #111130.
While I think the PR is good for now to remove the lint on the bin crate_type, discussion with other people and especially @est31 made me realize that the better and cleaner solution would be to split the lint into several lints:

Proposal

  • The lint non_snake_case:
    Should work as before (checks variables, methods, functions, lifetime parameters and modules), except it wouldn't check crate_type.

  • The (new) lint non_snake_case_crate_bin: Would check only for crate_type == bin.

  • The (new) lint non_snake_case_crate_cdylib: Would check only for crate_type == cdylib.

  • The (new) lint non_snake_case_crate_staticlib: Would check only for crate_type == staticlib.

  • The (new) lint non_snake_case_crate_lib: Would check only for crate_type == lib.

  • The (new) lint non_snake_case_crate_dylib: Would check only for crate_type == dylib.

  • The (new) lint non_snake_case_crate_rlib: Would check only for crate_type == dylib.

  • The (new) lint non_snake_case_crate_proc_macro: Would check only for crate_type == proc-macro.

Where non_snake_case would be on warn by default as before and for the new lints:

On warn by default: the lib, dylib, rlib and proc-macro lints (as they are to be consumed by the Rust ecosystem and thus should be linted by default with the Rust convention).

On allow by default: the bin, cdylib and staticlib lints (as they are exported outside of the Rust ecosystem and should not be forced to the Rust convention).

We should probably add a lints group called non_snake_case_crate that would permits the users to set the level on all those new lints in a more comfortable manner.

Question

  1. In order to add new lints in the compiler, is there a proper procedure on how to do this ?
    Should I open an RFC for that ? (I would need a guide/documentation on how to do that please, as I have never done that before)

  2. Instead of adding one lint per crate_type, it would have been nicer to have one lint with an argument, like non_snake_case_crate(<crate_type>) , but having not seen a single lint with argument in rustc, I just assume this is not possible the way the compiler works now. Am I wrong ?

@oli-obk
Copy link
Contributor

oli-obk commented May 9, 2023

I don't think we should have that many lints for the same thing.

While we can't make lints have options (that would require its own RFC just for the ability to do that), we can have custom attributes that the lint knows about. So we could have a #![allow_non_snake_case_crate_name] attribute that works differently from #![allow(non_snake_case)], because it only affects the crate name.

@oli-obk
Copy link
Contributor

oli-obk commented May 9, 2023

If we want to go even more general (so a solution that is not specific to the lint), we could have a shallow lint modifier, so you could do #![shallow(allow(non_snake_case))] which would not affect the crate (or other item)'s contents, but only the crate/item itself. This, too, would require its own RFC.

@Manishearth
Copy link
Member

I would be against that many lints.

I think the purpose of the lint is to enforce style: first and foremost we should figure out what our style requirements are. It seems quite likely that enforcing this on binaries is not within our style needs, and not unlikely that the same is true for cdylibs and staticlibs. Once we settle on that set, those should simply not have lints: if there are people wishing to opt in to them being checked, they can file a clippy issue for a restriction lint, but let's not preemptively do work for users we are not sure exist.

Basically: whatever we figure out as the final warn set should be one lint, and everything else can be asked for individually later if there are users.

I'm not actually sure we need to worry about shallow stuff if we simply disable the lint for the three library types that have been brought up so far: it is quite reasonable for them to want to opt out; their naming is not from the Rust universe, it is from the C and universes. I think it's a valid problem to be solved, but I'm not sure we need to solve it immediately.

@Ltrlg
Copy link

Ltrlg commented May 9, 2023

The good thing with separate lints is that, together with RFC 3389, they would allow a workspace-global configuration.

@Manishearth
Copy link
Member

Yes, but the question remains to be answered if such a configuration is needed in the first place, and I don't want to preemptivel design for users we don't know exist.

The concrete needs I see here are:

  • binaries and C library types really do not need this convention
  • It is annoying to disable this lint for just the artifact name, which is needed for above^

So far I have yet to see a need to disable this lint for just the artifact name for reasons other than "I am building a binary/C library which has a different naming convention". I am also yet to see if there are other artifact types that need this.

@est31
Copy link
Member

est31 commented May 9, 2023

My point earlier was that even though that the use case that made people open this issue is addressed well by #111130, one should go further than that and reconsider how it even came to that problem. In addition to merging #111130, one should try to address the useful feedback users gave about the inflexibility to configure lints. That was my earlier point.

But thinking of it, the same problem also exists if you have a non-snake case module name or function name, for whatever reason. In those cases, too, you can't easily disable the lint on that level only, keeping it enabled for variables declared inside the body.

Maybe, considering that this is such a general weakness, shallow is really the best solution. It would also help with making #[expect] be more precise (cc #54503).

I definitely think that splitting up the lint into one for each crate type is too much. At most one would have three lints: one for non-crate names, one for crate names that are for the rust ecosystem only, and a last one that is allow by default, for externally exposed crate names. Then people creating linux-only software can disallow that lint. But IMO it occurs so rarely that people put major characters in crate names exposed to the system, one doesn't really need a lint for that only. Most linux tooling is following the convention without there being lints that enforce it.

@GilShoshan94
Copy link
Contributor

Thank you all for the replies.
Very interresting points.
I won't open an RFC.

Personally, I will be happy with #111130 getting merged as it solves my issue for the bin case.
Whenever I will encounter a case I need at work with a staticlib or a cdylib I will open a new PR for those as well.

@Manishearth
Copy link
Member

@est31 yep

Though I'm somewhat against even three lints until someone comes up with a need that they would actually wish to enable the allow-by-default lint.

Two seems fine if we're not doing shallow, but for now I think downscoping the lint is an easy step and the broader shallow thing can be discussed at its own pace.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 11, 2023
…i-obk

Don't lint snake-case on executable crate name

Fix rust-lang#45127

Hi,
First PR on Rust, I tried my best to follow "Rust Compiler Development Guide".

I tested it with a custom toolchain on a dummy bin crate with one submodule and it seems to work.
The lint `non_snake_case` ignore the binary crate name but not the module name.

Thank you `@Manishearth` and `@jyn514` for the guidance !
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 2, 2024
…henkov

Don't lint on executable crates with `non_snake_case` names

Revives rust-lang#111130, cc `@GilShoshan94.`
Closes rust-lang#45127.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 3, 2024
…nkov

Don't lint on executable crates with `non_snake_case` names

Revives rust-lang#111130, cc `@GilShoshan94.`
Closes rust-lang#45127.
@bors bors closed this as completed in 10234fc Mar 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 4, 2024
Rollup merge of rust-lang#121749 - jieyouxu:issue-45127-fix, r=petrochenkov

Don't lint on executable crates with `non_snake_case` names

Revives rust-lang#111130, cc `@GilShoshan94.`
Closes rust-lang#45127.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 21, 2024
…r=petrochenkov

Explain why `non_snake_case` is skipped for binary crates and cleanup tests

- Explain `non_snake_case` lint is skipped for bin crate names because binaries are not intended to be distributed or consumed like library crates (rust-lang#45127).
- Coalesce the bunch of tests into a single one but with revisions, which is easier to compare the differences for `non_snake_case` behavior with respect to crate types.

Follow-up to rust-lang#121749 with some more comments and test cleanup.

cc `@saethlin` who bumped into one of the tests and was confused why it was `only-x86_64-unknown-linux-gnu`.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 21, 2024
…r=<try>

Explain why `non_snake_case` is skipped for binary crates and cleanup tests

- Explain `non_snake_case` lint is skipped for bin crate names because binaries are not intended to be distributed or consumed like library crates (rust-lang#45127).
- Coalesce the bunch of tests into a single one but with revisions, which is easier to compare the differences for `non_snake_case` behavior with respect to crate types.

Follow-up to rust-lang#121749 with some more comments and test cleanup.

cc `@saethlin` who bumped into one of the tests and was confused why it was `only-x86_64-unknown-linux-gnu`.

try-job: dist-i586-gnu-i586-i686-musl
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 21, 2024
…r=petrochenkov

Explain why `non_snake_case` is skipped for binary crates and cleanup tests

- Explain `non_snake_case` lint is skipped for bin crate names because binaries are not intended to be distributed or consumed like library crates (rust-lang#45127).
- Coalesce the bunch of tests into a single one but with revisions, which is easier to compare the differences for `non_snake_case` behavior with respect to crate types.

Follow-up to rust-lang#121749 with some more comments and test cleanup.

cc `@saethlin` who bumped into one of the tests and was confused why it was `only-x86_64-unknown-linux-gnu`.

try-job: dist-i586-gnu-i586-i686-musl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet