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

Add stability inheritance #15029

Merged
merged 1 commit into from
Jun 21, 2014
Merged

Add stability inheritance #15029

merged 1 commit into from
Jun 21, 2014

Conversation

aturon
Copy link
Member

@aturon aturon commented Jun 19, 2014

This commit makes several changes to the stability index infrastructure:

  • Stability levels are now inherited lexically, i.e., each item's
    stability level becomes the default for any nested items.

  • The computed stability level for an item is stored as part of the
    metadata. When using an item from an external crate, this data is
    looked up and cached.

  • The stability lint works from the computed stability level, rather
    than manual stability attribute annotations. However, the lint still
    checks only a limited set of item uses (e.g., it does not check every
    component of a path on import). This will be addressed in a later PR,
    as part of issue Detect stability attributes on modules and crates #8962.

  • The stability lint only applies to items originating from external
    crates, since the stability index is intended as a promise to
    downstream crates.

  • The "experimental" lint is now allow by default. This is because
    almost all existing crates have been marked "experimental", pending
    library stabilization. With inheritance in place, this would generate
    a massive explosion of warnings for every Rust program.

    The lint should be changed back to deny-by-default after library
    stabilization is complete.

  • The "deprecated" lint still warns by default.

The net result: we can begin tracking stability index for the standard
libraries as we stabilize, without impacting most clients.

Closes #13540.

This commit makes several changes to the stability index infrastructure:

* Stability levels are now inherited lexically, i.e., each item's
  stability level becomes the default for any nested items.

* The computed stability level for an item is stored as part of the
  metadata. When using an item from an external crate, this data is
  looked up and cached.

* The stability lint works from the computed stability level, rather
  than manual stability attribute annotations. However, the lint still
  checks only a limited set of item uses (e.g., it does not check every
  component of a path on import). This will be addressed in a later PR,
  as part of issue rust-lang#8962.

* The stability lint only applies to items originating from external
  crates, since the stability index is intended as a promise to
  downstream crates.

* The "experimental" lint is now _allow_ by default. This is because
  almost all existing crates have been marked "experimental", pending
  library stabilization. With inheritance in place, this would generate
  a massive explosion of warnings for every Rust program.

  The lint should be changed back to deny-by-default after library
  stabilization is complete.

* The "deprecated" lint still warns by default.

The net result: we can begin tracking stability index for the standard
libraries as we stabilize, without impacting most clients.

Closes rust-lang#13540.
@sfackler
Copy link
Member

You may be able to remove the stability attributes from the unused_attribute lint whitelist: https://github.com/rust-lang/rust/blob/master/src/librustc/middle/lint.rs#L1107

@huonw
Copy link
Member

huonw commented Jun 19, 2014

The "experimental" lint is now allow by default. This is because
almost all existing crates have been marked "experimental", pending
library stabilization. With inheritance in place, this would generate
a massive explosion of warnings for every Rust program.

Would it be possible to having it on warn by default, but only warn on the actual extern crate ...; declaration. That is,

extern crate core; // warning: use of experimental crate

fn main() {
     core::iter::Repeat::new(1); // no warning
}

I imagine this kinda goes against the whole inheritance thing. Maybe the warning should only be on the "top-level"/first use of names with an explicit stability level, i.e. use foo::experimental_module; warns, but then writing experimental_item::bar() later doesn't warn (unless bar has its own explicit marking).

This would require warning on things like ::stable_crate::experimental_module::bar().

@brson
Copy link
Contributor

brson commented Jun 19, 2014

@huonw I also think that may be a useful way to treat experimental crates. I do think we can experiment with the ergonomics of how this is presented to the user later.

Another approach that we were discussing was to have two modes: one that just reminds you a single time that you are using experimental/deprecated features, and another that displays them all individually for when you want to fix them.

@brson
Copy link
Contributor

brson commented Jun 19, 2014

@aturon Have you noted any performance impact from this change?

@brson
Copy link
Contributor

brson commented Jun 19, 2014

LGTM, but I'll wait to hear the response to the comments to r+.

foo.trait_experimental_text();
foo.trait_unstable();
foo.trait_unstable_text();
foo.trait_unmarked();
Copy link
Member

Choose a reason for hiding this comment

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

Could you move these cases to an external crate as well, and make sure they still invoke errors?

I think this was an exhaustiveness check to make sure stability is warned about in all possible cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding, but I believe all of the cases are being checked in the cross_crate module above.

Copy link
Member

Choose a reason for hiding this comment

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

Aha, indeed!

@aturon
Copy link
Member Author

aturon commented Jun 19, 2014

@huonw @brson Yes, I'd like to treat the lint UI separately, and for right now focus on the tooling we need to make progress on stabilization (e.g. a stability dashboard).

re: performance, I will run some timing tests and get back to you later today, @brson.

@aturon
Copy link
Member Author

aturon commented Jun 19, 2014

@brson I did the following test on master and my branch: git clean -f && ./configure && time make -j8

The user time was:

  • master: 1067.5s
  • branch: 1082.5s

giving a 15s total overhead (i.e. 1.4%).

The difference in real time was ~6s, or 0.7%.

I'm not sure what the typical overhead for an additional pass over the AST is? If this seems too high to you, I can spend some time figuring out whether the overhead is coming from the extra pass or the lookups for the lint.

@huonw
Copy link
Member

huonw commented Jun 19, 2014

@aturon you can get detailed pass break-down by passing -Z time-passes to rustc running on a specific crate (e.g. rustc -Z time-passes src/librustc/lib.rs).

@aturon
Copy link
Member Author

aturon commented Jun 19, 2014

@huonw @brson

Running stage2 time-passes on librustc:

  • New stability index AST pass: 0.045s
  • Lint in master: 0.551s
  • Lint in branch: 0.360s

These numbers are consistent across multiple runs.

So I think my time experiment before was just too noisy, and the new stability indexing code appears to be faster. Perhaps that's not surprising, since it's using fast hashmap lookups during the lint.

bors added a commit that referenced this pull request Jun 21, 2014
This commit makes several changes to the stability index infrastructure:

* Stability levels are now inherited lexically, i.e., each item's
  stability level becomes the default for any nested items.

* The computed stability level for an item is stored as part of the
  metadata. When using an item from an external crate, this data is
  looked up and cached.

* The stability lint works from the computed stability level, rather
  than manual stability attribute annotations. However, the lint still
  checks only a limited set of item uses (e.g., it does not check every
  component of a path on import). This will be addressed in a later PR,
  as part of issue #8962.

* The stability lint only applies to items originating from external
  crates, since the stability index is intended as a promise to
  downstream crates.

* The "experimental" lint is now _allow_ by default. This is because
  almost all existing crates have been marked "experimental", pending
  library stabilization. With inheritance in place, this would generate
  a massive explosion of warnings for every Rust program.

  The lint should be changed back to deny-by-default after library
  stabilization is complete.

* The "deprecated" lint still warns by default.

The net result: we can begin tracking stability index for the standard
libraries as we stabilize, without impacting most clients.

Closes #13540.
@bors bors closed this Jun 21, 2014
@bors bors merged commit 6008f2c into rust-lang:master Jun 21, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Jun 19, 2023
Add more log in "terminator is none" assert

cc rust-lang#15029
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.

Overhaul stability pass
6 participants