-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
[breaking change] Validate crate name in --extern
[MCP 650]
#116001
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Apart from the incorrect crate name, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
rm $(TMPDIR)/first_crate.rs | ||
$(RUSTC) $(TMPDIR)/second_crate.rs -C incremental=$(INCR) --target $(TARGET) --cfg second_run --crate-type lib | ||
|
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() {} |
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` | ||
|
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() {} |
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 | ||
|
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() {} |
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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test has become obsolete since it would've triggered on the |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
andconfusable_idents
are amazing for sure! I'd lean towards not doing that here since it'd mean moving the code fromrustc_lint
here and adapting it to an environment that doesn't have aSession
yet, yuck! Not sure how easy that'd be, maybe it's not that bad.early_error
s 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.