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

Macros: Add a 'literal' fragment specifier #49835

Merged
merged 1 commit into from
May 13, 2018

Conversation

da-x
Copy link
Member

@da-x da-x commented Apr 10, 2018

See: #35625

macro_rules! test_literal {
    ($l:literal) => {
        println!("literal: {}", $l);
    };
    ($e:expr) => {
        println!("expr: {}", $e);
    };
}

fn main() {
    let a = 1;
    test_literal!(a);
    test_literal!(2);
    test_literal!(-3);
}

Output:

expr: 1
literal: 2
literal: -3

ToDo:

  • Feature gate
  • Basic tests
  • Tests for range patterns
  • Tests for attributes
  • Documentation
  • Fix for true/false
  • Fix for negative number literals

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2018
@TimNN
Copy link
Contributor

TimNN commented Apr 10, 2018

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
Resolving deltas: 100% (613503/613503), completed with 4872 local objects.
---
[00:00:46] configure: rust.quiet-tests     := True
---
[00:43:58] ..............................................................................i.....................
[00:44:04] .....................i..............................................................................
---
[00:44:39] ..................................................F.................................................
[00:44:46] i.........................................................................i.........................
[00:44:52] ....................................................................................................
[00:45:00] ....................................................................................................
[00:45:08] ....................................................................................................
known-linux-gnu/test/ui" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macro-invalid-fragment-spec.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Zmiri" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/macro-invalid-fragment-spec.stage2-x86_64-unknown-linux-gnu.aux" "-A" "unused"
---
[00:45:09] {"message":"invalid fragment specifier `foo`","code":null,"level":"error","spans":[{"file_name":"/checkout/src/test/ui/macro-invalid-fragment-spec.rs","byte_start":490,"byte_end":496,"line_start":12,"line_end":12,"column_start":6,"column_end":12,"is_primary":true,"text":[{"text":"    ($x:foo) => ()","highlight_start":6,"highlight_end":12}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `literal`, `path`, `meta`, `tt`, `item` and `vis`","code":null,"level":"help","spans":[],"children":[],"rendered":null}],"rendered":"error: invalid fragment specifier `foo`\n  --> /checkout/src/test/ui/macro-invalid-fragment-spec.rs:12:6\n   |\nLL |     ($x:foo) => ()\n   |      ^^^^^^\n   |\n   = help: valid fragment specifiers are `ident`, `block`, `stmt`, `expr`, `pat`, `ty`, `literal`, `path`, `meta`, `tt` "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:45:09] expected success, got: exit code: 101
[00:45:09]
[00:45:09]
[00:45:09] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:45:09] Build completed unsuccessfully in 0:02:28
[00:45:09] make: *** [check] Error 1
[00:45:09] Makefile:58: recipe for target 'check' failed
---
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
ls: cannot access /home/travis/Library/Logs/DiagnosticReports/: No such file or directory

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Apr 15, 2018
@petrochenkov
Copy link
Contributor

Could you add tests where literals of various kinds (integral, string, boolean) are used in expressions, patterns (including range patterns LIT ... LIT) and attributes (#[attr = LIT]).
After that we can discuss supporting -.
Docs are not mandatory, but they may be added into src/doc/unstable-book/src/language-features.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2018
@petrochenkov
Copy link
Contributor

This also needs to be feature gated, see b284419 for an example.

@pietroalbini
Copy link
Member

Ping from triage @da-x! Will you have some time to work on this in the future?

@da-x
Copy link
Member Author

da-x commented Apr 23, 2018

@pietroalbini - Yes - I am planning to add tests, feature gating, and documentation. Hopefully soon!

@bors
Copy link
Contributor

bors commented Apr 28, 2018

☔ The latest upstream changes (presumably #50155) made this pull request unmergeable. Please resolve the merge conflicts.

@da-x
Copy link
Member Author

da-x commented Apr 29, 2018

Added tests, feature gate, and docs.

Testing revealed that true and false are not recognized as literals. I'm working to fix it.

}

macro_rules! match_attr {
(#[$attr:meta] $e:literal) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant literal inside the attribute produced by the macro, e.g. something like

macro_rules! match_attr {
    ($e: literal) => {
        // Struct with doc comment passed via $literal
        #[doc = $literal]
        struct S;
    }
}

}

macro_rules! test_user {
($s:literal, $e:literal) => {
Copy link
Contributor

@petrochenkov petrochenkov Apr 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a case checking that $lit1 ... $lit2 is parsed as a range pattern in patterns?

}

macro_rules! catch_range {
($s:literal ... $e:literal) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to attributes, the range should rather be produced by a macro (1833a1f#r184891034), this test with ... on the left side of the macro is testing something entirely different and probably not useful.

@bors
Copy link
Contributor

bors commented May 1, 2018

☔ The latest upstream changes (presumably #49789) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

Ping from triage, @da-x ! You have review comments and merge conflicts to address!

@da-x
Copy link
Member Author

da-x commented May 6, 2018

Yes, will be on it!

@da-x
Copy link
Member Author

da-x commented May 8, 2018

@petrochenkov Tried to address your comments. Thanks.

@da-x
Copy link
Member Author

da-x commented May 9, 2018

Also fixed for true/false to pass as literals rather than expressions.

match *self {
Literal(..) => true,
Ident(ident, false) if ident.name == "true" => true,
Ident(ident, false) if ident.name == "false" => true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: keywords::True.name()/keywords::False.name() instead of "true"/"false".

@petrochenkov
Copy link
Contributor

@da-x
Do you prefer to add support for - in this PR or in a separate one?
It will need changing ast::Lit in NtLiteral into ast::Expr and replacing the parse_lit logic for "literal" in macro_parser with something like fn parse_pat_literal_maybe_minus currently used in patterns.

@petrochenkov petrochenkov removed the C-enhancement Category: An issue proposing an enhancement or a PR with one. label May 10, 2018
@da-x
Copy link
Member Author

da-x commented May 11, 2018

I've added a fix for negative literals. Thanks for the help.

@@ -310,6 +315,14 @@ impl Token {
}
}

/// Returns true if this token can begin a two-token literal (i.e. negative numbers)
pub fn can_begin_literal(&self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you merge this with is_lit_or_bool?

"ident" | "lifetime" => {
// being a single token, idents and lifetimes are harmless
"ident" | "lifetime" | "literal" => {
// being a single token, idents, literals and lifetimes are harmless
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, literals are no longer necessarily single-token, but it shouldn't change anything with regards to follow set.

@@ -156,6 +156,10 @@ pub trait Folder : Sized {
noop_fold_ident(i, self)
}

fn fold_lit(&mut self, i: P<Expr>) -> P<Expr> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fold_lit and noop_fold_lit are no longer necessary.

@petrochenkov
Copy link
Contributor

Excellent, thanks!
r=me after addressing the remaining comments and squashing commits.

Implements RFC 1576.

See: https://github.com/rust-lang/rfcs/blob/master/text/1576-macros-literal-matcher.md

Changes are mostly in libsyntax, docs, and tests. Feature gate is
enabled for 1.27.0.

Many thanks to Vadim Petrochenkov for following through code reviews
and suggestions.

Example:

````rust

macro_rules! test_literal {
    ($l:literal) => {
        println!("literal: {}", $l);
    };
    ($e:expr) => {
        println!("expr: {}", $e);
    };
}

fn main() {
    let a = 1;
    test_literal!(a);
    test_literal!(2);
    test_literal!(-3);
}
```

Output:

```
expr: 1
literal: 2
literal: -3
```
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2018

📌 Commit 37ed2ab has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2018
@bors
Copy link
Contributor

bors commented May 13, 2018

⌛ Testing commit 37ed2ab with merge 9fae153...

bors added a commit that referenced this pull request May 13, 2018
Macros: Add a 'literal' fragment specifier

See: #35625

```rust

macro_rules! test_literal {
    ($l:literal) => {
        println!("literal: {}", $l);
    };
    ($e:expr) => {
        println!("expr: {}", $e);
    };
}

fn main() {
    let a = 1;
    test_literal!(a);
    test_literal!(2);
    test_literal!(-3);
}
```

Output:

```
expr: 1
literal: 2
literal: -3
```

ToDo:

* [x] Feature gate
* [x] Basic tests
* [x] Tests for range patterns
* [x] Tests for attributes
* [x] Documentation
* [x] Fix for `true`/`false`
* [x] Fix for negative number literals
@bors
Copy link
Contributor

bors commented May 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 9fae153 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants