Allow multiple asm! options groups and report an error on duplicate options
#73227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Is there a way to add tests to check the semantics? |
|
r? @Amanieu |
Amanieu
reviewed
Jun 11, 2020
Amanieu
reviewed
Jun 12, 2020
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I added the tests in a separate file, |
|
LGTM. Are you planning on implementing the check for duplicate options as well? |
|
Yes, I just haven’t gotten to it yet |
|
Okay, I got a warning working: Should I make it a lint rather than a hard-coded warning? |
Amanieu
reviewed
Jun 14, 2020
Amanieu
reviewed
Jun 15, 2020
|
I feel like there should be a lint for options blocks that do nothing, e.g.: asm!("", options()); // <- warning: useless options
asm!("", options(att_syntax, nomem), options(att_syntax));
// ^--- warning: useless optionsI'm not sure if it would be worth it, though. Also, it should be implemented in a separate pull request. |
camelid
changed the title
asm! optionsasm! options and error on duplicate options
Amanieu
reviewed
Jun 15, 2020
|
This is going to conflict with #73364, so let's wait for that one to land first. |
I think this a lint is necessary. This kind of code may be generated from macros, and it's pretty obvious otherwise. |
camelid
commented
Jun 16, 2020
src/librustc_builtin_macros/asm.rs
Outdated
Comment on lines
278
to
285
| if p.look_ahead(0, |t| t == &token::Comma) { | ||
| err.tool_only_span_suggestion( | ||
| p.token.span, | ||
| "remove this comma", | ||
| String::new(), | ||
| Applicability::MachineApplicable, | ||
| ); | ||
| } |
There was a problem hiding this comment.
@Amanieu, the way I implemented the suggestion with the comma is to create a separate, tool-only suggestion to remove the comma if it's there. Otherwise, the UI makes it look like the comma is part of the option.
The tests will fail, though, because for some reason, the tool-only suggestion is showing up in the UI and in a very bizarre way. Am I doing something wrong? Maybe I should just go back to 13c0caff100cc69a6699fb727a5684fbb1fc10c1.
Here's what it looks like:
error: the `att_syntax` option was already provided
--> rust-issue-73193.rs:10:28
|
10 | options(nomem, att_syntax, nostack, att_syntax),
| ^^^^^^^^^^
|
help: remove this option
|
10 | options(nomem, , nostack, att_syntax),
| --
There was a problem hiding this comment.
I'm a bit out of my depth here, so I will delegate to @estebank who is more familiar with the diagnostic infrastructure.
There was a problem hiding this comment.
I'm thinking about just going back to 13c0caf, and have it highlight the option and the comma, and then I can revise it in a later pull request. What do you think?
There was a problem hiding this comment.
I believe that here you can use
let comma_sp = p.look_ahead(1, |t| t.span);
p.token.span.to(comma_sp)to point at att_syntax,, although ideally, you would do
let next_ident = p.look_ahead(2, |t| t.span);
p.token.span.until(next_ident)
to point at att_syntax, (including the whitespace before nostack).
There was a problem hiding this comment.
@estebank In 13c0caf, I was able to point at att_syntax,, but I would rather just point at att_syntax in the UI, but have the tooling see att_syntax,.
So, my goal for the UI is:
error: the `att_syntax` option was already provided
--> rust-issue-73193.rs:10:28
|
10 | options(nomem, att_syntax, nostack),
| ^^^^^^^^^^
And my goal for the tooling is the suggestion:
- options(nomem, att_syntax, nostack)
+ options(nomem, , nostack)There was a problem hiding this comment.
We have a tool_only_suggestion that will not be shown in the cli output, but the suggestion will still appear in VSCode and be applicable by rustfix. That's what I normally use for a case like this.
The display of structured suggestions are normally as a separate window with the change applied. There's an optimization to show it as a label with the suggested code as part of the label, but there are a bunch of rules on when it doesn't get displayed that way (length of label, if there are more than one suggestion, devel selectable with span_suggestion_verbose or span_suggestion_short).
There was a problem hiding this comment.
Hmm. So what do you recommend I do? Because the help message is confusing for the users:
error: the `att_syntax` option was already provided
--> rust-issue-73193.rs:10:28
|
10 | options(nomem, att_syntax, nostack, att_syntax),
| ^^^^^^^^^^
|
help: remove this option
|
10 | options(nomem, , nostack, att_syntax),
| --
(The help message only appears separately and it only highlights the wrong part when the tool-only suggestion for the comma is there. I tested it with a user-visible suggestion too, and it messed up the help message as well.)
Is it not possible to have multiple suggestions on a diagnostic (or am I doing it wrong)?
Thanks for your help!
There was a problem hiding this comment.
there's a multipart_suggestion method that takes a Vec<(Span, String)> that can be used, but that shouldn't be necessary for this case, I think. What I would do is have a span_label pointing at att_syntax saying "option already provide it, remove it" and a hidden structured suggestion with a span between the start of att_syntax to the start of nostack for the removal. That way in VSCode you'd see
option already provided, remove it
remove the option
which is not perfect, but serviceable.
There was a problem hiding this comment.
Okay, I’ll try that. Thanks for helping me figure this out!
There was a problem hiding this comment.
@Amanieu What do you think of this output?
error: the `att_syntax` option was already provided
--> rust-issue-73193.rs:10:28
|
10 | options(nomem, att_syntax, nostack, att_syntax),
| ^^^^^^^^^^ this option was already provided
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
estebank
reviewed
Jun 18, 2020
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #73511) made this pull request unmergeable. Please resolve the merge conflicts. |
camelid
changed the title
asm! options and error on duplicate optionsasm! options groups and report an error on duplicate options
|
Time for some rebasing! |
For some reason, the help message is now in a separate message, which adds a lot of noise. I would like to try to get it back to one message.
That way the comma isn't highlighted as part of the option in the UI. Weirdly, the comma removal suggestion shows up in the UI.
The UI isn't glitching anymore.
camelid
force-pushed
the
multiple-asm-options
branch
from
f901ba2 to
c31785a
Compare
|
@Amanieu It looks like the rebase worked. |
|
@bors r+ |
|
📌 Commit c31785a has been approved by |
bors
added
S-waiting-on-bors
|
Thanks for reviewing this and helping me along the way! I really appreciate it :) |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 20, 2020
…arth Rollup of 9 pull requests Successful merges: - rust-lang#72456 (Try to suggest dereferences on trait selection failed) - rust-lang#72788 (Projection bound validation) - rust-lang#72790 (core/time: Add Duration methods for zero) - rust-lang#73227 (Allow multiple `asm!` options groups and report an error on duplicate options) - rust-lang#73287 (lint: normalize projections using opaque types) - rust-lang#73291 (Pre-compute `LocalDefId` <-> `HirId` mappings and remove `NodeId` <-> `HirId` conversion APIs) - rust-lang#73378 (Remove use of specialization from librustc_arena) - rust-lang#73411 (Update bootstrap to rustc 1.45.0-beta.2 (1dc0f6d 2020-06-15)) - rust-lang#73443 (ci: allow gating GHA on everything but macOS) Failed merges: r? @ghost
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 10, 2021
Fix ABNF of inline asm options This is the case since rust-lang#73227. r? `@camelid`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #73193
Cc @joshtriplett @Amanieu