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

Tracking issue for literal fragment specifier (RFC 1576) #35625

Closed
nikomatsakis opened this issue Aug 12, 2016 · 40 comments · Fixed by #56072
Closed

Tracking issue for literal fragment specifier (RFC 1576) #35625

nikomatsakis opened this issue Aug 12, 2016 · 40 comments · Fixed by #56072
Labels
B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Tracking issue for rust-lang/rfcs#1576.

@nikomatsakis nikomatsakis added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2016
@durka
Copy link
Contributor

durka commented Aug 13, 2016

Is anyone working on implementation already? If not I can take a crack at it.

@LeoTestard
Copy link
Contributor

I'll probably do it.

@LeoTestard
Copy link
Contributor

Huh oh, we have an issue already: do we mean to include potentially minus-prefixed literals? If yes, we have a problem because 1. it means we have to return an Expr instead of a Lit, which means I'm not sure one will be able to use the bound fragment in a non-expr context (for example in a static array type?) and 2. it's no longer always a single-TT, so the benefits in terms of future-proofing are a bit less good. If we don't... well, we loose expressivity. I have no idea what is more intuitive though... Would you, as a Rust user, expect $l:lit to parse -5? I certainly can think of use cases for this...

@nikomatsakis
Copy link
Contributor Author

@LeoTestard good point. I think I would have expected it, yeah...but I also agree with the complications.

@durka
Copy link
Contributor

durka commented Aug 18, 2016

Can't we upgrade Lit to support negative literals?

@joshtriplett
Copy link
Member

joshtriplett commented Dec 14, 2016

@LeoTestard I'd expect "literal" (or a future integer-literal specifier) to include -5 (negative 5), but not - 5 (the negation of positive 5).

Perhaps that expectation doesn't match reality, though. For instance, based on parsing rules for non-literals, I'd expect -5.function() to act like -(5.function()).

So, if handling both -5 and - 5 as literals makes this easier, that works. But "literal" definitely needs to include negative numbers, or we'd have a weird user trap to explain.

@petrochenkov
Copy link
Contributor

@joshtriplett

I'd expect "literal" (or a future integer-literal specifier) to include -5 (negative 5), but not - 5 (the negation of positive 5).

Unary operators, including minus, are processed purely by the parser at the moment, no lexer involved. So their treatment is whitespace agnostic.
(I personally think this is a good thing.)

@LeoTestard
Copy link
Contributor

I agree with @petrochenkov. -5 is not syntactically different from - 5 and has no reason to be.
But I also agree with the fact that the user will probably expect negative numbers to work as literals (even it – syntactically – they're not).

@petrochenkov
Copy link
Contributor

- literal is also supported in literal patterns while arbitrary expressions are not.

fn main() {
    match 10 {
        -1 => {}
        -3 ... -2 => {}
        _ => {}
    }
}

It'd be reasonable to support it in fragments as well.

@jorendorff
Copy link
Contributor

Another thing that won't match is a macro call, like concat!("hello", "world"), or any of the other macros that I think of as expanding to a literal (include_str!, env!...).

I dunno, maybe this is not such a good idea after all. Re-reading the RFC in a more skeptical light, I felt like the "Motivation" section could use some more concrete use cases. Maybe this is just failure of imagination on my part, though!

I think the likelihood of this feature attracting people who really want constant expressions should be acknowledged as a drawback, too.

@RReverser
Copy link
Contributor

Another thing that won't match is a macro call, like concat!("hello", "world"), or any of the other macros that I think of as expanding to a literal (include_str!, env!...).

That's fine IMO. Personally I'd like it to match only explicit literals in code to distinguish, for example, between literal and identifier and produce different code for them. Right now this is not possible because all of tt, expr, ident are too broad and conflict with each other.

@da-x
Copy link
Member

da-x commented Apr 4, 2018

Hi All,

I have implemented this feature in my fork of Rust over 1.25.0, under this branch:

https://github.com/da-x/rust/tree/literal-fragment

Following a quick review by a rustc mentor I may have the free time to provide a full pull request for this.

@da-x
Copy link
Member

da-x commented Apr 4, 2018

BTW B-unstable label has the tooltip "implemented in nightly and unstable" but I did not see it there, i.e. commits & pull requests referencing this. Did I miss it?

bors added a commit that referenced this issue 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
@da-x
Copy link
Member

da-x commented May 13, 2018

@nikomatsakis

The implementation that I worked on was merged, yay!

@Centril Centril added B-RFC-implemented Blocker: Approved by a merged RFC and implemented. and removed B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. labels May 13, 2018
@clarfonthey
Copy link
Contributor

Is there anything stopping an FCP for this?

@joshtriplett
Copy link
Member

@clarcharr It's currently merged in unstable; do you mean an FCP to declare it stable?

@clarfonthey
Copy link
Contributor

Yes, an FCP to stabilise. Should have clarified.

@RReverser
Copy link
Contributor

@jendrikw Agreed, this should be allowed just like passing any other node types.

@0e4ef622
Copy link
Contributor

0e4ef622 commented Aug 9, 2018

Is there anything else preventing this from being stabilized? The issue above has been fixed.

@Centril
Copy link
Contributor

Centril commented Sep 15, 2018

This has baked for some time now and I believe it should be ready for stabilization.
So let's start the review.

@rfcbot merge

The feature gate test is here:

Unstable book:

Tests defined here:

@rfcbot
Copy link

rfcbot commented Sep 15, 2018

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 15, 2018
@scottmcm
Copy link
Member

Reading through the tests, I see that -2 is a literal.

While that makes sense to me, I remember hearing that it wasn't, but was instead a negation operator and a literal. Am I misremembering?

@eddyb
Copy link
Member

eddyb commented Sep 15, 2018

@Centril
Copy link
Contributor

Centril commented Sep 15, 2018

@scottmcm

While that makes sense to me, I remember hearing that it wasn't, [...]

I agree that it makes sense.

I would say that even if -2 is interpreted as negation + a literal, I would probably still want the literal macro matcher to accept -2 since that behavior seems more intuitive. In other words, I would see -2 being -(2) more as an implementation detail than what a literal is in the mental model.

@clarfonthey
Copy link
Contributor

I think that the nuance with -2 causes a bit of a problem, because I don't think it should be treated as a literal, but I also don't think that the current implementation of literal would allow detecting it either. You could naïvely allow $(-)?$lit:literalbut that would allow -"hello" which is clearly wrong.

@da-x
Copy link
Member

da-x commented Sep 16, 2018

@clarcharr there are tests currently implemented that verify that -2 is detected as a literal.

@clarfonthey
Copy link
Contributor

I worded that wrong; I meant that only having a literal matcher and not being able to distinguish strings and numbers means that you can't easily make -2 not a literal.

@alexcrichton
Copy link
Member

@scottmcm for procedural macros the input as lexed by the compiler is - 2 (two tokens) but procedural macros can create Literal tokens that are negatives (like -2). In that sense matching -2 for :literal makes sense to me.

@Jezza
Copy link

Jezza commented Oct 9, 2018

Are literals generated by the compiler counted as true literals?
A quick example would be something like:

macro_rules! bind {
	($type:ty) => {
		bind!($type, concat!(stringify!($type), ".html"));
	};
    ($type:ty, $path:literal) => {
		println!("Page: {:?}", stringify!($type));
    	println!("Path: {}", $path);
    };
}

This fails to match, as the expansion, I guess, happens after the selection.

I just naively assumed that would have worked.

@dtolnay
Copy link
Member

dtolnay commented Oct 9, 2018

concat!(stringify!($type), ".html") is not a literal generated by the compiler, it is an identifier followed by an exclamation mark followed by a set of parentheses containing some tokens. See for example:

macro_rules! m {
    ($first:tt $second:tt $third:tt) => {
        println!(stringify!($third));
        println!(stringify!($second));
        println!(stringify!($first));
    };
}

fn main() {
    m! {
        concat!(stringify!($type), ".html")
    }
}

If you were to use it in expression position, it would be interpreted as an invocation of concat! which expands to a string literal. In this case and in your bind macro though, it is never in a syntactic position where it would be treated as an expression.

@Jezza
Copy link

Jezza commented Oct 9, 2018

I can understand semantically why it doesn't make sense, I just thought it would have, but that being said, I already changed it to $path:expr, so there's no issue.

It just made more sense to me as a literal, because according to the compiler, it could generate a literal from it.
I guess it was more a question than an actual issue.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 9, 2018

concat!(stringify!($type), ".html") is not a literal generated by the compiler, it is an identifier followed by an exclamation mark followed by a set of parentheses containing some tokens.

This is true, but there's more to it!
A few built-in macros can expand their arguments eagerly through ~m a g i c~, so they accept concat!(...) and likes even if they expect string literals:

fn main() {
    println!("{}", env!("PATH")); // OK
    // println!("{}", env!(10)); // ERROR expected string literal
    println!("{}", env!(concat!("PATH"))); // OK
}

Fortunately (or not), this currently cannot be done in user-defined macros.

@Jezza
Copy link

Jezza commented Oct 9, 2018

I actually just realised that I can't change it to $path:expr, as it's used in include_str.
So, I've just had to manually unroll the macro.

4gqbg0i

It doesn't look pretty. :(

Ninja edit:
I just tested what actually happens if you try to pass in a non-literal with expr.
It rejects it the exact same way.
Does the include_str already make use of this feature?
If not, the include_str seems to propagate the required token.
Oh, unless the compiler steps in... That's actually probably the case...

I've reverted it back to the :expr type.

@rfcbot
Copy link

rfcbot commented Nov 9, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 9, 2018
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 19, 2018
@rfcbot
Copy link

rfcbot commented Nov 19, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril
Copy link
Contributor

Centril commented Nov 19, 2018

@da-x Since you implemented this, would you be willing to write up the stabilization PR?

@da-x
Copy link
Member

da-x commented Nov 19, 2018

@Centril sure

pietroalbini added a commit to pietroalbini/rust that referenced this issue Nov 25, 2018
…etrochenkov

Stabilize macro_literal_matcher

This followed FCP in rust-lang#35625.

Closes rust-lang#35625
@jethrogb
Copy link
Contributor

This appears to be missing from the Rust reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-implemented Blocker: Approved by a merged RFC and implemented. B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.