Improve the help message for an invalid calling convention #100488

khyperia · 2022-08-13T09:35:41Z

I mostly followed the suggestions of @nagisa in that issue, however, I wasn't sure how to check stability for the suggestion of "Do not suggest CCs that cannot be used due to them being unstable and feature not being enabled", so I did not implement that point.

I haven't contributed to rustc much, please feel free to point out suggestions! For example, the .map(|s| Symbol::intern(s)).collect::<Vec<_>>() seems pretty gross performance-wise, but maybe that's OK in error reporting code.

rust-highfive · 2022-08-13T09:35:44Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

jyn514

I wasn't sure how to check stability

I'm not sure either, but I'd suggest compiling a crate with an unstable ABI and using -Ztreat-err-as-bug to see where the error is reported. Then you can use that same logic in the diagnostics code :)

the .map(|s| Symbol::intern(s)).collect::<Vec<_>>() seems pretty gross performance-wise, but maybe that's OK in error reporting code.

Yeah, that's fine, error reporting doesn't care about performance.

jyn514 · 2022-08-13T10:57:29Z

compiler/rustc_driver/src/lib.rs

+            CallingConventions => {
+                let mut calling_conventions = rustc_target::spec::abi::all_names();
+                calling_conventions.sort_unstable();
+                println!("{}", calling_conventions.join("\n"));


Can you also add a UI test for this flag?

Hmm, I'm not finding any tests for other --print options, e.g. --print=target-list has extremely similar code to this one. Could you point out where those are so I can add this test alongside them?

Hmm, not sure off the top of my head. Maybe it's in run-make? If I were at a laptop I'd find this with grep -r -- --print src/test, but I'm on mobile right now.

Ah thanks! Found this, but it's a bit odd as there seems to be no expected output. I'll add a similar Makefile for calling-conventions. https://github.com/rust-lang/rust/tree/master/src/test/run-make-fulldeps/print-target-list

khyperia · 2022-08-13T11:31:55Z

I'm not sure either, but I'd suggest compiling a crate with an unstable ABI and using -Ztreat-err-as-bug to see where the error is reported. Then you can use that same logic in the diagnostics code :)

Hmm. The errors seem to be reported here

rust/compiler/rustc_ast_passes/src/feature_gate.rs

Line 77 in 75b7e52

match symbol_unescaped.as_str() {

The crate rustc_ast_passes is not available in rustc_ast_lowering. Should I add a reference? Also, I'm not sure how to call that function without reporting an error - do I need to construct a dummy Session or something like that? How do I do that?

jyn514 · 2022-08-13T12:58:17Z

The crate rustc_ast_passes is not available in rustc_ast_lowering. Should I add a reference? Also, I'm not sure how to call that function without reporting an error - do I need to construct a dummy Session or something like that?

I'd suggest taking that giant match and refactoring it a bit - add a public function in rustc_ast_lowering that only determines whether the ABI is unstable or not, then calling it from the feature gate code in rustc_ast_passes. Then you don't need a dummy session and you can make sure the diagnostic code uses the same logic as the feature gate code.

jyn514

This looks great :) I'll let a compiler member review since I don't often touch target specs, but the code LGTM.

jyn514 · 2022-08-14T13:36:03Z

compiler/rustc_target/src/spec/abi.rs

+    Unrecognized,
+}
+
+macro_rules! gate_feature_post {


nit: I would change this from a macro to a function, since it no longer affects control flow.

The one slight annoyance with that is that $feature is used in both $features.$feature (bool field for if it's enabled), as well as sym::$feature, so the caller would have to duplicate the name of the feature, passing in both. I guess that's okay, though, and probably worth it being a regular function over a macro?

Hmm. I agree changing it is a pain. But I also kind of feel that if this is a macro, it's not helping enough for it to be worth it - it would be nice to find a way to simplify the giant match some more, e.g. only pass in a list of ABI names and feature gates and have it generate the match for you.

That said, it's a big change unrelated to your improvement, I'm fine with just merging as is.

ah! @m-ou-se pointed out that Features::enabled(Symbol) exists, that makes it nicer.

bors · 2022-08-27T03:28:45Z

☔ The latest upstream changes (presumably #101064) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa

This looks great!

src/test/ui/parser/issues/issue-8537.stderr

nagisa · 2022-08-28T10:01:34Z

src/test/run-make-fulldeps/print-calling-conventions/Makefile

+-include ../tools.mk
+
+all:
+	$(RUSTC) --print calling-conventions


Hm, wouldn’t this be better off as a UI test?

Not sure - I'm following the example set by other similar tests. If we want to change the group of them to test in a different way, that seems like a separate change IMO #100488 (comment)

nagisa · 2022-08-28T10:06:52Z

I'll let a compiler member review since I don't often touch target specs, but the code LGTM.

r=me in that regard. This needs a rebase and this could probably benefit from fewer commits (e.g. squash in the commits that fix mistakes introduced by prior commits as well as those that add tests)

khyperia · 2022-08-28T11:01:44Z

I'm not sure how to resolve the merge conflict from 0043d10, to be honest. I don't know how this new error reporting system works, or what this .ftl file is. Is there any documentation on how this extremely magical-seeming system works, so I can use it properly? (macros are one thing, this seems like a whole custom DSL, haha)

@JeanCASPAR do you think you could help out?

jyn514 · 2022-08-28T14:31:33Z

cc @davidtwco , is there documentation somewhere for how to use the translation macros?

davidtwco · 2022-08-30T15:24:28Z

cc @davidtwco , is there documentation somewhere for how to use the translation macros?

https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-structs.html primarily, #100717 links most of our resources

rustbot · 2022-09-08T14:20:02Z

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

khyperia · 2022-09-08T14:26:02Z

Sorry for taking so long for the update! @nagisa could I get another review? I (...we) changed some stuff in the rebase, it might be nice to check again. Thanks!

jyn514 · 2022-09-08T15:05:26Z

@khyperia fyi you can use

@rustbot ready

to mark this as ready for review.

bors · 2022-09-21T23:17:16Z

☔ The latest upstream changes (presumably #101558) made this pull request unmergeable. Please resolve the merge conflicts.

nagisa · 2022-09-22T21:57:08Z

@bors r+

bors · 2022-09-22T21:57:10Z

📌 Commit 9a206a7 has been approved by nagisa

It is now in the queue for this repository.

nagisa · 2022-09-22T21:58:03Z

@bors r-
@bors r=nagisa,jyn514

(/me wonders if bors can stomach this...)

bors · 2022-09-22T21:58:05Z

📌 Commit 9a206a7 has been approved by nagisa,jyn514

It is now in the queue for this repository.

bors · 2022-09-23T04:22:17Z

⌛ Testing commit 9a206a7 with merge 77e7e88...

bors · 2022-09-23T06:52:33Z

☀️ Test successful - checks-actions
Approved by: nagisa,jyn514
Pushing 77e7e88 to master...

rust-timer · 2022-09-23T08:11:32Z

Finished benchmarking commit (77e7e88): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	3.4%	[3.4%, 3.4%]	1
Regressions ❌ (secondary)	4.1%	[2.6%, 5.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.4%	[3.4%, 3.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean¹	range	count²
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 13, 2022

rust-highfive assigned michaelwoerister Aug 13, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 13, 2022

This comment has been minimized.

Sign in to view

jyn514 reviewed Aug 13, 2022

View reviewed changes

khyperia force-pushed the invalid-calling-convention-help-message branch from 9edf69a to fb426cf Compare August 14, 2022 13:22

jyn514 approved these changes Aug 14, 2022

View reviewed changes

nagisa reviewed Aug 28, 2022

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 30, 2022

m-ou-se force-pushed the invalid-calling-convention-help-message branch from 45ea9a6 to c46abb6 Compare September 8, 2022 14:19

rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Sep 8, 2022

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 8, 2022

Improve the help message for an invalid calling convention

9a206a7

khyperia force-pushed the invalid-calling-convention-help-message branch from c46abb6 to 9a206a7 Compare September 22, 2022 20:42

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 22, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2022

bors merged commit 77e7e88 into rust-lang:master Sep 23, 2022

rustbot added this to the 1.66.0 milestone Sep 23, 2022

khyperia deleted the invalid-calling-convention-help-message branch September 23, 2022 06:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the help message for an invalid calling convention #100488

Improve the help message for an invalid calling convention #100488

khyperia commented Aug 13, 2022 •

edited

Loading

rust-highfive commented Aug 13, 2022

This comment has been minimized.

jyn514 left a comment

jyn514 Aug 13, 2022

khyperia Aug 13, 2022

jyn514 Aug 13, 2022

khyperia Aug 14, 2022

khyperia commented Aug 13, 2022

jyn514 commented Aug 13, 2022

jyn514 left a comment

jyn514 Aug 14, 2022

khyperia Aug 14, 2022

jyn514 Aug 14, 2022

khyperia Aug 14, 2022

bors commented Aug 27, 2022

nagisa left a comment

nagisa Aug 28, 2022

khyperia Aug 28, 2022

nagisa commented Aug 28, 2022

khyperia commented Aug 28, 2022 •

edited

Loading

jyn514 commented Aug 28, 2022

davidtwco commented Aug 30, 2022

rustbot commented Sep 8, 2022

khyperia commented Sep 8, 2022

jyn514 commented Sep 8, 2022

bors commented Sep 21, 2022

nagisa commented Sep 22, 2022

bors commented Sep 22, 2022

nagisa commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 23, 2022

bors commented Sep 23, 2022

rust-timer commented Sep 23, 2022

Improve the help message for an invalid calling convention #100488

Improve the help message for an invalid calling convention #100488

Conversation

khyperia commented Aug 13, 2022 • edited Loading

rust-highfive commented Aug 13, 2022

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia commented Aug 13, 2022

jyn514 commented Aug 13, 2022

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 27, 2022

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Aug 28, 2022

khyperia commented Aug 28, 2022 • edited Loading

jyn514 commented Aug 28, 2022

davidtwco commented Aug 30, 2022

rustbot commented Sep 8, 2022

khyperia commented Sep 8, 2022

jyn514 commented Sep 8, 2022

bors commented Sep 21, 2022

nagisa commented Sep 22, 2022

bors commented Sep 22, 2022

nagisa commented Sep 22, 2022

bors commented Sep 22, 2022

bors commented Sep 23, 2022

bors commented Sep 23, 2022

rust-timer commented Sep 23, 2022

Overall result: no relevant changes - no action needed

Footnotes

khyperia commented Aug 13, 2022 •

edited

Loading

khyperia commented Aug 28, 2022 •

edited

Loading