Conditions are always private

[brson]

The condition! macro expands to a private mod and adding pub to the condition! declaration has no effect. As a result, all conditions are private and they are not usable. The quickest workaround is probably to make all conditions public.

[brson]

Here's my test case

    mod m {
        condition! {
            sadness: int -> int;
        }

        mod n {
            use super::sadness;

            #[test]                                                                                                                                                                                                  
            fn test_conditions_are_public() {
                let mut trapped = false;
                do sadness::cond.trap(|_| {
                    0
                }).in {
                    sadness::cond.raise(0);
                }
            }
        }
    }

[brson]

@pnkfelix Had a lot of good comments about this issue in #6046. Reproducing them here:

I was about to r+ this, since I agree that if we have to choose between "all conditions are never public" or "all conditions are always public", then I would choose the latter.

But I stopped short, due to two things:

1. A comment / potential scope creep: This may be a sign of a general weakness in our module system. I.e., instead of requiring macro writers to have to build in pub declarations (or thread optional ones down from the caller; see 2 below), perhaps we should have an export item that can publicize names from a module. (Ideally an export item would be able to also rename identifiers, so that a local name could be short for ease of use by the module owner, but be given as descriptive a name as necessary on export.) Or maybe pub use could be adapted to serve as such a feature.
2. More specifically about this issue (and thus not scope creep): I generally agree that we need to be able to express public conditions, and so the current implementation of the condition! syntax is unsatisfactory. But perhaps instead of making every invocation of condition! publicize its identifier, we might want to achieve this effect via a use of the pub keyword. For example, by making two variants of the condition! form, and dispatch based on whether pub appears, like so:

macro_rules! condition (
{ $c:ident: $in:ty -> $out:ty; } => {
...
};
{ pub $c:ident: $in:ty -> $out:ty; } => {
...
}
)

or, if we were to extend our macro system somewhat:

macro_rules! condition (
{ $access:keyword(pub)? $c:ident: $in:ty -> $out:ty; } => {
$access mod $c {
...
}
};
)

where the $access expands to nothing if the pub (made optional by the ?) is absent from the input. (At least, as far > as I can tell we do not support code like the above right now; corrections welcome.)

1. (ignore)
2. (ignore)
3. Even if we did not want put the code in for both of the variants from (2.) above right now, it might be nice to try to encourage a property that identifiers exported from the module should have the word pub explicitly attached to them, no? That is, maybe your macro should be written as:

macro_rules! condition (
{ pub $c:ident: $in:ty -> $out:ty; } => {
pub mod $c {
fn key(_x: @::core::condition::Handler<$in,$out>) { }
...
}
}
)

If you were to post a revised commit that took the approach outlined in (3.), so that we could introduce variant (2.) in a backwards compatible way later if we liked, I am pretty sure I would r+ it unreservedly.

[bblum]

Not sure or not if changing condition! will be backwards-incompatible... maybe the triage committee knows. Nominating.

[alexcrichton]

I think that this can be fixed now? In libsyntax/ext/expand there are two rules for defining conditions and one has the pub keyword in front (condition! { pub ... }), and it looks like it was awaiting a snapshot to remove the previous form of always-pub.

I think that this now entails removing the pub from the private form of defining conditions and then fixing all code which currently depends on it. Regardless though I'd agree with this being backwards-compatible.

[alexcrichton]

Actually, looking more into this, this is blocked on #5129.

In the generated mod, the cond static has to be pub so siblings of the mod can access it to do things like raise, but currently anyway can reach through private mods to public items (that's the issue)

[graydon]

I don't think this is blocked on #5129. That's a different bug. This bug is about the condition itself being priv, which it no longer is (you can declare it pub). Am I misunderstanding, or can this close?

[alexcrichton]

I figured the problem would be that you can't actually make a condition private right now. We do have a pub and a non-pub variant of the condition! macro, but that's not quite sufficient. Currently resolve rules block this from working.

If you declare a pub condition, you'd expect:

pub mod condition_name {
  pub static cond: ... = ...;
}

Which certainly works to make it public. If you declare a private condition, you would expect

mod condition_name {
  static cond: ... = ...;
}

Now in theory you can access any member of your immediate children's modules, but that's not the case in this situation. Currently resolve bugs block this from working. Knowing that, you might want to generate code for private conditions like

mod condition_name {
  pub static cond: ... = ...;
}

which indicates that you can reach your private child module to read cond. The owning module of the condition can certainly do that, but the bug in #5129 means that so can everyone else.

I guess this isn't really blocked on #5129 per-se, but rather it's blocked on fixing resolve rules in general.

[alexcrichton]

Opened #8215 about resolve issues

[graydon]

Ok, so I think this bug as written is fixed, but I'm not entirely sure if it'll have to change again when dependent bugs are fixed. Leaving open for re-analysis in future triage when Brian and Patrick are here.

[catamorphism]

accepted for backwards-compatibility

[pnkfelix]

@alexcrichton So, one way to make "progress" on this ticket is to:

1. change the expansion of condition! to be pub-preserving, i.e. pub declaration expands to pub mod, and non-pub declaration expands to non-pub mod (which corresponds to your first "if you declare a private condition" scenario; I will sidestep the question of whether the static cond inside the mod should be pub or not -- it depends on how the semantics of resolve get resolved), and then,
2. at every spot in the code base that cannot deal with that due to resolve issues, change those call-sites, adding a pub and a note pointing to one of the resolve issues.

Basically, I don't think it's a great idea to build a resolve-workaround into a core macro like this, especially if the workaround itself is just as leaky as the per-call-site workaround. (That is, I would understand if putting the work-around into the macro actually bought us anything, but it doesn't, its just making the resolve issues seem like less of a problem than they really are, because people are not aware of the extra pubs that are getting implicitly inserted into the macro expansions.)

Any thoughts on that strategy? Is it a waste of time? (I'm going to give it a shot nonetheless.)

[pnkfelix]

(and of course, there's the question of whether the cases that need a pub added should have had it anyway. I'll play it by ear.)

[alexcrichton]

Sounds like a good strategy to me! I hope to look into the resolve stuff soon as well, but no guarantees on that sadly.