kernel: remove unstable crate_visibility_modifier, aka crate -> pub(crate)

[mcy]

Pull Request Overview

This commit removes the crate_visibility_modifier unstable pragma. The
upgrade path is a slightly nuanced variant of s/crate/pub(crate)/g, so
this change is a trivial noop.

Testing Strategy

This change is a noop, so make allcheck is sufficient.

TODO or Help Wanted

N/A

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make formatall.

[bradjc]

This also is a step backwards in my opinion, and is far from a noop. This series of changes could use a tracking issue because I don't understand the motivation.

[mcy]

@bradjc How isn't it a noop? crate experimental is syntax sugar for pub(crate), so no behavior will have changed form this PR.

This feature seems to be a leftover from the big module system shakeup from a couple years ago, and stabilization interest in it seems to have mostly disappeared (since as pub(crate) already does this).

[alevy]

is a slightly nuanced variant of s/crate/pub(crate)/g

@mcy do you just mean that the keyword crate sometimes appears in other places too that are not replaced?

@bradjc is the trouble just that this is more verbose? And if so, would being convinced that the crate visiblity modifier will never be stabilized sufficient to convince you? Or do you have non-syntactic concerns as well?

[bradjc]

Is it clear that a replacement for pub(crate) is not happening? rust-lang/rust#53120 isn't clear.

In any case, why does it matter if Rust is going to stabilize it? Rust can't stabilize asm! but we have to use it. I agree with the commenters rust-lang/rust#53120 that pub(crate) is clunky and not a better option. I think this is a case where an issue is the place to discuss this, or at least provide motivation, like #1544 did.

[alevy]

Perhaps a good place for this would be a tracking issue for all the features that need to be stabilized or removed from Tock in order to compile on stable, a-la SergioBenitez/rocker#19

That way we won't lose track of features to address even if they don't make sense to remove independently. I.e. if this were the last thing we had to change in order to compile on stable, I suspect @bradjc would think differently. At the same time, if we have to compile on nightly anyway, this does seem like a trivial feature to use that may be stabilized.

@mcy does that seem reasonable?

FWIW, I agree that pub(crate) is harder to read, but don't necessarily feel as strongly about keeping crate atm.

[ppannuto]

I think @alevy nicely summarized my feelings here as well. If this were the only thing keeping us off stable, then it's a regression worth accepting. While we're still comfortably stuck on nightly, however, let's use the nicer ergonomics that are available.

[gendx]

I don't think renaming crate to pub(crate) is worth calling a "regression".

On a personal level I would even add that I was somewhat confused when I first saw the use of crate in this place, as I expected it in imports only (i.e. extern crate foo; or use crate::bar;). To me, pub(crate) makes it clearer that it's visibility that we're talking about.

Besides, my text editor doesn't recognize the use of crate as a visibility specifier, and therefore just highlights it as invalid syntax, which is also quite annoying. I was wondering why that was the case, but now that I'm aware it's an unstable feature it's clearer.

Last, I'd add that I quite agree with the following argument, which was mentioned in some of the other "unstable feature" issues: using the most stable spelling of two equivalent things is allowing more Rust developers to understand and join the Tock code base (as opposed to having to learn the quirks of "Tock's dialect" of Rust). Some unstable features really bring unique value (e.g. asm), but crate_visibility_modifier seems like the least useful of all.

[bradjc]

Tock has always been on nightly, and has always used more bleeding-edge features of the language as a result. I'm sure that if we went back a couple years there would be features of Rust used in Tock that are now commonplace but at the time would have been unusual.

I don't think it's reasonable for Tock developers, who are using features included in the nightly version of rust language, to know what will eventually become common or not. Particularly when the tracking issue (as in this case) seems supportive of the issue.

Moving to a more conservative approach, which is very different than how Tock has been developed so far, is a rather significant change, and this PR is in a sense a proxy for that philosophical change. In my opinion, Tock has always seen Rust as a tool, and wants to experiment with and exploit all of the advantages that Rust can give us, hence the willingness to embrace new features.

[mcy]

I don't think it's so easy as "embracing new features"... it's a bit more subtle. There is an implicit contract you make when you use a nightly feature: "this is experimental, it may miscompile, and it may disappear without warning". The last one is especially worrying, because it may cost many engineer hours to untangle us from particular features if they are abandoned or radically changed. This is an especially large concern with fringe type-system features (which, thankfully, we barely use). Even the choice of keyword crate itself remains contentious.

For something like crate/pub(crate), we need to weigh the tradeoffs. If we keep it, we risk all of our code breaking if Rust decides they don't want to keep it the way it is (maybe they'll say "lol yeah intern is the name we're using"). If we remove it, we have to type a few extra characters (and the compiler's errors will teach everyone the "new" way to do things). For a change like this one, where I generated the PR with a single perl -pie command, the upgrade path is trivial.

Of course, I may accidentally misrepresent the progress of some unstable features, because I'm not looking closely at all of them and might forget to check the latest progress. Perhaps people want to get rid of pub(crate) more than I thought.

Given how fickle the compilers internal community is, I am very hesitant to use unstable features in new code, especially when there is major technical barrier (compare const_fn, which we're suck with because we're missing the one thing that the evaluator can't do yet, and the costs of moving off will probably be high).

[ppannuto]

I think @bradjc's comment of "philosophical change" captures the tension here well. I remember a discussion in 2015 when we were first learning Rust that (paraphrasing) stable Rust and nightly Rust are like two different languages, so when you're finding tutorials and examples, be careful of what language they are written in.

In my mind, Tock has always been a project that embraces and is written in unstable Rust. That's partially a function of its academic roots, where exploring the capabilities of new features is intellectually interesting and useful. It was partially a function of the capabilities of stable/unstable Rust both then and now.

Speaking personally, my philosophy has always been that if all of stable Rust manages to catch up to Tock, that's lovely and maybe we can flip the switch. Indeed, when nightly features do stabilize it makes sense to adapt to them, and we have. However, actively working towards stable Rust has never been a goal (to me).

As the stakeholders in Tock change and the project matures, perhaps it is time to re-evaluate this decision [and, having typed this here, I realize that perhaps #1654 is really the place to have this discussion]. However, it should come with the understanding that this is effectively asking a project to change the language it is written in. As it stands today, adding or removing a feature is a total no-op to me, so to the me of March 4th, this PR is strictly a regression, as it changes to a more obtuse syntax for no benefit. I'm willing to have the discussion about whether I should re-evaluate how we treat features (preferably in #1654), but pending that, I'm currently opposed to changes like this.

[phil-levis]

Pat, I think you capture the essence of the issue well.

I personally lean towards stable Rust. Or, perhaps, if we could enumerate a very small set of unstable features we use and will limit ourselves to. But it seems like we've started using some I wasn't even aware of -- they seemed good in that situation, but have larger-scale issues for long-term support.

[ppannuto]

Blocked on #1654

[ppannuto]

Just a quick note that this is being discussed again upstream: rust-lang/rust#53120

I think I'm willing to accede to whichever way upstream decides for Tock. If it looks like it will be adopted, let's keep it, otherwise let's move to pub(crate). Hopefully a clear direction will come out in the next little while from the renewed discussion.

[bradjc]

#1654 is a worthy goal, and it looks more and more like crate_visibility_modifier was a false start on my part. Can we update this PR and merge it?

If for some reason Rust has a change of heart and presents an alternative to pub(crate) I will open a new PR, however :)

[hudson-ayers]

rebased; ready for review

[bradjc]

This is such a Rusty PR. They made visibility and scoping use the "(" character, rather than "{" or ":", the two symbols Rust already uses for visibility and scoping. Reminds me of the old days of learning Rust :).

[ppannuto]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)
• continuous-integration/travis-ci/push
• emulation-check