else can be used without if #23

Open
opened 2019-08-05 06:33:12 -05:00 by hellow554 · 12 comments
hellow554 commented 2019-08-05 06:33:12 -05:00 (Migrated from github.com)

TIL: https://lukaslueg.github.io/macro_railroad_wasm_demo

It clearly shows that an else can be used solely without a leading `if.
A quick jump into the compiler shows it as well:

use cfg_if::cfg_if;

fn main() { 
    cfg_if! {
        else {
            fn a() {
                println!("Foo");
            }
        }
    }
    a();
}

Is this intended? If yes, it should be documented, if not, it should be prohibited.

TIL: https://lukaslueg.github.io/macro_railroad_wasm_demo It clearly shows that an `else` can be used solely without a leading `if. A quick jump into the compiler shows it as well: ```rust use cfg_if::cfg_if; fn main() { cfg_if! { else { fn a() { println!("Foo"); } } } a(); } ``` Is this intended? If yes, it should be documented, if not, it should be prohibited.
alexcrichton commented 2019-08-05 10:03:20 -05:00 (Migrated from github.com)

I think this is just a mistake of the macro! It's not an intended use case but I don't really think it's that big of an issue to either document or prohibit. It seems like sort of a weird quirk at best?

I think this is just a mistake of the macro! It's not an intended use case but I don't really think it's that big of an issue to either document or prohibit. It seems like sort of a weird quirk at best?
zicklag commented 2020-03-28 19:58:36 -05:00 (Migrated from github.com)

My gut reaction is that you shouldn't allow something like this because quirks like that that you aren't expecting could lead to bugs. This would be an opportunity to accidentally forget the if of the statement which would be a logic error in ( I think ) every instance.

I mean, imagine if rustc let us have else's without if's.

If it is a big pain to fix from an implementation perspective, though, then I think that documenting the quirk would be reasonable enough.

My gut reaction is that you *shouldn't* allow something like this because quirks like that that you aren't expecting could lead to bugs. This would be an opportunity to accidentally forget the `if` of the statement which would be a logic error in ( I *think* ) every instance. I mean, imagine if `rustc` let us have `else`'s without `if`'s. If it is a big pain to fix from an implementation perspective, though, then I think that documenting the quirk would be reasonable enough.
camsteffen commented 2020-10-08 13:35:59 -05:00 (Migrated from github.com)

I agree with @alexcrichton. But I would add that I think an else clause by itself is kinda cute.

I agree with @alexcrichton. But I would add that I think an `else` clause by itself is kinda cute.
V0ldek commented 2022-01-11 09:05:01 -06:00 (Migrated from github.com)

This is fixed on the current master, appears to be changed in github.com/alexcrichton/cfg-if@3499136896

Any hope this gets released? The commit is one year old.

I'd happily take this. I'm using cfg_if quite heavily for architectural dependencies and I've already had a confusing bug whose root cause was a missing if due to some copy-paste error. There's no use case for allowing this syntax, I'd prefer it to be rejected.

This is fixed on the current master, appears to be changed in https://github.com/alexcrichton/cfg-if/commit/349913689695130c2c63d1075e57b0917c98ee6e Any hope this gets released? The commit is one year old. I'd happily take this. I'm using `cfg_if` quite heavily for architectural dependencies and I've already had a confusing bug whose root cause was a missing `if` due to some copy-paste error. There's no use case for allowing this syntax, I'd prefer it to be rejected.
chris-morgan commented 2022-01-12 07:37:42 -06:00 (Migrated from github.com)

I don’t think there’s no use case. I can imagine realistic code depending on this behaviour. Something along these lines:

use cfg_if::cfg_if;

// Several different implementations of a thing
#[derive(Debug)] struct Windows;
#[derive(Debug)] struct Unix;
#[derive(Debug)] struct Fallback;

macro_rules! demo {
    ($($x:ident => $y:expr),*) => {{
        cfg_if! {
            $(
                if #[cfg($x)] {
                    $y
                }
            )else*
            else {
                Fallback
            }
        }
    }}
}

fn main() {
    println!("{:?}", demo!());
    println!("{:?}", demo!(unix => Unix, windows => Windows));
}

That single-character change of * to + is a breaking change; although I doubt that anyone is depending on it, I reckon it should either be reverted, or the next release called 2.0.0.

(It’s pure coincidence that I’m commenting less than a day after you: I just happen to be starting doing detailed code review of crates starting with this one today.)

I don’t think there’s *no* use case. I can imagine realistic code depending on this behaviour. Something along these lines: ```rust use cfg_if::cfg_if; // Several different implementations of a thing #[derive(Debug)] struct Windows; #[derive(Debug)] struct Unix; #[derive(Debug)] struct Fallback; macro_rules! demo { ($($x:ident => $y:expr),*) => {{ cfg_if! { $( if #[cfg($x)] { $y } )else* else { Fallback } } }} } fn main() { println!("{:?}", demo!()); println!("{:?}", demo!(unix => Unix, windows => Windows)); } ``` That single-character change of `*` to `+` *is* a breaking change; although I doubt that anyone is depending on it, I reckon it should either be reverted, or the next release called 2.0.0. (It’s pure coincidence that I’m commenting less than a day after you: I just happen to be starting doing detailed code review of crates starting with this one today.)
V0ldek commented 2022-01-14 06:09:21 -06:00 (Migrated from github.com)

Yes, this is technically a breaking change, so semver requires that release be 2.0.0. That's fine by me, I'd just like to see this released, it's frozen on master which is simply a waste :)

Yes, this is technically a breaking change, so semver requires that release be 2.0.0. That's fine by me, I'd just like to see this released, it's frozen on master which is simply a waste :)
JohnTitor commented 2023-04-22 20:34:27 -05:00 (Migrated from github.com)

@rust-lang/crate-maintainers Any thoughts on this? I think releasing it as 2.0.0 makes some sense but also concerned that would affect a lot of crates. The decision here seems important.

@rust-lang/crate-maintainers Any thoughts on this? I think releasing it as 2.0.0 makes some sense but also concerned that would affect [a lot of crates](https://crates.io/crates/cfg-if/reverse_dependencies). The decision here seems important.
joshtriplett commented 2023-04-22 21:39:35 -05:00 (Migrated from github.com)

I don't think this alone is worth a release, given that it would introduce a transition period where crates would be using both for a while. If we have other features motivating a major release then we could include this as well.

But personally, I would love to see this included in the standard library instead. If people are going to go through a transition, let's have them go through a transition to the version in the standard library.

I don't think this alone is worth a release, given that it would introduce a transition period where crates would be using both for a while. If we have other features motivating a major release then we could include this as well. But personally, I would love to see this included in the standard library instead. If people are going to go through a transition, let's have them go through a transition to the version in the standard library.
JohnTitor commented 2023-04-22 22:25:19 -05:00 (Migrated from github.com)

That sounds great, thank you!
Then I'm going to do the following if no one raises any objections in a few days:

  • revert else+ to else* in this crate
  • submit a PR as a proposal to include it in the stdlib
That sounds great, thank you! Then I'm going to do the following if no one raises any objections in a few days: - revert `else+` to `else*` in this crate - submit a PR as a proposal to include it in the stdlib
joshtriplett commented 2023-04-23 00:19:47 -05:00 (Migrated from github.com)

@JohnTitor There's already an internal copy in the stdlib; I put it there. The PR would just need to propose stabilizing it.

(Also, you might add a comment next to the version number in Cargo.toml saying if this ever needs bumping to a new major version then we should fix this.)

@JohnTitor There's already an internal copy in the stdlib; I put it there. The PR would just need to propose stabilizing it. (Also, you might add a comment next to the version number in `Cargo.toml` saying if this ever needs bumping to a new major version then we should fix this.)
Amanieu commented 2023-04-23 00:32:03 -05:00 (Migrated from github.com)

If we're considering moving this to the standard library (which I wholly support!) then we should perhaps consider whether to introduce language-level syntax for this considering how widely used cfg_if is. However this discussion is starting to be off-topic for this issue.

Regarding the issue itself, I agree that we should not do a 2.0.0 release just for this.

If we're considering moving this to the standard library (which I wholly support!) then we should perhaps consider whether to introduce language-level syntax for this considering how widely used `cfg_if` is. However this discussion is starting to be off-topic for this issue. Regarding the issue itself, I agree that we should not do a 2.0.0 release just for this.
photino commented 2023-07-21 23:37:56 -05:00 (Migrated from github.com)

Any progress on moving this to the standard library?

Any progress on moving this to the standard library?
This discussion has been locked. Commenting is limited to contributors.
No milestone
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-rust-lang/cfg-if#23
No description provided.