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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions compiler/rustc_session/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2451,6 +2451,19 @@ pub fn parse_externs(
Some((opts, name)) => (Some(opts), name.to_string()),
};

if !crate::utils::is_ascii_ident(&name) {
let mut error = handler.early_struct_error(format!(
"crate name `{name}` passed to `--extern` is not a valid ASCII identifier"
));
let adjusted_name = name.replace("-", "_");
if crate::utils::is_ascii_ident(&adjusted_name) {
error.help(format!(
"consider replacing the dashes with underscores: `{adjusted_name}`"
));
}
error.emit();
}
Comment on lines +2454 to +2465
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not worth it given how rare this error will be in practice, but on other non-ASCII errors we go beyond replacing - with _ and try to provide more context on which chars were problematic and try to provide suggestions to similar ASCII chars when possible. This is particularly useful for homonyms, where someone is trying to sneak (on purpose or accidentally) a, for example, cyrillic char.

Copy link
Member Author

@fmease fmease Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lints uncommon_codepoints, mixed_script_confusables and confusable_idents are amazing for sure! I'd lean towards not doing that here since it'd mean moving the code from rustc_lint here and adapting it to an environment that doesn't have a Session yet, yuck! Not sure how easy that'd be, maybe it's not that bad. early_errors can't even be translated right now.

Esp. since --extern is not actually used directly as we've established (apart from rustc devs). Alternative Rust package managers should be able to figure out what to pass to --extern relatively quickly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmease fair points 👍 . It can also be later work, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My stance is always "don't block on improving a solution short of perfect as long as it improves over the status quo", and this PR comfortably glides over that bar.


let path = path.map(|p| CanonicalizedPath::new(p));

let entry = externs.entry(name.to_owned());
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_session/src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1710,6 +1710,15 @@ impl EarlyErrorHandler {
self.handler.struct_fatal(msg).emit()
}

#[allow(rustc::untranslatable_diagnostic)]
#[allow(rustc::diagnostic_outside_of_impl)]
pub(crate) fn early_struct_error(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that I had to introduce this method. I can get rid of it again at the cost of not showing the .help("replace dashes with underscores …").

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine.

&self,
msg: impl Into<DiagnosticMessage>,
) -> DiagnosticBuilder<'_, !> {
self.handler.struct_fatal(msg)
}

#[allow(rustc::untranslatable_diagnostic)]
#[allow(rustc::diagnostic_outside_of_impl)]
pub fn early_warn(&self, msg: impl Into<DiagnosticMessage>) {
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_session/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,12 @@ pub fn extra_compiler_flags() -> Option<(Vec<String>, bool)> {

if !result.is_empty() { Some((result, excluded_cargo_defaults)) } else { None }
}

pub(crate) fn is_ascii_ident(string: &str) -> bool {
let mut chars = string.chars();
if let Some(start) = chars.next() && (start.is_ascii_alphabetic() || start == '_') {
chars.all(|char| char.is_ascii_alphanumeric() || char == '_')
} else {
false
}
}
2 changes: 1 addition & 1 deletion tests/run-make/incr-foreign-head-span/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ INCR=$(TMPDIR)/incr
all:
cp first_crate.rs second_crate.rs $(TMPDIR)
$(RUSTC) $(TMPDIR)/first_crate.rs -C incremental=$(INCR) --target $(TARGET) --crate-type lib
$(RUSTC) $(TMPDIR)/second_crate.rs -C incremental=$(INCR) --target $(TARGET) --extern first-crate=$(TMPDIR) --crate-type lib
$(RUSTC) $(TMPDIR)/second_crate.rs -C incremental=$(INCR) --target $(TARGET) --extern first_crate=$(TMPDIR)/libfirst_crate.rlib --crate-type lib
Copy link
Member Author

@fmease fmease Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still baffled that this test used to work at all, can somebody enlighten me? If I manually follow the steps as taken by this Makefile, I correctly get the error can't find crate for first_crate with a nightly rustc (since the name is first-crate on master). Is there any preprocessing going on here?

Apart from the incorrect crate name, --extern can't point to a directory, which I've also fixed. What is going on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is weird indeed. I've checked the logs of recently successful runs and tests/run-make/incr-foreign-head-span does succeed on master. Very weird.

rm $(TMPDIR)/first_crate.rs
$(RUSTC) $(TMPDIR)/second_crate.rs -C incremental=$(INCR) --target $(TARGET) --cfg second_run --crate-type lib

10 changes: 10 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name-dashed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// compile-flags: --extern=my-awesome-library=libawesome.rlib
// error-pattern: crate name `my-awesome-library` passed to `--extern` is not a valid ASCII identifier
// error-pattern: consider replacing the dashes with underscores: `my_awesome_library`

// In a sense, this is a regression test for issue #113035. We no longer suggest
// `pub use my-awesome-library::*;` (sic!) as we outright ban this crate name.

pub use my_awesome_library::*;

fn main() {}
4 changes: 4 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name-dashed.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: crate name `my-awesome-library` passed to `--extern` is not a valid ASCII identifier
|
= help: consider replacing the dashes with underscores: `my_awesome_library`

4 changes: 4 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name-non-ascii.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// compile-flags: --extern čɍαţē=libnon_ascii.rlib
// error-pattern: crate name `čɍαţē` passed to `--extern` is not a valid ASCII identifier

fn main() {}
2 changes: 2 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name-non-ascii.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
error: crate name `čɍαţē` passed to `--extern` is not a valid ASCII identifier

4 changes: 4 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// compile-flags: --extern=?#1%$
// error-pattern: crate name `?#1%$` passed to `--extern` is not a valid ASCII identifier

fn main() {}
2 changes: 2 additions & 0 deletions tests/ui/extern-flag/invalid-crate-name.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
error: crate name `?#1%$` passed to `--extern` is not a valid ASCII identifier

6 changes: 0 additions & 6 deletions tests/ui/rfcs/rfc-2457-non-ascii-idents/crate_name_nonascii_forbidden-2.rs
View file
Open in desktop
Copy link
Member Author

@fmease fmease Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has become obsolete since it would've triggered on the --extern first, instead of on the use item. It's superseded by extern-flag/invalid-crate-name-non-ascii.rs.

This file was deleted.

8 changes: 0 additions & 8 deletions tests/ui/rfcs/rfc-2457-non-ascii-idents/crate_name_nonascii_forbidden-2.stderr
View file
Open in desktop

This file was deleted.

0 ...idents/crate_name_nonascii_forbidden-1.rs → ...i-idents/crate_name_nonascii_forbidden.rs
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion ...ts/crate_name_nonascii_forbidden-1.stderr → ...ents/crate_name_nonascii_forbidden.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: cannot load a crate with a non-ascii name `ьаг`
--> $DIR/crate_name_nonascii_forbidden-1.rs:1:1
--> $DIR/crate_name_nonascii_forbidden.rs:1:1
|
LL | extern crate ьаг;
| ^^^^^^^^^^^^^^^^^
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/rust-2018/trait-import-suggestions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// edition:2018
// aux-build:trait-import-suggestions.rs
// compile-flags:--extern trait-import-suggestions
// compile-flags:--extern trait_import_suggestions

mod foo {
mod foobar {
Expand Down
Loading