Skip to content

Commit

Permalink
Rollup merge of rust-lang#68764 - Centril:self-semantic, r=petrochenkov
Browse files Browse the repository at this point in the history
parser: syntactically allow `self` in all `fn` contexts

Part of rust-lang#68728.

`self` parameters are now *syntactically* allowed as the first parameter irrespective of item context (and in function pointers). Instead, semantic validation (`ast_validation`) is used.

r? @petrochenkov
  • Loading branch information
Centril authored Feb 2, 2020
2 parents 2e1790d + 71a6f58 commit 5951cd3
Show file tree
Hide file tree
Showing 20 changed files with 453 additions and 87 deletions.
44 changes: 36 additions & 8 deletions src/librustc_ast_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ use syntax::expand::is_proc_macro_attr;
use syntax::visit::{self, Visitor};
use syntax::walk_list;

/// Is `self` allowed semantically as the first parameter in an `FnDecl`?
enum SelfSemantic {
Yes,
No,
}

/// A syntactic context that disallows certain kinds of bounds (e.g., `?Trait` or `?const Trait`).
#[derive(Clone, Copy)]
enum BoundContext {
Expand Down Expand Up @@ -302,7 +308,13 @@ impl<'a> AstValidator<'a> {
}
}

fn check_fn_decl(&self, fn_decl: &FnDecl) {
fn check_fn_decl(&self, fn_decl: &FnDecl, self_semantic: SelfSemantic) {
self.check_decl_cvaradic_pos(fn_decl);
self.check_decl_attrs(fn_decl);
self.check_decl_self_param(fn_decl, self_semantic);
}

fn check_decl_cvaradic_pos(&self, fn_decl: &FnDecl) {
match &*fn_decl.inputs {
[Param { ty, span, .. }] => {
if let TyKind::CVarArgs = ty.kind {
Expand All @@ -324,7 +336,9 @@ impl<'a> AstValidator<'a> {
}
_ => {}
}
}

fn check_decl_attrs(&self, fn_decl: &FnDecl) {
fn_decl
.inputs
.iter()
Expand Down Expand Up @@ -352,6 +366,21 @@ impl<'a> AstValidator<'a> {
});
}

fn check_decl_self_param(&self, fn_decl: &FnDecl, self_semantic: SelfSemantic) {
if let (SelfSemantic::No, [param, ..]) = (self_semantic, &*fn_decl.inputs) {
if param.is_self() {
self.err_handler()
.struct_span_err(
param.span,
"`self` parameter is only allowed in associated functions",
)
.span_label(param.span, "not semantically valid as function parameter")
.note("associated functions are those in `impl` or `trait` definitions")
.emit();
}
}
}

fn check_defaultness(&self, span: Span, defaultness: Defaultness) {
if let Defaultness::Default = defaultness {
self.err_handler()
Expand Down Expand Up @@ -504,7 +533,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_expr(&mut self, expr: &'a Expr) {
match &expr.kind {
ExprKind::Closure(_, _, _, fn_decl, _, _) => {
self.check_fn_decl(fn_decl);
self.check_fn_decl(fn_decl, SelfSemantic::No);
}
ExprKind::InlineAsm(..) if !self.session.target.target.options.allow_asm => {
struct_span_err!(
Expand All @@ -524,7 +553,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_ty(&mut self, ty: &'a Ty) {
match ty.kind {
TyKind::BareFn(ref bfty) => {
self.check_fn_decl(&bfty.decl);
self.check_fn_decl(&bfty.decl, SelfSemantic::No);
Self::check_decl_no_pat(&bfty.decl, |span, _| {
struct_span_err!(
self.session,
Expand Down Expand Up @@ -685,7 +714,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
ItemKind::Fn(ref sig, ref generics, _) => {
self.visit_fn_header(&sig.header);
self.check_fn_decl(&sig.decl);
self.check_fn_decl(&sig.decl, SelfSemantic::No);
// We currently do not permit const generics in `const fn`, as
// this is tantamount to allowing compile-time dependent typing.
if sig.header.constness.node == Constness::Const {
Expand Down Expand Up @@ -793,7 +822,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
fn visit_foreign_item(&mut self, fi: &'a ForeignItem) {
match fi.kind {
ForeignItemKind::Fn(ref decl, _) => {
self.check_fn_decl(decl);
self.check_fn_decl(decl, SelfSemantic::No);
Self::check_decl_no_pat(decl, |span, _| {
struct_span_err!(
self.session,
Expand Down Expand Up @@ -987,9 +1016,8 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
AssocItemKind::Const(_, body) => {
self.check_impl_item_provided(ii.span, body, "constant", " = <expr>;");
}
AssocItemKind::Fn(sig, body) => {
AssocItemKind::Fn(_, body) => {
self.check_impl_item_provided(ii.span, body, "function", " { <body> }");
self.check_fn_decl(&sig.decl);
}
AssocItemKind::TyAlias(bounds, body) => {
self.check_impl_item_provided(ii.span, body, "type", " = <type>;");
Expand All @@ -1005,7 +1033,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
self.check_defaultness(ti.span, ti.defaultness);

if let AssocItemKind::Fn(sig, block) = &ti.kind {
self.check_fn_decl(&sig.decl);
self.check_trait_fn_not_async(ti.span, sig.header.asyncness.node);
self.check_trait_fn_not_const(sig.header.constness);
if block.is_none() {
Expand Down Expand Up @@ -1035,6 +1062,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {

fn visit_assoc_item(&mut self, item: &'a AssocItem) {
if let AssocItemKind::Fn(sig, _) = &item.kind {
self.check_fn_decl(&sig.decl, SelfSemantic::Yes);
self.check_c_varadic_type(&sig.decl);
}
visit::walk_assoc_item(self, item);
Expand Down
25 changes: 7 additions & 18 deletions src/librustc_parse/parser/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1336,8 +1336,7 @@ impl<'a> Parser<'a> {
err: &mut DiagnosticBuilder<'_>,
pat: P<ast::Pat>,
require_name: bool,
is_self_allowed: bool,
is_trait_item: bool,
first_param: bool,
) -> Option<Ident> {
// If we find a pattern followed by an identifier, it could be an (incorrect)
// C-style parameter declaration.
Expand All @@ -1357,13 +1356,12 @@ impl<'a> Parser<'a> {
return Some(ident);
} else if let PatKind::Ident(_, ident, _) = pat.kind {
if require_name
&& (is_trait_item
|| self.token == token::Comma
&& (self.token == token::Comma
|| self.token == token::Lt
|| self.token == token::CloseDelim(token::Paren))
{
// `fn foo(a, b) {}`, `fn foo(a<x>, b<y>) {}` or `fn foo(usize, usize) {}`
if is_self_allowed {
if first_param {
err.span_suggestion(
pat.span,
"if this is a `self` type, give it a parameter name",
Expand Down Expand Up @@ -1420,21 +1418,12 @@ impl<'a> Parser<'a> {
Ok((pat, ty))
}

pub(super) fn recover_bad_self_param(
&mut self,
mut param: ast::Param,
is_trait_item: bool,
) -> PResult<'a, ast::Param> {
pub(super) fn recover_bad_self_param(&mut self, mut param: Param) -> PResult<'a, Param> {
let sp = param.pat.span;
param.ty.kind = TyKind::Err;
let mut err = self.struct_span_err(sp, "unexpected `self` parameter in function");
if is_trait_item {
err.span_label(sp, "must be the first associated function parameter");
} else {
err.span_label(sp, "not valid as function parameter");
err.note("`self` is only valid as the first parameter of an associated function");
}
err.emit();
self.struct_span_err(sp, "unexpected `self` parameter in function")
.span_label(sp, "must be the first parameter of an associated function")
.emit();
Ok(param)
}

Expand Down
64 changes: 22 additions & 42 deletions src/librustc_parse/parser/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1715,8 +1715,6 @@ impl<'a> Parser<'a> {

/// The parsing configuration used to parse a parameter list (see `parse_fn_params`).
pub(super) struct ParamCfg {
/// Is `self` is allowed as the first parameter?
pub is_self_allowed: bool,
/// `is_name_required` decides if, per-parameter,
/// the parameter must have a pattern or just a type.
pub is_name_required: fn(&token::Token) -> bool,
Expand All @@ -1732,8 +1730,8 @@ impl<'a> Parser<'a> {
attrs: Vec<Attribute>,
header: FnHeader,
) -> PResult<'a, Option<P<Item>>> {
let (ident, decl, generics) =
self.parse_fn_sig(ParamCfg { is_self_allowed: false, is_name_required: |_| true })?;
let cfg = ParamCfg { is_name_required: |_| true };
let (ident, decl, generics) = self.parse_fn_sig(&cfg)?;
let (inner_attrs, body) = self.parse_inner_attrs_and_block()?;
let kind = ItemKind::Fn(FnSig { decl, header }, generics, body);
self.mk_item_with_info(attrs, lo, vis, (ident, kind, Some(inner_attrs)))
Expand All @@ -1747,20 +1745,13 @@ impl<'a> Parser<'a> {
attrs: Vec<Attribute>,
extern_sp: Span,
) -> PResult<'a, P<ForeignItem>> {
let cfg = ParamCfg { is_name_required: |_| true };
self.expect_keyword(kw::Fn)?;
let (ident, decl, generics) =
self.parse_fn_sig(ParamCfg { is_self_allowed: false, is_name_required: |_| true })?;
let (ident, decl, generics) = self.parse_fn_sig(&cfg)?;
let span = lo.to(self.token.span);
self.parse_semi_or_incorrect_foreign_fn_body(&ident, extern_sp)?;
Ok(P(ast::ForeignItem {
ident,
attrs,
kind: ForeignItemKind::Fn(decl, generics),
id: DUMMY_NODE_ID,
span,
vis,
tokens: None,
}))
let kind = ForeignItemKind::Fn(decl, generics);
Ok(P(ast::ForeignItem { ident, attrs, kind, id: DUMMY_NODE_ID, span, vis, tokens: None }))
}

fn parse_assoc_fn(
Expand All @@ -1770,8 +1761,7 @@ impl<'a> Parser<'a> {
is_name_required: fn(&token::Token) -> bool,
) -> PResult<'a, (Ident, AssocItemKind, Generics)> {
let header = self.parse_fn_front_matter()?;
let (ident, decl, generics) =
self.parse_fn_sig(ParamCfg { is_self_allowed: true, is_name_required })?;
let (ident, decl, generics) = self.parse_fn_sig(&ParamCfg { is_name_required })?;
let sig = FnSig { header, decl };
let body = self.parse_assoc_fn_body(at_end, attrs)?;
Ok((ident, AssocItemKind::Fn(sig, body), generics))
Expand Down Expand Up @@ -1847,7 +1837,7 @@ impl<'a> Parser<'a> {
}

/// Parse the "signature", including the identifier, parameters, and generics of a function.
fn parse_fn_sig(&mut self, cfg: ParamCfg) -> PResult<'a, (Ident, P<FnDecl>, Generics)> {
fn parse_fn_sig(&mut self, cfg: &ParamCfg) -> PResult<'a, (Ident, P<FnDecl>, Generics)> {
let ident = self.parse_ident()?;
let mut generics = self.parse_generics()?;
let decl = self.parse_fn_decl(cfg, true)?;
Expand All @@ -1858,7 +1848,7 @@ impl<'a> Parser<'a> {
/// Parses the parameter list and result type of a function declaration.
pub(super) fn parse_fn_decl(
&mut self,
cfg: ParamCfg,
cfg: &ParamCfg,
ret_allow_plus: bool,
) -> PResult<'a, P<FnDecl>> {
Ok(P(FnDecl {
Expand All @@ -1868,11 +1858,11 @@ impl<'a> Parser<'a> {
}

/// Parses the parameter list of a function, including the `(` and `)` delimiters.
fn parse_fn_params(&mut self, mut cfg: ParamCfg) -> PResult<'a, Vec<Param>> {
let is_trait_item = cfg.is_self_allowed;
// Parse the arguments, starting out with `self` being possibly allowed...
fn parse_fn_params(&mut self, cfg: &ParamCfg) -> PResult<'a, Vec<Param>> {
let mut first_param = true;
// Parse the arguments, starting out with `self` being allowed...
let (mut params, _) = self.parse_paren_comma_seq(|p| {
let param = p.parse_param_general(&cfg, is_trait_item).or_else(|mut e| {
let param = p.parse_param_general(&cfg, first_param).or_else(|mut e| {
e.emit();
let lo = p.prev_span;
// Skip every token until next possible arg or end.
Expand All @@ -1881,29 +1871,25 @@ impl<'a> Parser<'a> {
Ok(dummy_arg(Ident::new(kw::Invalid, lo.to(p.prev_span))))
});
// ...now that we've parsed the first argument, `self` is no longer allowed.
cfg.is_self_allowed = false;
first_param = false;
param
})?;
// Replace duplicated recovered params with `_` pattern to avoid unnecessary errors.
self.deduplicate_recovered_params_names(&mut params);
Ok(params)
}

/// Skips unexpected attributes and doc comments in this position and emits an appropriate
/// error.
/// This version of parse param doesn't necessarily require identifier names.
fn parse_param_general(&mut self, cfg: &ParamCfg, is_trait_item: bool) -> PResult<'a, Param> {
/// Parses a single function parameter.
///
/// - `self` is syntactically allowed when `first_param` holds.
fn parse_param_general(&mut self, cfg: &ParamCfg, first_param: bool) -> PResult<'a, Param> {
let lo = self.token.span;
let attrs = self.parse_outer_attributes()?;

// Possibly parse `self`. Recover if we parsed it and it wasn't allowed here.
if let Some(mut param) = self.parse_self_param()? {
param.attrs = attrs.into();
return if cfg.is_self_allowed {
Ok(param)
} else {
self.recover_bad_self_param(param, is_trait_item)
};
return if first_param { Ok(param) } else { self.recover_bad_self_param(param) };
}

let is_name_required = match self.token.kind {
Expand All @@ -1915,13 +1901,9 @@ impl<'a> Parser<'a> {

let pat = self.parse_fn_param_pat()?;
if let Err(mut err) = self.expect(&token::Colon) {
return if let Some(ident) = self.parameter_without_type(
&mut err,
pat,
is_name_required,
cfg.is_self_allowed,
is_trait_item,
) {
return if let Some(ident) =
self.parameter_without_type(&mut err, pat, is_name_required, first_param)
{
err.emit();
Ok(dummy_arg(ident))
} else {
Expand Down Expand Up @@ -1975,8 +1957,6 @@ impl<'a> Parser<'a> {
}

/// Returns the parsed optional self parameter and whether a self shortcut was used.
///
/// See `parse_self_param_with_attrs` to collect attributes.
fn parse_self_param(&mut self) -> PResult<'a, Option<Param>> {
// Extract an identifier *after* having confirmed that the token is one.
let expect_self_ident = |this: &mut Self| {
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_parse/parser/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,7 @@ impl<'a> Parser<'a> {
let unsafety = self.parse_unsafety();
let ext = self.parse_extern()?;
self.expect_keyword(kw::Fn)?;
let cfg = ParamCfg { is_self_allowed: false, is_name_required: |_| false };
let decl = self.parse_fn_decl(cfg, false)?;
let decl = self.parse_fn_decl(&ParamCfg { is_name_required: |_| false }, false)?;
Ok(TyKind::BareFn(P(BareFnTy { ext, unsafety, generic_params, decl })))
}

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/invalid-self-argument/bare-fn-start.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
fn a(&self) { }
//~^ ERROR unexpected `self` parameter in function
//~| NOTE not valid as function parameter
//~| NOTE `self` is only valid as the first parameter of an associated function
//~^ ERROR `self` parameter is only allowed in associated functions
//~| NOTE not semantically valid as function parameter
//~| NOTE associated functions are those in `impl` or `trait` definitions

fn main() { }
6 changes: 3 additions & 3 deletions src/test/ui/invalid-self-argument/bare-fn-start.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error: unexpected `self` parameter in function
error: `self` parameter is only allowed in associated functions
--> $DIR/bare-fn-start.rs:1:6
|
LL | fn a(&self) { }
| ^^^^^ not valid as function parameter
| ^^^^^ not semantically valid as function parameter
|
= note: `self` is only valid as the first parameter of an associated function
= note: associated functions are those in `impl` or `trait` definitions

error: aborting due to previous error

3 changes: 1 addition & 2 deletions src/test/ui/invalid-self-argument/bare-fn.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
fn b(foo: u32, &mut self) { }
//~^ ERROR unexpected `self` parameter in function
//~| NOTE not valid as function parameter
//~| NOTE `self` is only valid as the first parameter of an associated function
//~| NOTE must be the first parameter of an associated function

fn main() { }
4 changes: 1 addition & 3 deletions src/test/ui/invalid-self-argument/bare-fn.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ error: unexpected `self` parameter in function
--> $DIR/bare-fn.rs:1:16
|
LL | fn b(foo: u32, &mut self) { }
| ^^^^^^^^^ not valid as function parameter
|
= note: `self` is only valid as the first parameter of an associated function
| ^^^^^^^^^ must be the first parameter of an associated function

error: aborting due to previous error

2 changes: 1 addition & 1 deletion src/test/ui/invalid-self-argument/trait-fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ struct Foo {}
impl Foo {
fn c(foo: u32, self) {}
//~^ ERROR unexpected `self` parameter in function
//~| NOTE must be the first associated function parameter
//~| NOTE must be the first parameter of an associated function

fn good(&mut self, foo: u32) {}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/invalid-self-argument/trait-fn.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: unexpected `self` parameter in function
--> $DIR/trait-fn.rs:4:20
|
LL | fn c(foo: u32, self) {}
| ^^^^ must be the first associated function parameter
| ^^^^ must be the first parameter of an associated function

error: aborting due to previous error

Loading

0 comments on commit 5951cd3

Please sign in to comment.