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

Tie stability attributes to features #21248

Merged
merged 24 commits into from
Jan 28, 2015
Merged

Conversation

brson
Copy link
Contributor

@brson brson commented Jan 16, 2015

This implements the remaining bits of 'feature staging', as described in RFC 507.

This is not quite done, but the substance of the work is complete so submitting for early review.

Key changes:

  • unstable, stable and deprecated attributes all require 'feature' and 'since', and support an optional 'reason'.
  • The unstable lint is removed.
  • A new 'stability checking' pass warns when a used unstable library feature has not been activated with the feature attribute. At 1.0 beta this will become an error.
  • A new 'unused feature checking' pass emits a lint ('unused_feature', renamed from 'unknown_feature') for any features that were activated but not used.
  • A new tidy script featureck.py performs some global sanity checking, particularly that 'since' numbers agree, and also prints out a summary of features.

Differences from RFC:

  • As implemented unstable requires a since attribute. I do not know if this is useful. I included it in the original sed script and just left it.
  • RFC didn't specify the name of the optional 'reason' attribute.
  • This continues to use 'unstable', 'stable' and 'deprecated' names (the 'nice' names) instead of 'staged_unstable', but only activates them with the crate-level 'staged_api' attribute.

I intend to update the RFC based on the outcome of this PR.

Issues:

  • The unused feature check doesn't account for language features - i.e. you can activate a language feature, not use it, and not get the error.

Open questions:

  • All unstable and deprecated features are named 'unnamed_feature', which i picked just because it is uniquely greppable. This is the 'catch-all' feature. What should it be?
  • All stable features are named 'grandfathered'. What should this be?

TODO:

  • Add check that all deprecated attributes are paired with a stable attribute in order to preserve the knowledge about when a feature became stable.
  • Update rustdoc in various ways.
  • Remove obsolete stability discussion from reference.
  • Add features for 'path', 'io', 'os', 'hash' and 'rand'.

cc #20445 @alexcrichton @aturon

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@brson
Copy link
Contributor Author

brson commented Jan 16, 2015

r? @alexcrichton

@@ -302,6 +302,7 @@ tidy:
| grep '^$(S)src/libbacktrace' -v \
| grep '^$(S)src/rust-installer' -v \
| xargs $(CFG_PYTHON) $(S)src/etc/check-binaries.py
$(CFG_PYTHON) $(S)src/etc/featureck.py $(S)src/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$(Q)-prefix?

@alexcrichton
Copy link
Member

After thinking about it I don't think that we should require since for unstable apis. I think it may just be extra bookkeeping overhead that may not be worth it. When you use an unstable API there are 0 versions of rust that it is guaranteed to work with, so I don't think it would help us too much to keep track of what version it started to be added in.

@alexcrichton
Copy link
Member

All unstable and deprecated features are named 'unnamed_feature', which i picked just because it is uniquely greppable. This is the 'catch-all' feature. What should it be?

I think in general we want some categories of features:

  • All crates should have their own feature name. For example libcore should probably have a global feature called core or something like that (same for alloc, collections, ...). Everything related to the compiler can probably just be called rustc perhaps?
  • Within the standard library, huge modules (those you listed below like path, io, os, ...) should just have their module name as the feature (or some derivative).
  • Within the remaining unstable APIs, there are some that are still "clearly separable" from a catch-all such as std::time, std::thread_local::scoped, etc. These can probably use the same naming rules as io/os/path.
  • With everything else, these should probably be in a catch-all for now.

I'm not sure how aggressive we want to be right this red-hot-second about categorizing features into these buckets, but it does seem prudent to start earlier rather than later!

For a catch-all name though we may want to strive for something that conveys "this is unstable, but we probably do not want this to be unstable, please tell us if you're using this". I'm not sure if a name can actually convey that though... All I can really think of is std_unstable :(

All stable features are named 'grandfathered'. What should this be?

Some other possibilities:

  • 1.0.0
  • rust-1.0.0
  • initial
  • stable
  • rust1

I suppose with the version in since we don't need to put the version in the feature name, but it also probably doesn't matter too much.

@alexcrichton
Copy link
Member

I didn't see too many changes to the tests in terms of new features, so I'm just writing down a smattering of ideas for tests which we may want to be sure we've got, but I have a feeling you'll know much more than I will bout whether we already have them!

  • Using an unstable API should issue a warning today.
  • Using a stable API should not issue a warning.
  • Using an unstable API with a feature should not issue a warning
  • Passing -A warnings should disable warnings about unstable APIs today (not eventually)
  • Enabling an unused feature should issue a warning, at least for APIs today.

@reem
Copy link
Contributor

reem commented Jan 16, 2015

Does the comment about updating the RFC mean we will be reserving the "nice" names for this? I think that would be a mistake, since this system is unlikely to be useful outside of the standard distribution (or at least it hasn't been designed and considered for outside use).

I also think that warning on unstable today is basically noise, since way too much is still unstable for most code to be reliably written with no use of unstable items. As a result, people have just been sticking #![allow(unstable)] at the top of all their crates, which vastly diminishes the use of the lint as we get closer to 1.0 beta and critical things are no longer unstable. We should leave the lint off for now, then turn it on when it is actually feasible to write most code with no use of unstable APIs.

@brson brson force-pushed the feature-staging branch 2 times, most recently from 8a8f4d5 to 9d117c0 Compare January 21, 2015 21:19
@brson
Copy link
Contributor Author

brson commented Jan 21, 2015

Re "1.0.0" as a feature name, feature names need to be parsable as idents, so it would have to be "1_0_0" or something. I think I'll use 'rust1'.

@brson
Copy link
Contributor Author

brson commented Jan 21, 2015

@reem The names used in-tree are distinguished by the crate-level #[staged_api] attribute, which means that the names can still be used out-of-tree, and since these attributes are only for in-tree we can even rename them if we must later. I think I'll even put staged_api behind a feature gate to ensure that it's unusable out-of-tree.

@brson
Copy link
Contributor Author

brson commented Jan 21, 2015

@reem I don't know if it's worth turning back now. These will become hard errors that require feature attributes in less than five weeks.

@brson
Copy link
Contributor Author

brson commented Jan 21, 2015

@alexcrichton I continue to be concerned about the disconnect between the behavior of deprecated and unstable/stable. I've identified one problem that i think is particularly ugly: as written anything that is marked deprecated can be used on the stable channel (and the RFC explicitly says that anything deprecated is also tagged stable), but i think there is a strong use case for deprecating unstable things as well. I'd like to suggest the following changes:

  • Every deprecated attribute must be paired with either an unstable or stable tag. In effect every feature is either stable or unstable, but specific APIs may be deprecated. This I think is more inline with the continued use of allow(deprecated) for deprecated APIs.
  • The deprecated tag does not require a 'feature' label - I can't think of any purpose it serves and istr coming up with some strange scenarios involving labels changing that i can't remember off hand.

@alexcrichton
Copy link
Member

Both those changes sound good to me, I was questioning the use of feature names for deprecated myself recently.

@alexcrichton
Copy link
Member

@bors: retry

@brson
Copy link
Contributor Author

brson commented Jan 27, 2015

@bors: r=alexcrichton 55d3ea3 p=2

@brson
Copy link
Contributor Author

brson commented Jan 27, 2015

@bors: r=alexcrichhton bd995e5 p=12

@bors
Copy link
Contributor

bors commented Jan 27, 2015

⌛ Testing commit bd995e5 with merge 784be79...

@bors
Copy link
Contributor

bors commented Jan 27, 2015

💔 Test failed - auto-mac-64-nopt-t

@brson
Copy link
Contributor Author

brson commented Jan 27, 2015

@bors: r=alexcrichton de42bc1 p=22

@bors
Copy link
Contributor

bors commented Jan 27, 2015

⌛ Testing commit de42bc1 with merge 01b34c6...

Conflicts:
	src/libcore/cell.rs
	src/librustc_driver/test.rs
	src/libstd/old_io/net/tcp.rs
	src/libstd/old_io/process.rs
@brson
Copy link
Contributor Author

brson commented Jan 27, 2015

@bors: r=alexcrichton 7122 p=22

@bors
Copy link
Contributor

bors commented Jan 27, 2015

⌛ Testing commit 7122305 with merge 68d7f73...

@bors
Copy link
Contributor

bors commented Jan 27, 2015

💔 Test failed - auto-linux-32-opt

@brson
Copy link
Contributor Author

brson commented Jan 27, 2015

@bors: retry
@bors: r=alexcrichton 7122 p=22

@bors
Copy link
Contributor

bors commented Jan 28, 2015

⌛ Testing commit 7122305 with merge a530cc9...

bors added a commit that referenced this pull request Jan 28, 2015
This implements the remaining bits of 'feature staging', as described in [RFC 507](https://github.com/rust-lang/rfcs/blob/master/text/0507-release-channels.md).

This is not quite done, but the substance of the work is complete so submitting for early review.

Key changes:
* `unstable`, `stable` and `deprecated` attributes all require 'feature' and 'since', and support an optional 'reason'.
* The `unstable` lint is removed.
* A new 'stability checking' pass warns when a used unstable library feature has not been activated with the `feature` attribute. At 1.0 beta this will become an error.
* A new 'unused feature checking' pass emits a lint ('unused_feature', renamed from 'unknown_feature') for any features that were activated but not used.
* A new tidy script `featureck.py` performs some global sanity checking, particularly that 'since' numbers agree, and also prints out a summary of features.

Differences from RFC:
* As implemented `unstable` requires a `since` attribute. I do not know if this is useful. I included it in the original sed script and just left it.
* RFC didn't specify the name of the optional 'reason' attribute.
* This continues to use 'unstable', 'stable' and 'deprecated' names (the 'nice' names) instead of 'staged_unstable', but only activates them with the crate-level 'staged_api' attribute.

I intend to update the RFC based on the outcome of this PR.

Issues:
* The unused feature check doesn't account for language features - i.e. you can activate a language feature, not use it, and not get the error.

Open questions:
* All unstable and deprecated features are named 'unnamed_feature', which i picked just because it is uniquely greppable. This is the 'catch-all' feature. What should it be?
* All stable features are named 'grandfathered'. What should this be?

TODO:
* Add check that all `deprecated` attributes are paired with a `stable` attribute in order to preserve the knowledge about when a feature became stable.
* Update rustdoc in various ways.
* Remove obsolete stability discussion from reference.
* Add features for 'path', 'io', 'os', 'hash' and 'rand'.

cc #20445 @alexcrichton @aturon
@bors bors merged commit 7122305 into rust-lang:master Jan 28, 2015
aroben added a commit to aroben/string-cache that referenced this pull request Jan 29, 2015
bors added a commit that referenced this pull request Feb 5, 2015
Builds on my [feature staging PR](#21248) to clean up the tidy scripts a bit, and make them much faster (6s vs ~40s).

Adds make rules 'tidy-basic', 'tidy-binaries', 'tidy-errors' and 'tidy-features'.

This is the output of `make tidy` here:

```
cfg: version 1.0.0-dev (a8c878d 2015-01-25 01:49:14 -0800)
cfg: build triple x86_64-unknown-linux-gnu
cfg: host triples x86_64-unknown-linux-gnu
cfg: target triples x86_64-unknown-linux-gnu
cfg: host for x86_64-unknown-linux-gnu is x86_64
cfg: os for x86_64-unknown-linux-gnu is unknown-linux-gnu
cfg: good valgrind for x86_64-unknown-linux-gnu is 1
cfg: using CC=gcc (CFG_CC)
cfg: enabling valgrind run-pass tests (CFG_ENABLE_VALGRIND_RPASS)
cfg: valgrind-rpass command set to "/usr/bin/valgrind" --error-exitcode=100 --soname-synonyms=somalloc=NONE --quiet --suppressions=/home/brian/dev/rust3/src/etc/x86.supp  --tool=memcheck --leak-check=full
cfg: no lualatex found, deferring to xelatex
cfg: no xelatex found, deferring to pdflatex
cfg: no pdflatex found, disabling LaTeX docs
cfg: no pandoc found, omitting PDF and EPUB docs
cfg: including test rules
cfg: javac not available, skipping lexer test...
check: formatting

* linted .rs files: 4948
* linted .py files: 27
* linted .js files: 2
* linted .sh files: 5
* linted .pl files: 0
* linted .c files: 28
* linted .h files: 3
* other linted files: 0
* total lines of code: 481217
* total non-blank lines of code: 423682

check: binaries
check: extended errors

* 249 error codes
* highest error code: E0315

check: feature sanity

* advanced_slice_patterns         lang    unstable    None    
* alloc                           lib     unstable    None    
* asm                             lang    unstable    None    
* associated_types                lang    stable      1.0.0   
* box_syntax                      lang    unstable    None    
* collections                     lib     unstable    None    
* concat_idents                   lang    unstable    None    
* core                            lib     unstable    None    
* default_type_params             lang    stable      1.0.0   
* globs                           lang    stable      1.0.0   
* hash                            lib     unstable    None    
* if_let                          lang    stable      1.0.0   
* import_shadowing                lang    unstable    None    
* int_uint                        lang    unstable    None    
* intrinsics                      lang    unstable    None    
* io                              lib     unstable    None    
* issue_5723_bootstrap            lang    stable      1.0.0   
* lang_items                      lang    unstable    None    
* link_args                       lang    unstable    None    
* link_llvm_intrinsics            lang    unstable    None    
* linkage                         lang    unstable    None    
* log_syntax                      lang    unstable    None    
* macro_rules                     lang    stable      1.0.0   
* main                            lang    unstable    None    
* managed_boxes                   lang    unstable    None    
* non_ascii_idents                lang    unstable    None    
* old_impl_check                  lang    unstable    None    
* old_orphan_check                lang    unstable    None    
* on_unimplemented                lang    unstable    None    
* opt_out_copy                    lang    unstable    None    
* optin_builtin_traits            lang    unstable    None    
* os                              lib     unstable    None    
* path                            lib     unstable    None    
* phase                           lang    unstable    None    
* plugin                          lang    unstable    None    
* plugin_registrar                lang    unstable    None    
* quad_precision_float            lang    unstable    None    
* quote                           lang    unstable    None    
* rand                            lib     unstable    None    
* rust1                           lib     stable      1.0.0   
* rustc_diagnostic_macros         lang    unstable    None    
* rustc_private                   lib     unstable    None    
* rustdoc                         lib     unstable    None    
* simd                            lang    unstable    None    
* simd_ffi                        lang    unstable    None    
* slicing_syntax                  lang    unstable    None    
* staged_api                      lang    unstable    None    
* start                           lang    unstable    None    
* std_misc                        lib     unstable    None    
* struct_inherit                  lang    unstable    None    
* struct_variant                  lang    stable      1.0.0   
* test                            lib     unstable    None    
* test_accepted_feature           lang    stable      1.0.0   
* test_removed_feature            lang    unstable    None    
* thread_local                    lang    unstable    None    
* trace_macros                    lang    unstable    None    
* tuple_indexing                  lang    stable      1.0.0   
* unboxed_closures                lang    unstable    None    
* unicode                         lib     unstable    None    
* unsafe_destructor               lang    unstable    None    
* visible_private_types           lang    unstable    None    
* while_let                       lang    stable      1.0.0   
```

There's a lot of informational output now, which comes after things like 'NOTE's.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants