Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

an empty :vis doesn't work in trait declaration #65041

Closed
limira opened this issue Oct 3, 2019 · 6 comments · Fixed by #66183
Closed

an empty :vis doesn't work in trait declaration #65041

limira opened this issue Oct 3, 2019 · 6 comments · Fixed by #66183
Assignees
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST A-visibility Area: Visibility / privacy C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@limira
Copy link
Contributor

limira commented Oct 3, 2019

I want to create a macro that generate fns that use in both impl Item... and trait TraitName. In trait, pub must be omitted, but in impl SomeStruct I need pub, so the macro need a $vis:vis like this:

macro_rules! create_method {
    ($vis:vis $name:ident) => {
        $vis fn $name(&self) {
            println!("{}", stringify!($name));
        }
    }
}

trait T1 {
    // This not works
    create_method!(method_of_t1);
}

trait T2 {
    fn method_of_t2(&self);
}

struct Struct;

impl T2 for Struct {
    // This works
    create_method!(method_of_t2);
}

impl Struct {
    // I want this method to be `pub`, so the macro need a `:vis`
    // for receiving the visibility keyword like this:
    create_method!(pub another_method);
}

Expected result: create_method!(method_of_t1); in trait T1 should works.

(Edit: a better comment about the :vis in impl Struct)

@petrochenkov
Copy link
Contributor

@Centril
This looks relevant to #64910 (comment) and the parser work you've been doing recently.

@Centril
Copy link
Contributor

Centril commented Oct 3, 2019

    // The macro need a `:vis` to generate `fn` for a normal `impl`

This bit doesn't seem right; when I remove the pub it works (which is expected because the impl T2 for Struct { works).

Looking at fn parse_trait_item_ there seems to be something odd going on. In particular we have:

  • self.eat_bad_pub();

  • } else if let Some(mac) = self.parse_assoc_macro_invoc("trait", None, &mut false)? {

    (note the None here -- for fn parse_impl_method we instead have:

    if let Some(mac) = self.parse_assoc_macro_invoc("impl", Some(vis), at_end)? {

    This suggests this is somehow by design? Odd...!

@Centril
Copy link
Contributor

Centril commented Oct 3, 2019

...actually it's not that odd. This code was likely written before we had the vis matcher.

I think what we want to do is to use let vis = self.parse_visibility(false)?; inparse_trait_item_ as well and then do a check right after to see if the visibility is not "empty" (VisibilityKind::Inherited) -- that would probably fix the issue here and allow us to discuss semantic vs. syntactic errors for pub fn foo in trait defs later.

@jonas-schievink jonas-schievink added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST A-visibility Area: Visibility / privacy C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2019
@Centril Centril added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated and removed C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 3, 2019
@limira
Copy link
Contributor Author

limira commented Oct 3, 2019

This bit doesn't seem right;

Sorry for the confusion caused by the comment. I edit the post, hope it is better now!

@nikomatsakis nikomatsakis changed the title an empty :vis not works in trait declaration an empty :vis doesn't work in trait declaration Oct 3, 2019
@Centril Centril self-assigned this Oct 3, 2019
@Centril
Copy link
Contributor

Centril commented Oct 3, 2019

We discussed this on the language team meeting today. The conclusion was that we regarded the issue here as a sort of bug that should be fixed (which I will do soon according to my notes above).

We also took the opportunity to discuss the grand unification of the 3 item grammars that @petrochenkov raised in #64910 (comment). We found the idea of "grand unification" and moving to semantic errors to be generally appealing as well.

@Centril
Copy link
Contributor

Centril commented Nov 7, 2019

Finally got around to this. I have a fix in #66183.

@bors bors closed this as completed in 8cba0a9 Nov 23, 2019
Centril added a commit to Centril/rust that referenced this issue Dec 20, 2019
Merge `TraitItem` & `ImplItem into `AssocItem`

In this PR we:

- Merge `{Trait,Impl}Item{Kind?}` into `AssocItem{Kind?}` as discussed in rust-lang#65041 (comment).

   - This is done by using the cover grammar of both forms.

   - In particular, it requires that we syntactically allow (under `#[cfg(FALSE)]`):

      - `default`ness on `trait` items,

      - `impl` items without a body / definition (`const`, `type`, and `fn`),

      - and associated `type`s in `impl`s with bounds, e.g., `type Foo: Ord;`.

   - The syntactic restrictions are replaced by semantic ones in `ast_validation`.

- Move syntactic restrictions around C-variadic parameters from the parser into `ast_validation`:

    - `fn`s in all contexts now syntactically allow `...`,

    - `...` can occur anywhere in the list syntactically (`fn foo(..., x: usize) {}`),

    - and `...` can be the sole parameter (`fn foo(...) {}`.

r? @petrochenkov
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 13, 2020
…enkov

parse: merge `fn` syntax + cleanup item parsing

Here we continue the work in rust-lang#67131 in particular to merge the grammars of `fn` items in various positions.

A list of *language level* changes (as sanctioned by the language team in rust-lang#65041 (comment) and rust-lang#67131):

- `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.

- Syntactically, `fn` items in `extern { ... }` blocks can now have bodies (`fn foo() { ... }` as opposed to `fn foo();`). As above, we use semantic restrictions instead.

- Syntactically, `fn` items in free contexts (directly in a file or a module) can now be without bodies (`fn foo();` as opposed to `fn foo() { ... }`. As above, we use semantic restrictions instead, including for non-ident parameter patterns.

- `const extern fn` feature gating is now done post-expansion such that we do not have conditional compatibilities of function qualifiers *in parsing*.

- The `FnFrontMatter` grammar becomes:
   ```rust
   Extern = "extern" StringLit ;
   FnQual = "const"? "async"? "unsafe"? Extern? ;
   FnFrontMatter = FnQual "fn" ;
   ```

   That is, all item contexts now *syntactically* allow `const async unsafe extern "C" fn` and use semantic restrictions to rule out combinations previously prevented syntactically. The semantic restrictions include in particular:

   - `fn`s in `extern { ... }` can have no qualifiers.
   - `const` and `async` cannot be combined.

- To fuse the list-of-items parsing in the 4 contexts that items are allowed, we now must permit inner attributes (`#![attr]`) inside `trait Foo { ... }` definitions. That is, we now allow e.g. `trait Foo { #![attr] }`. This was probably an oversight due to not using a uniform parsing mechanism, which we now do have (`fn parse_item_list`). The semantic support (including e.g. for linting) falls out directly from the attributes infrastructure. To ensure this, we include a test for lints.

Put together, these grammar changes allow us to substantially reduce the complexity of item parsing and its grammar. There are however some other non-language improvements that allow the compression to take place.

A list of *compiler-internal* changes (in particular noting the parser-external data-structure changes):

- We use `enum AllowPlus/RecoverQPath/AllowCVariadic { Yes, No }` in `parser/ty.rs` instead of passing around 3 different `bool`s. I felt this was necessary as it was becoming mentally taxing to track which-is-which.

- `fn visit_trait_item` and `fn visit_impl_item` are merged into `fn visit_assoc_item` which now is passed an `AssocCtxt` to check which one it is.

- We change `FnKind` to:

  ```rust
  pub enum FnKind<'a> {
      Fn(FnCtxt, Ident, &'a FnSig, &'a Visibility, Option<&'a Block>),
      Closure(&'a FnDecl, &'a Expr),
  }
  ```

  with:

  ```rust
  pub enum FnCtxt {
      Free,
      Foreign,
      Assoc(AssocCtxt),
  }
  ```

  This is then taken advantage of in tweaking the various semantic restrictions as well as in pretty printing.

- In `ItemKind::Fn`, we change `P<Block>` to `Option<P<Block>>`.

- In `ForeignItemKind::Fn`, we change `P<FnDecl>` to `FnSig` and `P<Block>` to `Option<P<Block>>`.

- We change `ast::{Unsafety, Spanned<Constness>}>` into `enum ast::{Unsafe, Const} { Yes(Span), No }` respectively. This change in formulation allow us to exclude `Span` in the case of `No`, which facilitates parsing. Moreover, we also add a `Span` to `IsAsync` which is renamed to `Async`. The new `Span`s in `Unsafety` and `Async` are then taken advantage of for better diagnostics. A reason this change was made is to have a more uniform and clear naming scheme.

  The HIR keeps the structures in AST (with those definitions moved into HIR) for now to avoid regressing perf.

- Various cleanups, bug fixes, and diagnostics improvements are made along the way. It is probably best to understand those via the diffs.

I would recommend reviewing this commit-by-commit with whitespace changes hidden.

r? @estebank @petrochenkov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-parser Area: The parsing of Rust source code to an AST A-visibility Area: Visibility / privacy C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants