From a53c5b3caa18eba40518e1d6a9a3f5423ce1cf09 Mon Sep 17 00:00:00 2001 From: Sean Chalmers Date: Sat, 25 Jan 2014 20:31:27 +0100 Subject: [PATCH 1/3] Extern Function Declarations Type Check #10877 Functions declared in an 'extern' were not properly typed checked and will report syntax errors on failure instead of an ICE. Fixes #10877 --- src/libsyntax/parse/parser.rs | 40 ++++++++++++-------- src/test/compile-fail/extern-bad-fn-decls.rs | 25 ++++++++++++ 2 files changed, 50 insertions(+), 15 deletions(-) create mode 100644 src/test/compile-fail/extern-bad-fn-decls.rs diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index 557e7e04ebfc5..be11baa812386 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -954,7 +954,7 @@ impl Parser { self.expect_or(); let inputs = self.parse_seq_to_before_or( &token::COMMA, - |p| p.parse_arg_general(false)); + |p| p.parse_arg_general(false, false)); self.expect_or(); inputs }; @@ -1021,7 +1021,7 @@ impl Parser { opt_vec::Empty }; - let (inputs, variadic) = self.parse_fn_args(false, allow_variadic); + let (inputs, variadic) = self.parse_fn_args(false, allow_variadic, false); let (ret_style, ret_ty) = self.parse_ret_ty(); let decl = P(FnDecl { inputs: inputs, @@ -1054,7 +1054,7 @@ impl Parser { let (explicit_self, d) = p.parse_fn_decl_with_self(|p| { // This is somewhat dubious; We don't want to allow argument // names to be left off if there is a definition... - p.parse_arg_general(false) + p.parse_arg_general(false, false) }); let hi = p.last_span.hi; @@ -1336,16 +1336,19 @@ impl Parser { // This version of parse arg doesn't necessarily require // identifier names. - pub fn parse_arg_general(&mut self, require_name: bool) -> Arg { - let pat = if require_name || self.is_named_argument() { - debug!("parse_arg_general parse_pat (require_name:{:?})", - require_name); + pub fn parse_arg_general(&mut self, require_name: bool, + require_ident_patterns_only: bool) -> Arg { + let pat = if !require_ident_patterns_only && require_name || self.is_named_argument() { + debug!("parse_arg_general parse_pat (require_name:{:?}) + ident_patterns_only (require_ident_patterns_only:{:?}", + require_name, + require_ident_patterns_only); let pat = self.parse_pat(); self.expect(&token::COLON); pat } else { - debug!("parse_arg_general ident_to_pat"); + debug!("parse_arg_general ident_to_pat require_ident_patterns_only"); ast_util::ident_to_pat(ast::DUMMY_NODE_ID, self.last_span, special_idents::invalid) @@ -1362,7 +1365,7 @@ impl Parser { // parse a single function argument pub fn parse_arg(&mut self) -> Arg { - self.parse_arg_general(true) + self.parse_arg_general(true, false) } // parse an argument in a lambda header e.g. |arg, arg| @@ -3567,7 +3570,8 @@ impl Parser { (lifetimes, opt_vec::take_vec(result)) } - fn parse_fn_args(&mut self, named_args: bool, allow_variadic: bool) + fn parse_fn_args(&mut self, named_args: bool, allow_variadic: bool, + require_ident_patterns_only: bool) -> (~[Arg], bool) { let sp = self.span; let mut args: ~[Option] = @@ -3589,7 +3593,7 @@ impl Parser { } None } else { - Some(p.parse_arg_general(named_args)) + Some(p.parse_arg_general(named_args, require_ident_patterns_only)) } } ); @@ -3615,9 +3619,12 @@ impl Parser { } // parse the argument list and result type of a function declaration - pub fn parse_fn_decl(&mut self, allow_variadic: bool) -> P { + pub fn parse_fn_decl(&mut self, allow_variadic: bool, + require_ident_patterns_only: bool) -> P { - let (args, variadic) = self.parse_fn_args(true, allow_variadic); + let (args, variadic) = self.parse_fn_args(true, + allow_variadic, + require_ident_patterns_only); let (ret_style, ret_ty) = self.parse_ret_ty(); P(FnDecl { @@ -3882,7 +3889,7 @@ impl Parser { // parse an item-position function declaration. fn parse_item_fn(&mut self, purity: Purity, abis: AbiSet) -> ItemInfo { let (ident, generics) = self.parse_fn_header(); - let decl = self.parse_fn_decl(false); + let decl = self.parse_fn_decl(false, false); let (inner_attrs, body) = self.parse_inner_attrs_and_block(); (ident, ItemFn(decl, purity, abis, generics, body), Some(inner_attrs)) } @@ -4320,7 +4327,10 @@ impl Parser { } let (ident, generics) = self.parse_fn_header(); - let decl = self.parse_fn_decl(true); + // ForeignItemFn definitions args: + // First arg: + // Second arg: require_ident_patterns_only (valid fns with ident only) + let decl = self.parse_fn_decl(true, true); let hi = self.span.hi; self.expect(&token::SEMI); @ast::ForeignItem { ident: ident, diff --git a/src/test/compile-fail/extern-bad-fn-decls.rs b/src/test/compile-fail/extern-bad-fn-decls.rs new file mode 100644 index 0000000000000..ca96286a05b16 --- /dev/null +++ b/src/test/compile-fail/extern-bad-fn-decls.rs @@ -0,0 +1,25 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// error-pattern: expected + +struct Foo { x: int } + +extern { + fn external_fn_one(1: ()); + + fn external_fn_two((): int); + + fn external_fn_three(Foo {x}: int); + + fn external_fn_four((x,y): int); +} + +fn main() {} \ No newline at end of file From 04b3772c0fbc1e1a65ed6dc480e3047d4ebd04b1 Mon Sep 17 00:00:00 2001 From: Sean Chalmers Date: Mon, 27 Jan 2014 12:22:33 +0100 Subject: [PATCH 2/3] Rework ident check. Changed how I was checking for the ident as the previous method did not account for the right patterns. This update ensures the extern fn fail on the patterns expected, based on the test cases I have in: src/test/compile-fail/extern-bad-fn-decls.rs The changes to intrinsics.rs and variadic-ffi.rs are because in extern definitions the use of '_' is not allowed, based on my understanding of the issue. I'm in need of help past this point as my tests are failing by the errors that are being reported are what I expect. Something else is going on but I'm at a loss as to where to even start looking. Hopefully this change is a little more up to scratch! :) --- src/libstd/unstable/intrinsics.rs | 2 +- src/libsyntax/parse/parser.rs | 20 ++++++++++++++++++-- src/test/compile-fail/extern-bad-fn-decls.rs | 17 +++++++++++------ src/test/compile-fail/variadic-ffi.rs | 2 +- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/libstd/unstable/intrinsics.rs b/src/libstd/unstable/intrinsics.rs index 198df3090ee80..451390d0f9962 100644 --- a/src/libstd/unstable/intrinsics.rs +++ b/src/libstd/unstable/intrinsics.rs @@ -323,7 +323,7 @@ extern "rust-intrinsic" { /// /// `forget` is unsafe because the caller is responsible for /// ensuring the argument is deallocated already. - pub fn forget(_: T) -> (); + pub fn forget(e: T) -> (); pub fn transmute(e: T) -> U; /// Returns `true` if a type requires drop glue. diff --git a/src/libsyntax/parse/parser.rs b/src/libsyntax/parse/parser.rs index be11baa812386..c2255616c14b8 100644 --- a/src/libsyntax/parse/parser.rs +++ b/src/libsyntax/parse/parser.rs @@ -1338,14 +1338,30 @@ impl Parser { // identifier names. pub fn parse_arg_general(&mut self, require_name: bool, require_ident_patterns_only: bool) -> Arg { - let pat = if !require_ident_patterns_only && require_name || self.is_named_argument() { + let pat = if require_name || self.is_named_argument() { debug!("parse_arg_general parse_pat (require_name:{:?}) ident_patterns_only (require_ident_patterns_only:{:?}", require_name, require_ident_patterns_only); - let pat = self.parse_pat(); + + let pat = if require_ident_patterns_only { + // issue #10877 + // Only identifiers are allowed for this type of fn decl, no + // pattern matching or destructuring. + let pat_ident = PatIdent(BindByValue(MutImmutable), + self.parse_path(NoTypesAllowed).path, + None); + @ast::Pat { + id: ast::DUMMY_NODE_ID, + span: self.last_span, + node: pat_ident + } + } else { + self.parse_pat() + }; self.expect(&token::COLON); + pat } else { debug!("parse_arg_general ident_to_pat require_ident_patterns_only"); diff --git a/src/test/compile-fail/extern-bad-fn-decls.rs b/src/test/compile-fail/extern-bad-fn-decls.rs index ca96286a05b16..111e9dc80b6e0 100644 --- a/src/test/compile-fail/extern-bad-fn-decls.rs +++ b/src/test/compile-fail/extern-bad-fn-decls.rs @@ -8,18 +8,23 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// error-pattern: expected +// Concerning issue #10877 struct Foo { x: int } extern { - fn external_fn_one(1: ()); - fn external_fn_two((): int); + fn external_fn_one(1: ()); //~ ERROR expected ident, found `1` - fn external_fn_three(Foo {x}: int); + fn external_fn_two((): int); //~ ERROR expected ident, found `(` - fn external_fn_four((x,y): int); + fn external_fn_three(Foo {x}: int); //~ ERROR expected `:`, found `{` + + fn external_fn_four((x,y): int); //~ ERROR expected ident, found `(` + + fn external_fn_five(_: int); //~ ERROR expected ident, found `_` + + fn external_fn_six(~int); //~ ERROR expected ident, found `~` } -fn main() {} \ No newline at end of file +fn main() {} diff --git a/src/test/compile-fail/variadic-ffi.rs b/src/test/compile-fail/variadic-ffi.rs index 3156cd9af3276..28615641187a6 100644 --- a/src/test/compile-fail/variadic-ffi.rs +++ b/src/test/compile-fail/variadic-ffi.rs @@ -9,7 +9,7 @@ // except according to those terms. extern "stdcall" { - fn printf(_: *u8, ...); //~ ERROR: variadic function must have C calling convention + fn printf(f: *u8, ...); //~ ERROR: variadic function must have C calling convention } extern { From 3d65bb907f4172062a35392248dc517b5b18e7b6 Mon Sep 17 00:00:00 2001 From: Sean Chalmers Date: Tue, 28 Jan 2014 22:30:48 +0100 Subject: [PATCH 3/3] Rebuilt Tests. Previous method of testing these failures reworked to break them out into separate files to ensure proper coverage, and that the tests actually perform as desired. Removed old test file. --- .../extern-fn-type-only-in-ident-position.rs | 17 +++++++++++++++++ .../extern-ignore-in-ident-position.rs | 17 +++++++++++++++++ .../extern-literal-in-ident-position.rs | 17 +++++++++++++++++ .../extern-nil-type-in-ident-position.rs | 17 +++++++++++++++++ .../extern-struct-destr-in-ident-position.rs | 19 +++++++++++++++++++ ...> extern-tuple-destr-in-ident-position.rs} | 13 ------------- 6 files changed, 87 insertions(+), 13 deletions(-) create mode 100644 src/test/compile-fail/extern-fn-type-only-in-ident-position.rs create mode 100644 src/test/compile-fail/extern-ignore-in-ident-position.rs create mode 100644 src/test/compile-fail/extern-literal-in-ident-position.rs create mode 100644 src/test/compile-fail/extern-nil-type-in-ident-position.rs create mode 100644 src/test/compile-fail/extern-struct-destr-in-ident-position.rs rename src/test/compile-fail/{extern-bad-fn-decls.rs => extern-tuple-destr-in-ident-position.rs} (61%) diff --git a/src/test/compile-fail/extern-fn-type-only-in-ident-position.rs b/src/test/compile-fail/extern-fn-type-only-in-ident-position.rs new file mode 100644 index 0000000000000..c3b4429112f76 --- /dev/null +++ b/src/test/compile-fail/extern-fn-type-only-in-ident-position.rs @@ -0,0 +1,17 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Concerning issue #10877 + +extern { + fn external_fn_six(~int); //~ ERROR expected ident, found `~` +} + +fn main() {} diff --git a/src/test/compile-fail/extern-ignore-in-ident-position.rs b/src/test/compile-fail/extern-ignore-in-ident-position.rs new file mode 100644 index 0000000000000..800a7ee7e7721 --- /dev/null +++ b/src/test/compile-fail/extern-ignore-in-ident-position.rs @@ -0,0 +1,17 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Concerning issue #10877 + +extern { + fn external_fn_five(_: int); //~ ERROR expected ident, found `_` +} + +fn main() {} diff --git a/src/test/compile-fail/extern-literal-in-ident-position.rs b/src/test/compile-fail/extern-literal-in-ident-position.rs new file mode 100644 index 0000000000000..ab22aff3eac85 --- /dev/null +++ b/src/test/compile-fail/extern-literal-in-ident-position.rs @@ -0,0 +1,17 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Concerning issue #10877 + +extern { + fn external_fn_one(1: ()); //~ ERROR expected ident, found `1` +} + +fn main() {} diff --git a/src/test/compile-fail/extern-nil-type-in-ident-position.rs b/src/test/compile-fail/extern-nil-type-in-ident-position.rs new file mode 100644 index 0000000000000..f4ad1ba0d4384 --- /dev/null +++ b/src/test/compile-fail/extern-nil-type-in-ident-position.rs @@ -0,0 +1,17 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Concerning issue #10877 + +extern { + fn external_fn_two((): int); //~ ERROR expected ident, found `(` +} + +fn main() {} diff --git a/src/test/compile-fail/extern-struct-destr-in-ident-position.rs b/src/test/compile-fail/extern-struct-destr-in-ident-position.rs new file mode 100644 index 0000000000000..552f22e227d7a --- /dev/null +++ b/src/test/compile-fail/extern-struct-destr-in-ident-position.rs @@ -0,0 +1,19 @@ +// Copyright 2012 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Concerning issue #10877 + +struct Foo { x: int } + +extern { + fn external_fn_three(Foo {x}: int); //~ ERROR expected `:` but found `{` +} + +fn main() {} diff --git a/src/test/compile-fail/extern-bad-fn-decls.rs b/src/test/compile-fail/extern-tuple-destr-in-ident-position.rs similarity index 61% rename from src/test/compile-fail/extern-bad-fn-decls.rs rename to src/test/compile-fail/extern-tuple-destr-in-ident-position.rs index 111e9dc80b6e0..049754cbf45b2 100644 --- a/src/test/compile-fail/extern-bad-fn-decls.rs +++ b/src/test/compile-fail/extern-tuple-destr-in-ident-position.rs @@ -10,21 +10,8 @@ // Concerning issue #10877 -struct Foo { x: int } - extern { - - fn external_fn_one(1: ()); //~ ERROR expected ident, found `1` - - fn external_fn_two((): int); //~ ERROR expected ident, found `(` - - fn external_fn_three(Foo {x}: int); //~ ERROR expected `:`, found `{` - fn external_fn_four((x,y): int); //~ ERROR expected ident, found `(` - - fn external_fn_five(_: int); //~ ERROR expected ident, found `_` - - fn external_fn_six(~int); //~ ERROR expected ident, found `~` } fn main() {}