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

Disallow non-identifier-valid --extern crate names #650

Closed
1 of 3 tasks
est31 opened this issue Jul 6, 2023 · 3 comments
Closed
1 of 3 tasks

Disallow non-identifier-valid --extern crate names #650

est31 opened this issue Jul 6, 2023 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@est31
Copy link
Member

est31 commented Jul 6, 2023

Proposal

In rust-lang/rust#113035 , @fmease has pointed out that rustc accepts any --extern crate name, even ones that wouldn't be valid identifiers:

touch b.rs; rustc b.rs --crate-type=lib
touch a.rs; rustc a.rs --edition=2021 --crate-type=lib --extern hi=libb.rlib
rustc a.rs --edition=2021 --crate-type=lib --extern "hello//i;d"=libb.rlib

As pointed out in #113035, these invalid --extern crate names are then used to provide (wrong) "did you mean" suggestions.

error[E0432]: unresolved import `hello__i_d`
 --> a.rs:1:5
  |
1 | use hello__i_d::hi;
  |     ^^^^^^^^^^ use of undeclared crate or module `hello__i_d`
  |
help: there is a crate or module with a similar name
  |
1 | use hello//i;d::hi;
  |     ~~~~~~~~~~

I propose requiring the extern crate names to be valid identifiers, and error if they are not. This prevents user confusion and wrong expectations.

So e.g. hello//i;d would be disallowed, just as hello-i-d would be, but hello_i_d would work (cargo already converts - to _, this should not affect crates with - in their name; cargo's ). hällo_world would still work as it is a valid identifier.

It would technically be a breaking change, but I don't think people specify such invalid --extern names outside of accidents. I guess a crater run before merging it would be a good idea. I don't think an entire forwards compat lint is needed.

The central reason why I am suggesting this is that it would be impossible to use such invalidly named externs. There is only one situation I can think of where you sometimes would opt for an invalid identifier, which is use via the force param, added by rust-lang/rust#109421. I said sometimes, because often you just want add the force param for crates that have important symbols. For those cases (extern with force arg plus invalid name), I propose suggesting the _ name in the error, as it is already present in the language via use foo::Trait as _; and let _ = <some expr that is must_use>;, and otherwise, suggest removing the --extern argument.

Similar to #610 in that it also proposes to do more CLI arg validation. There is also precedent in that #![crate_name = "hello-//world"] is not accepted by the compiler either.

To expand on the cargo point earlier, cargo's - to _ conversion also applies to crate renaming:

[dependencies]
"hi-world" = { package = "anyhow", version = "1.0" }

for example gets passed as --extern hi_world=/path/to/libanyhow-deadbeef.rlib. If you try to put // or :, cargo will already error. There might, or might not, be a small subset of invalid identifiers that cargo doesn't catch, but I doubt this shouldn't be a problem I think. I think it's better to have an error message when you try to include it via cargo than to have it when you try to use it.

Mentors or Reviewers

An implementation should be pretty easy. I mainly do this MCP for the approval part of it. I can help to mentor this.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@est31 est31 added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jul 6, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@estebank
Copy link

estebank commented Jul 6, 2023

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jul 6, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 6, 2023
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Aug 10, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 10, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 22, 2023
…i-opt, r=est31

[breaking change] Validate crate name in `--extern` [MCP 650]

Reject non-ASCII-identifier crate names passed to the CLI option `--extern` (`rustc`, `rustdoc`).
Implements [MCP 650](rust-lang/compiler-team#650) (except that we only allow ASCII identifiers not arbitrary Rust identifiers).
Fixes rust-lang#113035.

[As mentioned on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Disallow.20non-identifier-valid.20--extern.20cr.E2.80.A6.20compiler-team.23650/near/376826988), doing a crater run probably doesn't make sense since it wouldn't yield anything. Most users don't interact with `rustc` directly but only ever through Cargo which always passes a valid crate name to `--extern` when it invokes `rustc` and `rustdoc`. In any case, the user wouldn't be able to use such a crate name in the source code anyway.

Note that I'm not using [`rustc_session::output::validate_crate_name`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_session/output/fn.validate_crate_name.html) (used for `--crate-name` and `#![crate_name]`) since the latter doesn't reject non-ASCII crate names and ones that start with a digit.

As an aside, I've also thought about getting rid of `validate_crate_name` entirely in a separate PR (with another MCP) in favor of `is_ascii_ident` to reject more weird `--crate-name`s, `#![crate_name]`s and file names but I think that would lead to a lot of actual breakage, namely because of file names starting with a digit. In `tests/ui` 9 tests would be impacted for example.

CC `@estebank`
r? `@est31`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 23, 2023
…est31

[breaking change] Validate crate name in `--extern` [MCP 650]

Reject non-ASCII-identifier crate names passed to the CLI option `--extern` (`rustc`, `rustdoc`).
Implements [MCP 650](rust-lang/compiler-team#650) (except that we only allow ASCII identifiers not arbitrary Rust identifiers).
Fixes #113035.

[As mentioned on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Disallow.20non-identifier-valid.20--extern.20cr.E2.80.A6.20compiler-team.23650/near/376826988), doing a crater run probably doesn't make sense since it wouldn't yield anything. Most users don't interact with `rustc` directly but only ever through Cargo which always passes a valid crate name to `--extern` when it invokes `rustc` and `rustdoc`. In any case, the user wouldn't be able to use such a crate name in the source code anyway.

Note that I'm not using [`rustc_session::output::validate_crate_name`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_session/output/fn.validate_crate_name.html) (used for `--crate-name` and `#![crate_name]`) since the latter doesn't reject non-ASCII crate names and ones that start with a digit.

As an aside, I've also thought about getting rid of `validate_crate_name` entirely in a separate PR (with another MCP) in favor of `is_ascii_ident` to reject more weird `--crate-name`s, `#![crate_name]`s and file names but I think that would lead to a lot of actual breakage, namely because of file names starting with a digit. In `tests/ui` 9 tests would be impacted for example.

CC `@estebank`
r? `@est31`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…est31

[breaking change] Validate crate name in `--extern` [MCP 650]

Reject non-ASCII-identifier crate names passed to the CLI option `--extern` (`rustc`, `rustdoc`).
Implements [MCP 650](rust-lang/compiler-team#650) (except that we only allow ASCII identifiers not arbitrary Rust identifiers).
Fixes #113035.

[As mentioned on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Disallow.20non-identifier-valid.20--extern.20cr.E2.80.A6.20compiler-team.23650/near/376826988), doing a crater run probably doesn't make sense since it wouldn't yield anything. Most users don't interact with `rustc` directly but only ever through Cargo which always passes a valid crate name to `--extern` when it invokes `rustc` and `rustdoc`. In any case, the user wouldn't be able to use such a crate name in the source code anyway.

Note that I'm not using [`rustc_session::output::validate_crate_name`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_session/output/fn.validate_crate_name.html) (used for `--crate-name` and `#![crate_name]`) since the latter doesn't reject non-ASCII crate names and ones that start with a digit.

As an aside, I've also thought about getting rid of `validate_crate_name` entirely in a separate PR (with another MCP) in favor of `is_ascii_ident` to reject more weird `--crate-name`s, `#![crate_name]`s and file names but I think that would lead to a lot of actual breakage, namely because of file names starting with a digit. In `tests/ui` 9 tests would be impacted for example.

CC `@estebank`
r? `@est31`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…est31

[breaking change] Validate crate name in `--extern` [MCP 650]

Reject non-ASCII-identifier crate names passed to the CLI option `--extern` (`rustc`, `rustdoc`).
Implements [MCP 650](rust-lang/compiler-team#650) (except that we only allow ASCII identifiers not arbitrary Rust identifiers).
Fixes #113035.

[As mentioned on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Disallow.20non-identifier-valid.20--extern.20cr.E2.80.A6.20compiler-team.23650/near/376826988), doing a crater run probably doesn't make sense since it wouldn't yield anything. Most users don't interact with `rustc` directly but only ever through Cargo which always passes a valid crate name to `--extern` when it invokes `rustc` and `rustdoc`. In any case, the user wouldn't be able to use such a crate name in the source code anyway.

Note that I'm not using [`rustc_session::output::validate_crate_name`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_session/output/fn.validate_crate_name.html) (used for `--crate-name` and `#![crate_name]`) since the latter doesn't reject non-ASCII crate names and ones that start with a digit.

As an aside, I've also thought about getting rid of `validate_crate_name` entirely in a separate PR (with another MCP) in favor of `is_ascii_ident` to reject more weird `--crate-name`s, `#![crate_name]`s and file names but I think that would lead to a lot of actual breakage, namely because of file names starting with a digit. In `tests/ui` 9 tests would be impacted for example.

CC `@estebank`
r? `@est31`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants