From c883fa45d39a5ef7316e2b6637885ec222b609d5 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 10 Jun 2020 17:47:13 -0700 Subject: [PATCH 01/20] Allow multiple `asm!` options --- src/librustc_builtin_macros/asm.rs | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 2988567960464..e98431a269591 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -16,7 +16,7 @@ struct AsmArgs { named_args: FxHashMap, reg_args: FxHashSet, options: ast::InlineAsmOptions, - options_span: Option, + options_spans: Option>, } fn parse_args<'a>( @@ -59,7 +59,7 @@ fn parse_args<'a>( named_args: FxHashMap::default(), reg_args: FxHashSet::default(), options: ast::InlineAsmOptions::empty(), - options_span: None, + options_spans: None, }; let mut allow_templates = true; @@ -174,9 +174,9 @@ fn parse_args<'a>( // Validate the order of named, positional & explicit register operands and options. We do // this at the end once we have the full span of the argument available. - if let Some(options_span) = args.options_span { + if let Some(ref options_spans) = args.options_spans { ecx.struct_span_err(span, "arguments are not allowed after options") - .span_label(options_span, "previous options") + .span_labels(options_spans.clone(), "previous options") .span_label(span, "argument") .emit(); } @@ -227,21 +227,21 @@ fn parse_args<'a>( if args.options.contains(ast::InlineAsmOptions::NOMEM) && args.options.contains(ast::InlineAsmOptions::READONLY) { - let span = args.options_span.unwrap(); - ecx.struct_span_err(span, "the `nomem` and `readonly` options are mutually exclusive") + let spans = args.options_spans.clone().unwrap(); + ecx.struct_span_err(spans, "the `nomem` and `readonly` options are mutually exclusive") .emit(); } if args.options.contains(ast::InlineAsmOptions::PURE) && args.options.contains(ast::InlineAsmOptions::NORETURN) { - let span = args.options_span.unwrap(); - ecx.struct_span_err(span, "the `pure` and `noreturn` options are mutually exclusive") + let spans = args.options_spans.clone().unwrap(); + ecx.struct_span_err(spans, "the `pure` and `noreturn` options are mutually exclusive") .emit(); } if args.options.contains(ast::InlineAsmOptions::PURE) && !args.options.intersects(ast::InlineAsmOptions::NOMEM | ast::InlineAsmOptions::READONLY) { - let span = args.options_span.unwrap(); + let span = args.options_spans.clone().unwrap(); ecx.struct_span_err( span, "the `pure` option must be combined with either `nomem` or `readonly`", @@ -267,7 +267,7 @@ fn parse_args<'a>( } if args.options.contains(ast::InlineAsmOptions::PURE) && !have_real_output { ecx.struct_span_err( - args.options_span.unwrap(), + args.options_spans.clone().unwrap(), "asm with `pure` option must have at least one output", ) .emit(); @@ -314,13 +314,10 @@ fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), Diagn } let new_span = span_start.to(p.prev_token.span); - if let Some(options_span) = args.options_span { - p.struct_span_err(new_span, "asm options cannot be specified multiple times") - .span_label(options_span, "previously here") - .span_label(new_span, "duplicate options") - .emit(); + if let Some(options_spans) = &mut args.options_spans { + options_spans.push(new_span); } else { - args.options_span = Some(new_span); + args.options_spans = Some(vec![new_span]); } Ok(()) From e61411673c8e47c504ff36cbc3479c0dd8939309 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 10 Jun 2020 18:21:22 -0700 Subject: [PATCH 02/20] Update tests --- src/test/ui/asm/parse-error.rs | 5 ---- src/test/ui/asm/parse-error.stderr | 48 ++++++++---------------------- 2 files changed, 12 insertions(+), 41 deletions(-) diff --git a/src/test/ui/asm/parse-error.rs b/src/test/ui/asm/parse-error.rs index fbf399d8b075c..538a3fde8fdeb 100644 --- a/src/test/ui/asm/parse-error.rs +++ b/src/test/ui/asm/parse-error.rs @@ -34,11 +34,6 @@ fn main() { //~^ ERROR expected one of asm!("", options(nomem, foo)); //~^ ERROR expected one of - asm!("", options(), options()); - //~^ ERROR asm options cannot be specified multiple times - asm!("", options(), options(), options()); - //~^ ERROR asm options cannot be specified multiple times - //~^^ ERROR asm options cannot be specified multiple times asm!("{}", options(), const foo); //~^ ERROR arguments are not allowed after options asm!("{a}", a = const foo, a = const bar); diff --git a/src/test/ui/asm/parse-error.stderr b/src/test/ui/asm/parse-error.stderr index ba7e8f7a03cca..dfbfc0abe3472 100644 --- a/src/test/ui/asm/parse-error.stderr +++ b/src/test/ui/asm/parse-error.stderr @@ -82,32 +82,8 @@ error: expected one of `)`, `att_syntax`, `nomem`, `noreturn`, `nostack`, `prese LL | asm!("", options(nomem, foo)); | ^^^ expected one of 8 possible tokens -error: asm options cannot be specified multiple times - --> $DIR/parse-error.rs:37:29 - | -LL | asm!("", options(), options()); - | --------- ^^^^^^^^^ duplicate options - | | - | previously here - -error: asm options cannot be specified multiple times - --> $DIR/parse-error.rs:39:29 - | -LL | asm!("", options(), options(), options()); - | --------- ^^^^^^^^^ duplicate options - | | - | previously here - -error: asm options cannot be specified multiple times - --> $DIR/parse-error.rs:39:40 - | -LL | asm!("", options(), options(), options()); - | --------- ^^^^^^^^^ duplicate options - | | - | previously here - error: arguments are not allowed after options - --> $DIR/parse-error.rs:42:31 + --> $DIR/parse-error.rs:37:31 | LL | asm!("{}", options(), const foo); | --------- ^^^^^^^^^ argument @@ -115,7 +91,7 @@ LL | asm!("{}", options(), const foo); | previous options error: duplicate argument named `a` - --> $DIR/parse-error.rs:44:36 + --> $DIR/parse-error.rs:39:36 | LL | asm!("{a}", a = const foo, a = const bar); | ------------- ^^^^^^^^^^^^^ duplicate argument @@ -123,7 +99,7 @@ LL | asm!("{a}", a = const foo, a = const bar); | previously here error: argument never used - --> $DIR/parse-error.rs:44:36 + --> $DIR/parse-error.rs:39:36 | LL | asm!("{a}", a = const foo, a = const bar); | ^^^^^^^^^^^^^ argument never used @@ -131,13 +107,13 @@ LL | asm!("{a}", a = const foo, a = const bar); = help: if this argument is intentionally unused, consider using it in an asm comment: `"/* {1} */"` error: explicit register arguments cannot have names - --> $DIR/parse-error.rs:47:18 + --> $DIR/parse-error.rs:42:18 | LL | asm!("", a = in("eax") foo); | ^^^^^^^^^^^^^^^^^ error: named arguments cannot follow explicit register arguments - --> $DIR/parse-error.rs:49:36 + --> $DIR/parse-error.rs:44:36 | LL | asm!("{a}", in("eax") foo, a = const bar); | ------------- ^^^^^^^^^^^^^ named argument @@ -145,7 +121,7 @@ LL | asm!("{a}", in("eax") foo, a = const bar); | explicit register argument error: named arguments cannot follow explicit register arguments - --> $DIR/parse-error.rs:51:36 + --> $DIR/parse-error.rs:46:36 | LL | asm!("{a}", in("eax") foo, a = const bar); | ------------- ^^^^^^^^^^^^^ named argument @@ -153,7 +129,7 @@ LL | asm!("{a}", in("eax") foo, a = const bar); | explicit register argument error: positional arguments cannot follow named arguments or explicit register arguments - --> $DIR/parse-error.rs:53:36 + --> $DIR/parse-error.rs:48:36 | LL | asm!("{1}", in("eax") foo, const bar); | ------------- ^^^^^^^^^ positional argument @@ -161,19 +137,19 @@ LL | asm!("{1}", in("eax") foo, const bar); | explicit register argument error: expected one of `const`, `in`, `inlateout`, `inout`, `lateout`, `options`, `out`, or `sym`, found `""` - --> $DIR/parse-error.rs:55:29 + --> $DIR/parse-error.rs:50:29 | LL | asm!("", options(), ""); | ^^ expected one of 8 possible tokens error: expected one of `const`, `in`, `inlateout`, `inout`, `lateout`, `options`, `out`, or `sym`, found `"{}"` - --> $DIR/parse-error.rs:57:33 + --> $DIR/parse-error.rs:52:33 | LL | asm!("{}", in(reg) foo, "{}", out(reg) foo); | ^^^^ expected one of 8 possible tokens error: asm template must be a string literal - --> $DIR/parse-error.rs:59:14 + --> $DIR/parse-error.rs:54:14 | LL | asm!(format!("{{{}}}", 0), in(reg) foo); | ^^^^^^^^^^^^^^^^^^^^ @@ -181,12 +157,12 @@ LL | asm!(format!("{{{}}}", 0), in(reg) foo); = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) error: asm template must be a string literal - --> $DIR/parse-error.rs:61:21 + --> $DIR/parse-error.rs:56:21 | LL | asm!("{1}", format!("{{{}}}", 0), in(reg) foo, out(reg) bar); | ^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 28 previous errors +error: aborting due to 25 previous errors From 1d2acdf8ec1959424be2728fa868e91b3eaeedc6 Mon Sep 17 00:00:00 2001 From: Camelid Date: Fri, 12 Jun 2020 11:31:41 -0700 Subject: [PATCH 03/20] Use `Vec` instead of `Option>` --- src/librustc_builtin_macros/asm.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index e98431a269591..8f6b8903d6676 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -16,7 +16,7 @@ struct AsmArgs { named_args: FxHashMap, reg_args: FxHashSet, options: ast::InlineAsmOptions, - options_spans: Option>, + options_spans: Vec, } fn parse_args<'a>( @@ -59,7 +59,7 @@ fn parse_args<'a>( named_args: FxHashMap::default(), reg_args: FxHashSet::default(), options: ast::InlineAsmOptions::empty(), - options_spans: None, + options_spans: vec![], }; let mut allow_templates = true; @@ -174,9 +174,9 @@ fn parse_args<'a>( // Validate the order of named, positional & explicit register operands and options. We do // this at the end once we have the full span of the argument available. - if let Some(ref options_spans) = args.options_spans { + if args.options_spans.len() > 0 { ecx.struct_span_err(span, "arguments are not allowed after options") - .span_labels(options_spans.clone(), "previous options") + .span_labels(args.options_spans.clone(), "previous options") .span_label(span, "argument") .emit(); } @@ -227,21 +227,21 @@ fn parse_args<'a>( if args.options.contains(ast::InlineAsmOptions::NOMEM) && args.options.contains(ast::InlineAsmOptions::READONLY) { - let spans = args.options_spans.clone().unwrap(); + let spans = args.options_spans.clone(); ecx.struct_span_err(spans, "the `nomem` and `readonly` options are mutually exclusive") .emit(); } if args.options.contains(ast::InlineAsmOptions::PURE) && args.options.contains(ast::InlineAsmOptions::NORETURN) { - let spans = args.options_spans.clone().unwrap(); + let spans = args.options_spans.clone(); ecx.struct_span_err(spans, "the `pure` and `noreturn` options are mutually exclusive") .emit(); } if args.options.contains(ast::InlineAsmOptions::PURE) && !args.options.intersects(ast::InlineAsmOptions::NOMEM | ast::InlineAsmOptions::READONLY) { - let span = args.options_spans.clone().unwrap(); + let span = args.options_spans.clone(); ecx.struct_span_err( span, "the `pure` option must be combined with either `nomem` or `readonly`", @@ -267,7 +267,7 @@ fn parse_args<'a>( } if args.options.contains(ast::InlineAsmOptions::PURE) && !have_real_output { ecx.struct_span_err( - args.options_spans.clone().unwrap(), + args.options_spans.clone(), "asm with `pure` option must have at least one output", ) .emit(); @@ -314,11 +314,7 @@ fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), Diagn } let new_span = span_start.to(p.prev_token.span); - if let Some(options_spans) = &mut args.options_spans { - options_spans.push(new_span); - } else { - args.options_spans = Some(vec![new_span]); - } + args.options_spans.push(new_span); Ok(()) } From 27cc7c7d9fb8a25cce9ab73ece034a9282406ce3 Mon Sep 17 00:00:00 2001 From: Camelid Date: Fri, 12 Jun 2020 11:39:52 -0700 Subject: [PATCH 04/20] Clean up --- src/librustc_builtin_macros/asm.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 8f6b8903d6676..2be45daef989c 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -174,7 +174,7 @@ fn parse_args<'a>( // Validate the order of named, positional & explicit register operands and options. We do // this at the end once we have the full span of the argument available. - if args.options_spans.len() > 0 { + if !args.options_spans.is_empty() { ecx.struct_span_err(span, "arguments are not allowed after options") .span_labels(args.options_spans.clone(), "previous options") .span_label(span, "argument") @@ -241,9 +241,9 @@ fn parse_args<'a>( if args.options.contains(ast::InlineAsmOptions::PURE) && !args.options.intersects(ast::InlineAsmOptions::NOMEM | ast::InlineAsmOptions::READONLY) { - let span = args.options_spans.clone(); + let spans = args.options_spans.clone(); ecx.struct_span_err( - span, + spans, "the `pure` option must be combined with either `nomem` or `readonly`", ) .emit(); From 820bba1c467aa5aa54d6db2869b15c643e5c350c Mon Sep 17 00:00:00 2001 From: Camelid Date: Fri, 12 Jun 2020 12:12:36 -0700 Subject: [PATCH 05/20] Add codegen test for multiple `asm!` options --- src/test/codegen/asm-multiple-options.rs | 53 ++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 src/test/codegen/asm-multiple-options.rs diff --git a/src/test/codegen/asm-multiple-options.rs b/src/test/codegen/asm-multiple-options.rs new file mode 100644 index 0000000000000..c702742bf1a63 --- /dev/null +++ b/src/test/codegen/asm-multiple-options.rs @@ -0,0 +1,53 @@ +// compile-flags: -O +// only-x86_64 + +#![crate_type = "rlib"] +#![feature(asm)] + +// CHECK-LABEL: @pure +// CHECK-NOT: asm +// CHECK: ret void +#[no_mangle] +pub unsafe fn pure(x: i32) { + let y: i32; + asm!("", out("ax") y, in("bx") x, options(pure), options(nomem)); +} + +pub static mut VAR: i32 = 0; +pub static mut DUMMY_OUTPUT: i32 = 0; + +// CHECK-LABEL: @readonly +// CHECK: call i32 asm +// CHECK: ret i32 1 +#[no_mangle] +pub unsafe fn readonly() -> i32 { + VAR = 1; + asm!("", out("ax") DUMMY_OUTPUT, options(pure), options(readonly)); + VAR +} + +// CHECK-LABEL: @nomem +// CHECK-NOT: store +// CHECK: call i32 asm +// CHECK: store +// CHECK: ret i32 2 +#[no_mangle] +pub unsafe fn nomem() -> i32 { + VAR = 1; + asm!("", out("ax") DUMMY_OUTPUT, options(pure), options(nomem)); + VAR = 2; + VAR +} + +// CHECK-LABEL: @not_nomem +// CHECK: store +// CHECK: call i32 asm +// CHECK: store +// CHECK: ret i32 2 +#[no_mangle] +pub unsafe fn not_nomem() -> i32 { + VAR = 1; + asm!("", out("ax") DUMMY_OUTPUT, options(pure), options(readonly)); + VAR = 2; + VAR +} From 2be403ce3e0bc21b36a6ef783476095d2e921fca Mon Sep 17 00:00:00 2001 From: Camelid Date: Sat, 13 Jun 2020 17:06:35 -0700 Subject: [PATCH 06/20] Warn on duplicate `asm!` options --- src/librustc_builtin_macros/asm.rs | 65 ++++++++++++++++++++++++++---- 1 file changed, 58 insertions(+), 7 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 2be45daef989c..b5641fdf2ab0f 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -19,6 +19,12 @@ struct AsmArgs { options_spans: Vec, } +impl AsmArgs { + fn option_is_set(&self, option: ast::InlineAsmOptions) -> bool { + (self.options & option) == option + } +} + fn parse_args<'a>( ecx: &mut ExtCtxt<'a>, sp: Span, @@ -283,6 +289,23 @@ fn parse_args<'a>( Ok(args) } +fn warn_duplicate_option<'a>(p: &mut Parser<'a>, span: Span) { + let mut warn = if let Ok(snippet) = p.sess.source_map().span_to_snippet(span) { + p.sess + .span_diagnostic + .struct_span_warn(span, &format!("the `{}` option was already provided", snippet)) + } else { + p.sess.span_diagnostic.struct_span_warn(span, "this option was already provided") + }; + warn.span_suggestion( + span, + "remove this option", + String::new(), + Applicability::MachineApplicable, + ); + warn.emit(); +} + fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), DiagnosticBuilder<'a>> { let span_start = p.prev_token.span; @@ -290,20 +313,48 @@ fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), Diagn while !p.eat(&token::CloseDelim(token::DelimToken::Paren)) { if p.eat(&token::Ident(sym::pure, false)) { - args.options |= ast::InlineAsmOptions::PURE; + if !args.option_is_set(ast::InlineAsmOptions::PURE) { + args.options |= ast::InlineAsmOptions::PURE; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else if p.eat(&token::Ident(sym::nomem, false)) { - args.options |= ast::InlineAsmOptions::NOMEM; + if !args.option_is_set(ast::InlineAsmOptions::NOMEM) { + args.options |= ast::InlineAsmOptions::NOMEM; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else if p.eat(&token::Ident(sym::readonly, false)) { - args.options |= ast::InlineAsmOptions::READONLY; + if !args.option_is_set(ast::InlineAsmOptions::READONLY) { + args.options |= ast::InlineAsmOptions::READONLY; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else if p.eat(&token::Ident(sym::preserves_flags, false)) { - args.options |= ast::InlineAsmOptions::PRESERVES_FLAGS; + if !args.option_is_set(ast::InlineAsmOptions::PRESERVES_FLAGS) { + args.options |= ast::InlineAsmOptions::PRESERVES_FLAGS; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else if p.eat(&token::Ident(sym::noreturn, false)) { - args.options |= ast::InlineAsmOptions::NORETURN; + if !args.option_is_set(ast::InlineAsmOptions::NORETURN) { + args.options |= ast::InlineAsmOptions::NORETURN; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else if p.eat(&token::Ident(sym::nostack, false)) { - args.options |= ast::InlineAsmOptions::NOSTACK; + if !args.option_is_set(ast::InlineAsmOptions::NOSTACK) { + args.options |= ast::InlineAsmOptions::NOSTACK; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } else { p.expect(&token::Ident(sym::att_syntax, false))?; - args.options |= ast::InlineAsmOptions::ATT_SYNTAX; + if !args.option_is_set(ast::InlineAsmOptions::ATT_SYNTAX) { + args.options |= ast::InlineAsmOptions::ATT_SYNTAX; + } else { + warn_duplicate_option(p, p.prev_token.span); + } } // Allow trailing commas From 7aaadb69e45134277e6091ec1ce173c0787cf896 Mon Sep 17 00:00:00 2001 From: Camelid Date: Sat, 13 Jun 2020 17:59:13 -0700 Subject: [PATCH 07/20] Add UI test for duplicate `asm!` options warning --- src/test/ui/asm/duplicate-options.rs | 19 ++++++++++++ src/test/ui/asm/duplicate-options.stderr | 38 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 src/test/ui/asm/duplicate-options.rs create mode 100644 src/test/ui/asm/duplicate-options.stderr diff --git a/src/test/ui/asm/duplicate-options.rs b/src/test/ui/asm/duplicate-options.rs new file mode 100644 index 0000000000000..9c447451511b4 --- /dev/null +++ b/src/test/ui/asm/duplicate-options.rs @@ -0,0 +1,19 @@ +// only-x86_64 +// build-pass + +#![feature(asm)] + +fn main() { + unsafe { + asm!("", options(nomem, nomem)); + //~^ WARNING the `nomem` option was already provided + asm!("", options(att_syntax, att_syntax)); + //~^ WARNING the `att_syntax` option was already provided + asm!("", options(nostack, att_syntax), options(nostack)); + //~^ WARNING the `nostack` option was already provided + asm!("", options(nostack, nostack), options(nostack), options(nostack)); + //~^ WARNING the `nostack` option was already provided + //~| WARNING the `nostack` option was already provided + //~| WARNING the `nostack` option was already provided + } +} diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr new file mode 100644 index 0000000000000..113aca8da900f --- /dev/null +++ b/src/test/ui/asm/duplicate-options.stderr @@ -0,0 +1,38 @@ +warning: the `nomem` option was already provided + --> $DIR/duplicate-options.rs:8:33 + | +LL | asm!("", options(nomem, nomem)); + | ^^^^^ help: remove this option + +warning: the `att_syntax` option was already provided + --> $DIR/duplicate-options.rs:10:38 + | +LL | asm!("", options(att_syntax, att_syntax)); + | ^^^^^^^^^^ help: remove this option + +warning: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:12:56 + | +LL | asm!("", options(nostack, att_syntax), options(nostack)); + | ^^^^^^^ help: remove this option + +warning: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:14:35 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ help: remove this option + +warning: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:14:53 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ help: remove this option + +warning: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:14:71 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ help: remove this option + +warning: 6 warnings emitted + From b94b7e7f1bf4e05d28ec7fca305c1d49f242c3dd Mon Sep 17 00:00:00 2001 From: Camelid Date: Sun, 14 Jun 2020 13:38:09 -0700 Subject: [PATCH 08/20] Make warning an error; use help instead of suggestion; clean up code 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. --- src/librustc_builtin_macros/asm.rs | 67 +++++++--------------- src/test/ui/asm/duplicate-options.rs | 19 ++++--- src/test/ui/asm/duplicate-options.stderr | 72 ++++++++++++++++++------ 3 files changed, 87 insertions(+), 71 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index b5641fdf2ab0f..922a06bae93e4 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -289,21 +289,24 @@ fn parse_args<'a>( Ok(args) } -fn warn_duplicate_option<'a>(p: &mut Parser<'a>, span: Span) { - let mut warn = if let Ok(snippet) = p.sess.source_map().span_to_snippet(span) { +fn err_duplicate_option<'a>(p: &mut Parser<'a>, span: Span) { + let mut err = if let Ok(snippet) = p.sess.source_map().span_to_snippet(span) { p.sess .span_diagnostic - .struct_span_warn(span, &format!("the `{}` option was already provided", snippet)) + .struct_span_err(span, &format!("the `{}` option was already provided", snippet)) } else { - p.sess.span_diagnostic.struct_span_warn(span, "this option was already provided") + p.sess.span_diagnostic.struct_span_err(span, "this option was already provided") }; - warn.span_suggestion( - span, - "remove this option", - String::new(), - Applicability::MachineApplicable, - ); - warn.emit(); + err.span_help(span, "remove this option"); + err.emit(); +} + +fn try_set_option<'a>(p: &mut Parser<'a>, args: &mut AsmArgs, option: ast::InlineAsmOptions) { + if !args.option_is_set(option) { + args.options |= option; + } else { + err_duplicate_option(p, p.prev_token.span); + } } fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), DiagnosticBuilder<'a>> { @@ -313,48 +316,20 @@ fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), Diagn while !p.eat(&token::CloseDelim(token::DelimToken::Paren)) { if p.eat(&token::Ident(sym::pure, false)) { - if !args.option_is_set(ast::InlineAsmOptions::PURE) { - args.options |= ast::InlineAsmOptions::PURE; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::PURE); } else if p.eat(&token::Ident(sym::nomem, false)) { - if !args.option_is_set(ast::InlineAsmOptions::NOMEM) { - args.options |= ast::InlineAsmOptions::NOMEM; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::NOMEM); } else if p.eat(&token::Ident(sym::readonly, false)) { - if !args.option_is_set(ast::InlineAsmOptions::READONLY) { - args.options |= ast::InlineAsmOptions::READONLY; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::READONLY); } else if p.eat(&token::Ident(sym::preserves_flags, false)) { - if !args.option_is_set(ast::InlineAsmOptions::PRESERVES_FLAGS) { - args.options |= ast::InlineAsmOptions::PRESERVES_FLAGS; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::PRESERVES_FLAGS); } else if p.eat(&token::Ident(sym::noreturn, false)) { - if !args.option_is_set(ast::InlineAsmOptions::NORETURN) { - args.options |= ast::InlineAsmOptions::NORETURN; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::NORETURN); } else if p.eat(&token::Ident(sym::nostack, false)) { - if !args.option_is_set(ast::InlineAsmOptions::NOSTACK) { - args.options |= ast::InlineAsmOptions::NOSTACK; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::NOSTACK); } else { p.expect(&token::Ident(sym::att_syntax, false))?; - if !args.option_is_set(ast::InlineAsmOptions::ATT_SYNTAX) { - args.options |= ast::InlineAsmOptions::ATT_SYNTAX; - } else { - warn_duplicate_option(p, p.prev_token.span); - } + try_set_option(p, args, ast::InlineAsmOptions::ATT_SYNTAX); } // Allow trailing commas diff --git a/src/test/ui/asm/duplicate-options.rs b/src/test/ui/asm/duplicate-options.rs index 9c447451511b4..e412932fa7ebe 100644 --- a/src/test/ui/asm/duplicate-options.rs +++ b/src/test/ui/asm/duplicate-options.rs @@ -1,19 +1,24 @@ // only-x86_64 -// build-pass #![feature(asm)] fn main() { unsafe { asm!("", options(nomem, nomem)); - //~^ WARNING the `nomem` option was already provided + //~^ ERROR the `nomem` option was already provided + //~| HELP remove this option asm!("", options(att_syntax, att_syntax)); - //~^ WARNING the `att_syntax` option was already provided + //~^ ERROR the `att_syntax` option was already provided + //~| HELP remove this option asm!("", options(nostack, att_syntax), options(nostack)); - //~^ WARNING the `nostack` option was already provided + //~^ ERROR the `nostack` option was already provided + //~| HELP remove this option asm!("", options(nostack, nostack), options(nostack), options(nostack)); - //~^ WARNING the `nostack` option was already provided - //~| WARNING the `nostack` option was already provided - //~| WARNING the `nostack` option was already provided + //~^ ERROR the `nostack` option was already provided + //~| HELP remove this option + //~| ERROR the `nostack` option was already provided + //~| HELP remove this option + //~| ERROR the `nostack` option was already provided + //~| HELP remove this option } } diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index 113aca8da900f..accfdef474bb9 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -1,38 +1,74 @@ -warning: the `nomem` option was already provided - --> $DIR/duplicate-options.rs:8:33 +error: the `nomem` option was already provided + --> $DIR/duplicate-options.rs:7:33 | LL | asm!("", options(nomem, nomem)); - | ^^^^^ help: remove this option + | ^^^^^ + | +help: remove this option + --> $DIR/duplicate-options.rs:7:33 + | +LL | asm!("", options(nomem, nomem)); + | ^^^^^ -warning: the `att_syntax` option was already provided +error: the `att_syntax` option was already provided + --> $DIR/duplicate-options.rs:10:38 + | +LL | asm!("", options(att_syntax, att_syntax)); + | ^^^^^^^^^^ + | +help: remove this option --> $DIR/duplicate-options.rs:10:38 | LL | asm!("", options(att_syntax, att_syntax)); - | ^^^^^^^^^^ help: remove this option + | ^^^^^^^^^^ -warning: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:12:56 +error: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:13:56 + | +LL | asm!("", options(nostack, att_syntax), options(nostack)); + | ^^^^^^^ + | +help: remove this option + --> $DIR/duplicate-options.rs:13:56 | LL | asm!("", options(nostack, att_syntax), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ -warning: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:14:35 +error: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:16:35 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ + | +help: remove this option + --> $DIR/duplicate-options.rs:16:35 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ -warning: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:14:53 +error: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:16:53 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ + | +help: remove this option + --> $DIR/duplicate-options.rs:16:53 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ -warning: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:14:71 +error: the `nostack` option was already provided + --> $DIR/duplicate-options.rs:16:71 + | +LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); + | ^^^^^^^ + | +help: remove this option + --> $DIR/duplicate-options.rs:16:71 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ -warning: 6 warnings emitted +error: aborting due to 6 previous errors From ac54265b137b307e5a369b9e8c78126784b8427b Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 15 Jun 2020 11:57:49 -0700 Subject: [PATCH 09/20] Use `span_label` --- src/librustc_builtin_macros/asm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 922a06bae93e4..474c54c998d99 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -297,7 +297,7 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, span: Span) { } else { p.sess.span_diagnostic.struct_span_err(span, "this option was already provided") }; - err.span_help(span, "remove this option"); + err.span_label(span, "remove this option"); err.emit(); } From 7c5b66f3287c95df7e1aaca1d2a0576eed69f7dd Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 15 Jun 2020 12:11:49 -0700 Subject: [PATCH 10/20] Update duplicate options test --- src/test/ui/asm/duplicate-options.rs | 6 --- src/test/ui/asm/duplicate-options.stderr | 58 +++++------------------- 2 files changed, 11 insertions(+), 53 deletions(-) diff --git a/src/test/ui/asm/duplicate-options.rs b/src/test/ui/asm/duplicate-options.rs index e412932fa7ebe..1df7f55f81248 100644 --- a/src/test/ui/asm/duplicate-options.rs +++ b/src/test/ui/asm/duplicate-options.rs @@ -6,19 +6,13 @@ fn main() { unsafe { asm!("", options(nomem, nomem)); //~^ ERROR the `nomem` option was already provided - //~| HELP remove this option asm!("", options(att_syntax, att_syntax)); //~^ ERROR the `att_syntax` option was already provided - //~| HELP remove this option asm!("", options(nostack, att_syntax), options(nostack)); //~^ ERROR the `nostack` option was already provided - //~| HELP remove this option asm!("", options(nostack, nostack), options(nostack), options(nostack)); //~^ ERROR the `nostack` option was already provided - //~| HELP remove this option //~| ERROR the `nostack` option was already provided - //~| HELP remove this option //~| ERROR the `nostack` option was already provided - //~| HELP remove this option } } diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index accfdef474bb9..6339ae9db8105 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -2,73 +2,37 @@ error: the `nomem` option was already provided --> $DIR/duplicate-options.rs:7:33 | LL | asm!("", options(nomem, nomem)); - | ^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:7:33 - | -LL | asm!("", options(nomem, nomem)); - | ^^^^^ + | ^^^^^ remove this option error: the `att_syntax` option was already provided - --> $DIR/duplicate-options.rs:10:38 - | -LL | asm!("", options(att_syntax, att_syntax)); - | ^^^^^^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:10:38 + --> $DIR/duplicate-options.rs:9:38 | LL | asm!("", options(att_syntax, att_syntax)); - | ^^^^^^^^^^ + | ^^^^^^^^^^ remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:13:56 - | -LL | asm!("", options(nostack, att_syntax), options(nostack)); - | ^^^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:13:56 + --> $DIR/duplicate-options.rs:11:56 | LL | asm!("", options(nostack, att_syntax), options(nostack)); - | ^^^^^^^ + | ^^^^^^^ remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:16:35 + --> $DIR/duplicate-options.rs:13:35 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:16:35 - | -LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ + | ^^^^^^^ remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:16:53 + --> $DIR/duplicate-options.rs:13:53 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:16:53 - | -LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ + | ^^^^^^^ remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:16:71 - | -LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ - | -help: remove this option - --> $DIR/duplicate-options.rs:16:71 + --> $DIR/duplicate-options.rs:13:71 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ + | ^^^^^^^ remove this option error: aborting due to 6 previous errors From c7da50d23f4fdcd2952a336d661373050730657b Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 15 Jun 2020 12:07:46 -0700 Subject: [PATCH 11/20] Get option name from symbol instead of snippet --- src/librustc_builtin_macros/asm.rs | 36 ++++++++++++++++-------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 474c54c998d99..5bb2408580290 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -289,23 +289,25 @@ fn parse_args<'a>( Ok(args) } -fn err_duplicate_option<'a>(p: &mut Parser<'a>, span: Span) { - let mut err = if let Ok(snippet) = p.sess.source_map().span_to_snippet(span) { - p.sess - .span_diagnostic - .struct_span_err(span, &format!("the `{}` option was already provided", snippet)) - } else { - p.sess.span_diagnostic.struct_span_err(span, "this option was already provided") - }; +fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { + let mut err = p + .sess + .span_diagnostic + .struct_span_err(span, &format!("the `{}` option was already provided", symbol)); err.span_label(span, "remove this option"); err.emit(); } -fn try_set_option<'a>(p: &mut Parser<'a>, args: &mut AsmArgs, option: ast::InlineAsmOptions) { +fn try_set_option<'a>( + p: &mut Parser<'a>, + args: &mut AsmArgs, + symbol: Symbol, + option: ast::InlineAsmOptions, +) { if !args.option_is_set(option) { args.options |= option; } else { - err_duplicate_option(p, p.prev_token.span); + err_duplicate_option(p, symbol, p.prev_token.span); } } @@ -316,20 +318,20 @@ fn parse_options<'a>(p: &mut Parser<'a>, args: &mut AsmArgs) -> Result<(), Diagn while !p.eat(&token::CloseDelim(token::DelimToken::Paren)) { if p.eat(&token::Ident(sym::pure, false)) { - try_set_option(p, args, ast::InlineAsmOptions::PURE); + try_set_option(p, args, sym::pure, ast::InlineAsmOptions::PURE); } else if p.eat(&token::Ident(sym::nomem, false)) { - try_set_option(p, args, ast::InlineAsmOptions::NOMEM); + try_set_option(p, args, sym::nomem, ast::InlineAsmOptions::NOMEM); } else if p.eat(&token::Ident(sym::readonly, false)) { - try_set_option(p, args, ast::InlineAsmOptions::READONLY); + try_set_option(p, args, sym::readonly, ast::InlineAsmOptions::READONLY); } else if p.eat(&token::Ident(sym::preserves_flags, false)) { - try_set_option(p, args, ast::InlineAsmOptions::PRESERVES_FLAGS); + try_set_option(p, args, sym::preserves_flags, ast::InlineAsmOptions::PRESERVES_FLAGS); } else if p.eat(&token::Ident(sym::noreturn, false)) { - try_set_option(p, args, ast::InlineAsmOptions::NORETURN); + try_set_option(p, args, sym::noreturn, ast::InlineAsmOptions::NORETURN); } else if p.eat(&token::Ident(sym::nostack, false)) { - try_set_option(p, args, ast::InlineAsmOptions::NOSTACK); + try_set_option(p, args, sym::nostack, ast::InlineAsmOptions::NOSTACK); } else { p.expect(&token::Ident(sym::att_syntax, false))?; - try_set_option(p, args, ast::InlineAsmOptions::ATT_SYNTAX); + try_set_option(p, args, sym::att_syntax, ast::InlineAsmOptions::ATT_SYNTAX); } // Allow trailing commas From e8be7971d1217312499c1258a1bb337fcdf3afa6 Mon Sep 17 00:00:00 2001 From: Camelid Date: Mon, 15 Jun 2020 13:30:13 -0700 Subject: [PATCH 12/20] Use bitflags function instead of custom one --- src/librustc_builtin_macros/asm.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 5bb2408580290..db4fae606ad34 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -19,12 +19,6 @@ struct AsmArgs { options_spans: Vec, } -impl AsmArgs { - fn option_is_set(&self, option: ast::InlineAsmOptions) -> bool { - (self.options & option) == option - } -} - fn parse_args<'a>( ecx: &mut ExtCtxt<'a>, sp: Span, @@ -304,7 +298,7 @@ fn try_set_option<'a>( symbol: Symbol, option: ast::InlineAsmOptions, ) { - if !args.option_is_set(option) { + if !args.options.contains(option) { args.options |= option; } else { err_duplicate_option(p, symbol, p.prev_token.span); From b00b1a45982a9f44d3339eab2142dc5f7278f812 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 16 Jun 2020 11:54:12 -0700 Subject: [PATCH 13/20] Use `span_suggestion` instead of `span_label` --- src/librustc_builtin_macros/asm.rs | 2 +- src/test/ui/asm/duplicate-options.stderr | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index db4fae606ad34..675cc904a9f04 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -288,7 +288,7 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { .sess .span_diagnostic .struct_span_err(span, &format!("the `{}` option was already provided", symbol)); - err.span_label(span, "remove this option"); + err.span_suggestion(span, "remove this option", String::new(), Applicability::Unspecified); err.emit(); } diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index 6339ae9db8105..356e27df19e58 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -2,37 +2,37 @@ error: the `nomem` option was already provided --> $DIR/duplicate-options.rs:7:33 | LL | asm!("", options(nomem, nomem)); - | ^^^^^ remove this option + | ^^^^^ help: remove this option error: the `att_syntax` option was already provided --> $DIR/duplicate-options.rs:9:38 | LL | asm!("", options(att_syntax, att_syntax)); - | ^^^^^^^^^^ remove this option + | ^^^^^^^^^^ help: remove this option error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:11:56 | LL | asm!("", options(nostack, att_syntax), options(nostack)); - | ^^^^^^^ remove this option + | ^^^^^^^ help: remove this option error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:13:35 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ remove this option + | ^^^^^^^ help: remove this option error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:13:53 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ remove this option + | ^^^^^^^ help: remove this option error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:13:71 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ remove this option + | ^^^^^^^ help: remove this option error: aborting due to 6 previous errors From f4dfc61e847c5a587aaf09635643034ec900ace8 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 16 Jun 2020 12:03:19 -0700 Subject: [PATCH 14/20] Add more to duplicate options test --- src/test/ui/asm/duplicate-options.rs | 7 +++++++ src/test/ui/asm/duplicate-options.stderr | 20 +++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/test/ui/asm/duplicate-options.rs b/src/test/ui/asm/duplicate-options.rs index 1df7f55f81248..34371b854f8e2 100644 --- a/src/test/ui/asm/duplicate-options.rs +++ b/src/test/ui/asm/duplicate-options.rs @@ -14,5 +14,12 @@ fn main() { //~^ ERROR the `nostack` option was already provided //~| ERROR the `nostack` option was already provided //~| ERROR the `nostack` option was already provided + asm!( + "", + options(nomem, noreturn), + options(att_syntax, noreturn), //~ ERROR the `noreturn` option was already provided + options(nomem, nostack), //~ ERROR the `nomem` option was already provided + options(noreturn), //~ ERROR the `noreturn` option was already provided + ); } } diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index 356e27df19e58..9aa699c4e76e8 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -34,5 +34,23 @@ error: the `nostack` option was already provided LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); | ^^^^^^^ help: remove this option -error: aborting due to 6 previous errors +error: the `noreturn` option was already provided + --> $DIR/duplicate-options.rs:20:33 + | +LL | options(att_syntax, noreturn), + | ^^^^^^^^ help: remove this option + +error: the `nomem` option was already provided + --> $DIR/duplicate-options.rs:21:21 + | +LL | options(nomem, nostack), + | ^^^^^ help: remove this option + +error: the `noreturn` option was already provided + --> $DIR/duplicate-options.rs:22:21 + | +LL | options(noreturn), + | ^^^^^^^^ help: remove this option + +error: aborting due to 9 previous errors From 4ba66970d87684fa412a6475bf68ed6042c9e1e9 Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 16 Jun 2020 12:32:13 -0700 Subject: [PATCH 15/20] Make suggestion machine-applicable --- src/librustc_builtin_macros/asm.rs | 13 ++++++++++-- src/test/ui/asm/duplicate-options.fixed | 26 ++++++++++++++++++++++++ src/test/ui/asm/duplicate-options.rs | 1 + src/test/ui/asm/duplicate-options.stderr | 20 +++++++++--------- 4 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 src/test/ui/asm/duplicate-options.fixed diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 675cc904a9f04..7e5eca29aeada 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -288,7 +288,12 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { .sess .span_diagnostic .struct_span_err(span, &format!("the `{}` option was already provided", symbol)); - err.span_suggestion(span, "remove this option", String::new(), Applicability::Unspecified); + err.span_suggestion( + span, + "remove this option", + String::new(), + Applicability::MachineApplicable, + ); err.emit(); } @@ -301,7 +306,11 @@ fn try_set_option<'a>( if !args.options.contains(option) { args.options |= option; } else { - err_duplicate_option(p, symbol, p.prev_token.span); + let mut span = p.prev_token.span; + if p.look_ahead(0, |t| t == &token::Comma) { + span = span.to(p.token.span); + } + err_duplicate_option(p, symbol, span); } } diff --git a/src/test/ui/asm/duplicate-options.fixed b/src/test/ui/asm/duplicate-options.fixed new file mode 100644 index 0000000000000..f4672a50fd0f4 --- /dev/null +++ b/src/test/ui/asm/duplicate-options.fixed @@ -0,0 +1,26 @@ +// only-x86_64 +// run-rustfix + +#![feature(asm)] + +fn main() { + unsafe { + asm!("", options(nomem, )); + //~^ ERROR the `nomem` option was already provided + asm!("", options(att_syntax, )); + //~^ ERROR the `att_syntax` option was already provided + asm!("", options(nostack, att_syntax), options()); + //~^ ERROR the `nostack` option was already provided + asm!("", options(nostack, ), options(), options()); + //~^ ERROR the `nostack` option was already provided + //~| ERROR the `nostack` option was already provided + //~| ERROR the `nostack` option was already provided + asm!( + "", + options(nomem, noreturn), + options(att_syntax, ), //~ ERROR the `noreturn` option was already provided + options( nostack), //~ ERROR the `nomem` option was already provided + options(), //~ ERROR the `noreturn` option was already provided + ); + } +} diff --git a/src/test/ui/asm/duplicate-options.rs b/src/test/ui/asm/duplicate-options.rs index 34371b854f8e2..80292d7521a9a 100644 --- a/src/test/ui/asm/duplicate-options.rs +++ b/src/test/ui/asm/duplicate-options.rs @@ -1,4 +1,5 @@ // only-x86_64 +// run-rustfix #![feature(asm)] diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index 9aa699c4e76e8..46a966ddc8606 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -1,53 +1,53 @@ error: the `nomem` option was already provided - --> $DIR/duplicate-options.rs:7:33 + --> $DIR/duplicate-options.rs:8:33 | LL | asm!("", options(nomem, nomem)); | ^^^^^ help: remove this option error: the `att_syntax` option was already provided - --> $DIR/duplicate-options.rs:9:38 + --> $DIR/duplicate-options.rs:10:38 | LL | asm!("", options(att_syntax, att_syntax)); | ^^^^^^^^^^ help: remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:11:56 + --> $DIR/duplicate-options.rs:12:56 | LL | asm!("", options(nostack, att_syntax), options(nostack)); | ^^^^^^^ help: remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:13:35 + --> $DIR/duplicate-options.rs:14:35 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); | ^^^^^^^ help: remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:13:53 + --> $DIR/duplicate-options.rs:14:53 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); | ^^^^^^^ help: remove this option error: the `nostack` option was already provided - --> $DIR/duplicate-options.rs:13:71 + --> $DIR/duplicate-options.rs:14:71 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); | ^^^^^^^ help: remove this option error: the `noreturn` option was already provided - --> $DIR/duplicate-options.rs:20:33 + --> $DIR/duplicate-options.rs:21:33 | LL | options(att_syntax, noreturn), | ^^^^^^^^ help: remove this option error: the `nomem` option was already provided - --> $DIR/duplicate-options.rs:21:21 + --> $DIR/duplicate-options.rs:22:21 | LL | options(nomem, nostack), - | ^^^^^ help: remove this option + | ^^^^^^ help: remove this option error: the `noreturn` option was already provided - --> $DIR/duplicate-options.rs:22:21 + --> $DIR/duplicate-options.rs:23:21 | LL | options(noreturn), | ^^^^^^^^ help: remove this option From 8fe6710b4db48e60f49c0d237d324d5490dc4f8e Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 16 Jun 2020 12:43:03 -0700 Subject: [PATCH 16/20] Create a separate, tool-only suggestion for the comma 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. --- src/librustc_builtin_macros/asm.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 7e5eca29aeada..1d59029333db0 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -294,6 +294,14 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { String::new(), Applicability::MachineApplicable, ); + if p.look_ahead(0, |t| t == &token::Comma) { + err.tool_only_span_suggestion( + p.token.span, + "remove this comma", + String::new(), + Applicability::MachineApplicable, + ); + } err.emit(); } @@ -306,11 +314,7 @@ fn try_set_option<'a>( if !args.options.contains(option) { args.options |= option; } else { - let mut span = p.prev_token.span; - if p.look_ahead(0, |t| t == &token::Comma) { - span = span.to(p.token.span); - } - err_duplicate_option(p, symbol, span); + err_duplicate_option(p, symbol, p.prev_token.span); } } From db9d3769b4f26b45b390d350ed3319433cd5e42c Mon Sep 17 00:00:00 2001 From: Camelid Date: Tue, 16 Jun 2020 18:10:26 -0700 Subject: [PATCH 17/20] Add documentation --- src/librustc_builtin_macros/asm.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 1d59029333db0..9c8e148f83a33 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -283,6 +283,10 @@ fn parse_args<'a>( Ok(args) } +/// Report a duplicate option error. +/// +/// This function must be called immediately after the option token is parsed. +/// Otherwise, the suggestion will be incorrect. fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { let mut err = p .sess @@ -305,6 +309,11 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { err.emit(); } +/// Try to set the provided option in the provided `AsmArgs`. +/// If it is already set, report a duplicate option error. +/// +/// This function must be called immediately after the option token is parsed. +/// Otherwise, the error will not point to the correct spot. fn try_set_option<'a>( p: &mut Parser<'a>, args: &mut AsmArgs, From 58f812bd610c214d5b0ea9493ae362f451378239 Mon Sep 17 00:00:00 2001 From: Camelid Date: Wed, 17 Jun 2020 19:02:57 -0700 Subject: [PATCH 18/20] Use `p.token` instead of `p.look_ahead()` --- src/librustc_builtin_macros/asm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 9c8e148f83a33..119536da05024 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -298,7 +298,7 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { String::new(), Applicability::MachineApplicable, ); - if p.look_ahead(0, |t| t == &token::Comma) { + if p.token.kind == token::Comma { err.tool_only_span_suggestion( p.token.span, "remove this comma", From 8d80cc56794e43f5c6e89550dc6ac64101e278fa Mon Sep 17 00:00:00 2001 From: Camelid Date: Fri, 19 Jun 2020 20:18:32 -0700 Subject: [PATCH 19/20] Fix duplicate options error The UI isn't glitching anymore. --- src/librustc_builtin_macros/asm.rs | 21 ++++++++++++--------- src/test/ui/asm/duplicate-options.stderr | 18 +++++++++--------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 119536da05024..6fa04c279d269 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -292,20 +292,23 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { .sess .span_diagnostic .struct_span_err(span, &format!("the `{}` option was already provided", symbol)); - err.span_suggestion( + err.span_label( span, + "this option was already provided", + ); + + // Tool-only output + let mut full_span = span; + if p.token.kind == token::Comma { + full_span = full_span.to(p.token.span); + } + err.tool_only_span_suggestion( + full_span, "remove this option", String::new(), Applicability::MachineApplicable, ); - if p.token.kind == token::Comma { - err.tool_only_span_suggestion( - p.token.span, - "remove this comma", - String::new(), - Applicability::MachineApplicable, - ); - } + err.emit(); } diff --git a/src/test/ui/asm/duplicate-options.stderr b/src/test/ui/asm/duplicate-options.stderr index 46a966ddc8606..cd8d743e031a5 100644 --- a/src/test/ui/asm/duplicate-options.stderr +++ b/src/test/ui/asm/duplicate-options.stderr @@ -2,55 +2,55 @@ error: the `nomem` option was already provided --> $DIR/duplicate-options.rs:8:33 | LL | asm!("", options(nomem, nomem)); - | ^^^^^ help: remove this option + | ^^^^^ this option was already provided error: the `att_syntax` option was already provided --> $DIR/duplicate-options.rs:10:38 | LL | asm!("", options(att_syntax, att_syntax)); - | ^^^^^^^^^^ help: remove this option + | ^^^^^^^^^^ this option was already provided error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:12:56 | LL | asm!("", options(nostack, att_syntax), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ this option was already provided error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:14:35 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ this option was already provided error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:14:53 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ this option was already provided error: the `nostack` option was already provided --> $DIR/duplicate-options.rs:14:71 | LL | asm!("", options(nostack, nostack), options(nostack), options(nostack)); - | ^^^^^^^ help: remove this option + | ^^^^^^^ this option was already provided error: the `noreturn` option was already provided --> $DIR/duplicate-options.rs:21:33 | LL | options(att_syntax, noreturn), - | ^^^^^^^^ help: remove this option + | ^^^^^^^^ this option was already provided error: the `nomem` option was already provided --> $DIR/duplicate-options.rs:22:21 | LL | options(nomem, nostack), - | ^^^^^^ help: remove this option + | ^^^^^ this option was already provided error: the `noreturn` option was already provided --> $DIR/duplicate-options.rs:23:21 | LL | options(noreturn), - | ^^^^^^^^ help: remove this option + | ^^^^^^^^ this option was already provided error: aborting due to 9 previous errors From c31785a4ed13f1a1bab752ea3c1177f7256e4f11 Mon Sep 17 00:00:00 2001 From: Camelid Date: Fri, 19 Jun 2020 20:25:36 -0700 Subject: [PATCH 20/20] Run `./x.py fmt` --- src/librustc_builtin_macros/asm.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/librustc_builtin_macros/asm.rs b/src/librustc_builtin_macros/asm.rs index 6fa04c279d269..52f86aa7e06b9 100644 --- a/src/librustc_builtin_macros/asm.rs +++ b/src/librustc_builtin_macros/asm.rs @@ -292,10 +292,7 @@ fn err_duplicate_option<'a>(p: &mut Parser<'a>, symbol: Symbol, span: Span) { .sess .span_diagnostic .struct_span_err(span, &format!("the `{}` option was already provided", symbol)); - err.span_label( - span, - "this option was already provided", - ); + err.span_label(span, "this option was already provided"); // Tool-only output let mut full_span = span;