Improve the help message for an invalid calling convention

[khyperia]

Fixes #93601

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]

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]

Can you also add a UI test for this flag?

[khyperia]

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?

[jyn514]

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.

[khyperia]

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]

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]

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]

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

[khyperia]

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?

[jyn514]

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.

[khyperia]

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

[bors]

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

[nagisa]

This looks great!

[nagisa]

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

[khyperia]

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]

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]

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]

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

[davidtwco]

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]

rustc_error_messages was changed

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

[khyperia]

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]

@khyperia fyi you can use

@rustbot ready

to mark this as ready for review.

[bors]

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

[nagisa]

@bors r+

[bors]

📌 Commit 9a206a7 has been approved by nagisa

It is now in the queue for this repository.

[nagisa]

@bors r-
@bors r=nagisa,jyn514

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

[bors]

📌 Commit 9a206a7 has been approved by nagisa,jyn514

It is now in the queue for this repository.

[bors]

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

[bors]

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

[rust-timer]

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.

	mean1	range	count2
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.

	mean1	range	count2
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

Footnotes

1. the arithmetic mean of the percent change ↩ ↩²

2. number of relevant changes ↩ ↩²