improve error message when global_asm! uses asm! operands
#128305
Conversation
rustbot
added
S-waiting-on-review
| builtin_macros_global_asm_unsupported_operand = the `{$symbol}` operand cannot be used with `global_asm!` | ||
| .label = the `{$symbol}` operand is not meaningful for global-scoped inline assembly, remove it |
There was a problem hiding this comment.
Based on https://doc.rust-lang.org/reference/inline-assembly.html#syntax, it seems like in/out/inout/etc are "direction specifiers", while "operand" includes the variable. So maybe "specifier" rather than "operand" here?
There was a problem hiding this comment.
I think the word "operand" is used for the whole thing, so all of x = inout(reg) x, is an operand. The word "specifier" does not occur on that page, "spec" only in the grammar.
The section describing these items is https://doc.rust-lang.org/reference/inline-assembly.html#operand-type. It says that "Several types of operands are supported" and then lists in, out etc. So I think using the word "operand" is the most helpful.
The error message points to the specifier specifically because it is easy to parse without emitting any unhelpful errors, but the operand as a whole is unsupported.
| /// Used for better error messages when operand types are used that are not | ||
| /// supported by the current macro (e.g. `in` or `out` for `global_asm!`) | ||
| /// | ||
| /// returns | ||
| /// | ||
| /// - `Ok(true)` if the current token matches the keyword, and was expected | ||
| /// - `Ok(false)` if the current token does not match the keyword | ||
| /// - `Err(_)` if the current token matches the keyword, but was not expected | ||
| fn eat_operand_keyword<'a>(p: &mut Parser<'a>, symbol: Symbol, expect: bool) -> PResult<'a, bool> { |
There was a problem hiding this comment.
Every time this gets called, expect is !is_global_asm. Maybe change expect to disallowed and reverse that logic, so you only need to pass is_global_asm?
There was a problem hiding this comment.
like the previous PR, this is in preparation of adding naked_asm!. That will pass an enum as the third argument to decide whether the symbol is expected. For now I think going with the concept of "expected" makes sense because that is the terminology that the parser uses.
| builtin_macros_global_asm_unsupported_operand = the `{$symbol}` operand cannot be used with `global_asm!` | ||
| .label = the `{$symbol}` operand is not meaningful for global-scoped inline assembly, remove it |
There was a problem hiding this comment.
I think the word "operand" is used for the whole thing, so all of x = inout(reg) x, is an operand. The word "specifier" does not occur on that page, "spec" only in the grammar.
The section describing these items is https://doc.rust-lang.org/reference/inline-assembly.html#operand-type. It says that "Several types of operands are supported" and then lists in, out etc. So I think using the word "operand" is the most helpful.
The error message points to the specifier specifically because it is easy to parse without emitting any unhelpful errors, but the operand as a whole is unsupported.
| /// Used for better error messages when operand types are used that are not | ||
| /// supported by the current macro (e.g. `in` or `out` for `global_asm!`) | ||
| /// | ||
| /// returns | ||
| /// | ||
| /// - `Ok(true)` if the current token matches the keyword, and was expected | ||
| /// - `Ok(false)` if the current token does not match the keyword | ||
| /// - `Err(_)` if the current token matches the keyword, but was not expected | ||
| fn eat_operand_keyword<'a>(p: &mut Parser<'a>, symbol: Symbol, expect: bool) -> PResult<'a, bool> { |
There was a problem hiding this comment.
like the previous PR, this is in preparation of adding naked_asm!. That will pass an enum as the third argument to decide whether the symbol is expected. For now I think going with the concept of "expected" makes sense because that is the terminology that the parser uses.
|
@bors r+ |
bors
added
S-waiting-on-bors
…operand, r=Amanieu improve error message when `global_asm!` uses `asm!` operands follow-up to rust-lang#128207 what was ``` error: expected expression, found keyword `in` --> src/lib.rs:1:31 | 1 | core::arch::global_asm!("{}", in(reg)); | ^^ expected expression ``` becomes ``` error: the `in` operand cannot be used with `global_asm!` --> $DIR/parse-error.rs:150:19 | LL | global_asm!("{}", in(reg)); | ^^ the `in` operand is not meaningful for global-scoped inline assembly, remove it ``` the span of the error is just the keyword, which means that we can't create a machine-applicable suggestion here. The alternative would be to attempt to parse the full operand, but then if there are syntax errors in the operand those would be presented to the user, even though the parser already knows that the output won't be valid. Also that would require more complexity in the parser. So I think this is a nice improvement at very low cost.
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128305 (improve error message when `global_asm!` uses `asm!` operands) - rust-lang#128362 (add test for symbol visibility of `#[naked]` functions) - rust-lang#128526 (time.rs: remove "Basic usage text") - rust-lang#128531 (Miri: add a flag to do recursive validity checking) - rust-lang#128589 (allow setting `link-shared` and `static-libstdcpp` with CI LLVM) - rust-lang#128615 (rustdoc: make the hover trail for doc anchors a bit bigger) - rust-lang#128620 (Update rinja version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup
bors
added
S-waiting-on-author
|
@matthiaskrgr I believe the reported error is actually with #128362, not with this PR. I think this PR is actually fine |
|
Mh right the file seems to come from the other PR? oops :D I'll wait to see if the other rollup fails as well.. |
|
right, rollup still failed ✨ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#128305 (improve error message when `global_asm!` uses `asm!` operands) - rust-lang#128526 (time.rs: remove "Basic usage text") - rust-lang#128531 (Miri: add a flag to do recursive validity checking) - rust-lang#128578 (rustdoc: Cleanup `CacheBuilder` code for building search index) - rust-lang#128589 (allow setting `link-shared` and `static-libstdcpp` with CI LLVM) - rust-lang#128615 (rustdoc: make the hover trail for doc anchors a bit bigger) - rust-lang#128620 (Update rinja version to 0.3.0) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128305 - folkertdev:asm-parser-unsupported-operand, r=Amanieu improve error message when `global_asm!` uses `asm!` operands follow-up to rust-lang#128207 what was ``` error: expected expression, found keyword `in` --> src/lib.rs:1:31 | 1 | core::arch::global_asm!("{}", in(reg)); | ^^ expected expression ``` becomes ``` error: the `in` operand cannot be used with `global_asm!` --> $DIR/parse-error.rs:150:19 | LL | global_asm!("{}", in(reg)); | ^^ the `in` operand is not meaningful for global-scoped inline assembly, remove it ``` the span of the error is just the keyword, which means that we can't create a machine-applicable suggestion here. The alternative would be to attempt to parse the full operand, but then if there are syntax errors in the operand those would be presented to the user, even though the parser already knows that the output won't be valid. Also that would require more complexity in the parser. So I think this is a nice improvement at very low cost.
follow-up to #128207
what was
becomes
the span of the error is just the keyword, which means that we can't create a machine-applicable suggestion here. The alternative would be to attempt to parse the full operand, but then if there are syntax errors in the operand those would be presented to the user, even though the parser already knows that the output won't be valid. Also that would require more complexity in the parser.
So I think this is a nice improvement at very low cost.