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.

Sign up for GitHub

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

Jump to bottom

Force vec![] to expression position only #81080

Merged
merged 2 commits into from
Jan 17, 2021
Merged

Force vec![] to expression position only #81080

merged 2 commits into from
Jan 17, 2021

Conversation

bugadani
Copy link
Contributor

r? @oli-obk

I went with the lazy way of only changing what broke. I moved the test to ui/macros because the diagnostics no longer give suggestions.

Closes #61933

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2021
@bugadani bugadani changed the title Vec diag Force vec![] to expression position only Jan 16, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2021

Superb, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2021

📌 Commit 23a7aaada1ef1bc8428bbff94cffb0f6d680b91b has been approved by oli-obk

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2021
@estebank
Copy link
Contributor

I would love if we ever got the suggestion back, but it is super low priority 😅

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2021

I would love if we ever got the suggestion back, but it is super low priority sweat_smile

maybe we can get it to actually work in patterns once we have deref or box patterns or whatever ends up becoming stable

@estebank
Copy link
Contributor

That'd be a great possibility, but would want us to lint against "expensive" pattern derefs in some form :)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 16, 2021 •
edited
Loading

Unfortunately, this new vec![@force_expr_internal_do_not_use@: expr] syntax is going to show up in the public interface. I suppose it's safe to assume no one will type that, but it'll show up in the generated documentation (and could also possibly be used by autocompletion). Screenshot of the generated std docs after this change:

image

If this is fixing a high priority issue, we can merge it now and deal with that problem later. But otherwise I'd prefer finding a different solution that doesn't leak to the public interface. Maybe something like $crate::__export::force_expr!(..) with a #[doc(hidden)] #[unstable] macro force_expr($e: expr) could work instead. Let me know if I can help.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 16, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Jan 17, 2021

Nice!

If you make it a pub macro force_expr($e:expr) { $e } instead of a #[macro_export] macro_rules! .., you can put it inside mod __export just like (the re-export of) format_args, instead of exposing it from the root of the crate. (You'll need to mark it as #[rustc_macro_transparency = "semitransparent"] to make it behave the same as a macro_rules! wrt hygiene.)

@bugadani
Copy link
Contributor Author

bugadani commented Jan 17, 2021 •
edited
Loading

All right, that's way more complicated than necessary here and could have been done once the macro gets used in more than one place, but whatever :) Thanks for the how-to.

TIL: pub macro exists

@m-ou-se
Copy link
Member

m-ou-se commented Jan 17, 2021

more complicated than necessary here

Yeah, maybe. But in general I think it's good to leave implementation details like this out of the root of the crate. The public interface of core/alloc/std is a sacred place. ^^

Any idea what happened with Cargo.lock? It's downgrading parking_lot now.

@bugadani
Copy link
Contributor Author

Apart from the fact that I accidentally committed it, I have no idea. I'll fix it asap, thanks for noticing :)

leave implementation details like this out of the root of the crate.

Fair enough :)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 17, 2021

Thanks!

@bors r=oli-obk,m-ou-se

@bors
Copy link
Contributor

bors commented Jan 17, 2021

📌 Commit c127ed6 has been approved by oli-obk,m-ou-se

@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 Jan 17, 2021
@m-ou-se m-ou-se mentioned this pull request Jan 17, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2021
@bors
Auto merge of rust-lang#81113 - m-ou-se:rollup-a1unz4x, r=m-ou-se
edeb631 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#79298 (correctly deal with late-bound lifetimes in anon consts)
 - rust-lang#80031 (resolve: Reject ambiguity built-in attr vs different built-in attr)
 - rust-lang#80201 (Add benchmark and fast path for BufReader::read_exact)
 - rust-lang#80635 (Improve diagnostics when closure doesn't meet trait bound)
 - rust-lang#80765 (resolve: Simplify collection of traits in scope)
 - rust-lang#80932 (Allow downloading LLVM on Windows and MacOS)
 - rust-lang#80983 (Remove is_dllimport_foreign_item definition from cg_ssa)
 - rust-lang#81064 (Support non-stage0 check)
 - rust-lang#81080 (Force vec![] to expression position only)
 - rust-lang#81082 (BTreeMap: clean up a few more comments)
 - rust-lang#81084 (Use Option::map instead of open-coding it)
 - rust-lang#81095 (Use Option::unwrap_or instead of open-coding it)
 - rust-lang#81107 (Add NonZeroUn::is_power_of_two)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 19370a4 into rust-lang:master Jan 17, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 17, 2021
@jonas-schievink jonas-schievink mentioned this pull request Jan 19, 2021
Closed
@jonas-schievink
Copy link
Contributor

I'd rather we use macro_rules! for force_expr instead of macro – this has broken vec![] in rust-analyzer, which does not yet support macros 2.0

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2021

There are implementation instructions for a macro solution in #81080 (comment)

I think it's ok to move to an unstable macro_rules macro so we get rust-analyzer support for vec! working again.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2021

The downside is that #[macro_use] extern crate alloc; will then import the force_expr macro.

@m-ou-se m-ou-se mentioned this pull request Jan 21, 2021
Merged
@m-ou-se
Copy link
Member

m-ou-se commented Jan 21, 2021

See #81241

@bugadani bugadani deleted the vec-diag branch January 21, 2021 16:23
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jan 22, 2021
@m-ou-se
Rollup merge of rust-lang#81241 - m-ou-se:force-expr-macro-rules, r=o…
70597f2 
…li-obk

Turn alloc's force_expr macro into a regular macro_rules.

This turns `alloc`'s `force_expr` macro into a regular `macro_rules`.

Otherwise rust-analyzer doesn't understand `vec![]`. See rust-lang/rust-analyzer#7349 and rust-lang#81080 (comment)

Edit: See rust-lang#81241 (comment) for a discussion of alternatives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

The vec![..] macro can be used in pattern position with bad diagnostics
9 participants
@bugadani @oli-obk @bors @estebank @m-ou-se @jonas-schievink @rust-highfive @rustbot @rylev