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

Tracking issue for RFC 2052: Epochs #44581

Closed
3 tasks done
aturon opened this issue Sep 14, 2017 · 23 comments
Closed
3 tasks done

Tracking issue for RFC 2052: Epochs #44581

aturon opened this issue Sep 14, 2017 · 23 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-core Relevant to the core team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management

Comments

@aturon
Copy link
Member

aturon commented Sep 14, 2017

This is a tracking issue for the RFC "Evolving Rust through Epochs" (rust-lang/rfcs#2052).

Overall status

Next work items

Unresolved questions

How to handle macros from epoch 2015 that are used in a crate from epoch 2018?

@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-core Relevant to the core team, which will review and decide on the PR/issue. labels Sep 14, 2017
@TimNN TimNN added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Sep 17, 2017
@epage
Copy link
Contributor

epage commented Jan 30, 2018

I think there would be a lot of value to leaving warning groups unchanged during an epoch (see my rust2018 post).

Would the appropriate venue for that to be discussed here to be added to this RFC or does it need its own RFC?

High level thoughts

  • Warning group doesn't change during epoch
    • No new warnings added
    • If a warning absolutely needs to be removed (say implementation details like Chalk change the warning enough not to be the same) we make it a no-op
  • Warning group -unstable variant is what the group will be in the future epoch
    • Warnings added as-needed
    • Warnings removed as-needed

@Manishearth
Copy link
Member

Unless someone gets to it first, I plan on doing most of the work here sometime in february. It involves:

  • cargo logic for accepting epochs in cargo.toml and passing them down
  • crates.io logic for validating epoch flags and ensuring they're part of a set of allowed flags (so you can't publish a 2018-pre crate or whatever)
  • rustc logic for setting epoches, and flipping features based on them. I will probably make Tracking issue for future-incompatibility lint tyvar_behind_raw_pointer #46906 my guinea pig for an epoch-gated error

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 30, 2018

@Manishearth glad to see you volunteering. =) Writing up a concrete plan and mentoring instructions has been on my to-do list for quite some time. I think the work items ought to be roughly as follows:

  • Add a -Zepoch=XXX switch to the compiler.
    • This will eventually be --epoch=XXX or -C epoch=XXX or something, but starting with -Z because it's unstable.
    • This will set some flags in the session or what have you, so that you can easily do if tcx.features.rust_2018 { ... }. We might want some helpers for checking "ranges" of epochs (i.e., "sometimes before rust-2018 vs sometime after?).
    • There will be a list of declared epochs and their status, probably starting with 2015 and 2018.
      • 2015 should be "stable". Stable epochs can be used on stable (except that the flag itself is unstable, but that's a separate check.)
      • 2018 would be "unstable". Unstable epochs may only be used on nightly.
  • Add a epoch = XXX thing to cargo, which is only usable on nightly, and which triggers this flag to be passed to the compiler.
    • Once the epoch flag stabilizes, we will want some infrastructure for declaring
  • I don't know what crates.io logic is required here. Just checking that things build probably suffices, right? (Since a bad epoch would fail to build?)

That itself is some pretty simple stuff, but it lays the groundwork. Then, yes, we can for example gate the #46906 error.

The idea of a 2018-pre epoch can be fit into this, although it's not entirely clear to me if it is needed. It's certainly not technically needed: we could just have a 2018 epoch. It can be allowed on stable as soon as the full set of deprecations are known. The main problem there is that it might confuse people if we allow you to do --epoch = 2018 on stable before the epoch is all done. But we can worry about this later. (Anyway, since the epoch isn't really going to alter much in the behavior of the compiler, it probably doesn't matter much -- that is, there might be a few features gated on the epoch, but most stuff is going to be available before, and the epoch is mostly about being stricter about a few deprecations.)

@Manishearth
Copy link
Member

Cool. This roughly aligns with my plan for it. I was considering a -pre epoch mostly so that we could experiment without confusing people but you're right that we can just make all new epochs nightly-only till they stabilize.

@Manishearth
Copy link
Member

I wonder -- did we ever decide how this works with macros? I guess macro expansions belong to the epoch of the crate the macro was defined in, but that can get icky to precisely define. Also a backcompat hazard -- what if the actual problematic code comes from code originally specified by the macro caller in the macro arguments? What if the problematic code comes from a combination of the two?

Macro expansions being in the expanded crate's epoch is a rather simple backcompat hazard.

I'm reminded of Clippy's macro check issue -- we want to determine if the code is yours or from a macro expansion, so that we can avoid warning in the expansion case. We end up basically bailing for non-local macros. The problem is that this misses code like wrap_code!(bad code here) if wrap_code is from a different crate because even though we wrote the bad code it turns up in the expansion of that macro and there's no straightforward way of tying it back.

We really need to figure this out before shipping epochs. It's listed as an unresolved question in the RFC right now.

@nikomatsakis
Copy link
Contributor

I wonder -- did we ever decide how this works with macros?

Good point, I think we "deferred" that =)

I guess macro expansions belong to the epoch of the crate the macro was defined in, but that can get icky to precisely define.

Seems like we will want this to be based on the span, basically same as hygiene. Not that I really know how that works precisely. Still, it complicates things quite a bit, since it wouldn't be a global switch.

@Manishearth
Copy link
Member

@nrc @jseyfried thoughts?

bors added a commit to rust-lang/cargo that referenced this issue Feb 6, 2018
… r=alexcrichton

Implement RFC 2052: Epoches

Todo:

 - Make epoches affect the fingerprint
 - Tests

cc rust-lang/rust#44581

Rust PR: rust-lang/rust#48014

r? @acrichto
Manishearth added a commit to Manishearth/rust that referenced this issue Feb 7, 2018
…e-internet, r=nikomatsakis

Implement RFC 2052 (Epochs)

This adds -Zepochs and uses it for tyvar_behind_raw_pointer (rust-lang#46906)

When we move this to --epoch=XXX, we'll need to gate the 2018 epoch on nightly, but not the 2015 one. I can make these changes here itself though it's kinda pointless given that the entire flag is nightly-only.

r? @nikomatsakis @aturon

cc rust-lang#44581 (epoch tracking)
cc rust-lang#46906 (tyvar_behind_raw_pointer)
@pravic
Copy link
Contributor

pravic commented Feb 15, 2018

According to the RFC:

cargo new will produce a Cargo.toml with the latest epoch value (including an epoch currently in its preview period).

Isn't it better to add the corresponding command line option for cargo too?
For example, if I want to stick with a specific epoch for a while, I will need to manually edit all new Cargo.toml. Or I need to use only the specific toolchain.

I think it would not hurt to add that option for convenience:

cargo new --bin --epoch 2015 myapp.

@SimonSapin
Copy link
Contributor

Why would new crates opt into an older epoch? There isn’t any code yet to keep compatibility with.

@retep998
Copy link
Member

Maybe that new crate specifically wants to be buildable by older versions of Rust? Not everyone is always using the latest bleeding edge version of Rust.

@nikomatsakis
Copy link
Contributor

@epage

I think there would be a lot of value to leaving warning groups unchanged during an epoch (see my rust2018 post).

Sorry nobody responded to this. I'm torn about this question.

On the one hand, I quite like the compromise we have setup now. I like that when you build dependencies, lints are just silenced. But I can put #[deny(warnings)] in my own code and I get all the latest warnings -- including the new ones that were recently added! That is, if I write deny(warnings), it's because I want the latest warnings.

On the other hand, I see that this can be a difficult situation for maintainers. You might have a situation where your 'master' is working fine one day, and then we add a new kind of lint and suddenly it's failing. For that, having fixed sets of names can be useful.

On the gripping hand, that same scenario can occur (and has multiple times) without new suites of lints. We often fix bugs in existing lints so that they catch cases they were missing before, and that is the same sort of problem.

So I'm not entirely sure what the right combination is here. Maybe writing up an RFC is a good idea, to solicit broader feedback.

@steveklabnik
Copy link
Member

steveklabnik commented Feb 15, 2018 via email

@retep998
Copy link
Member

My personal opinion is that lints should be allowed to evolve independently of epochs. Epochs should really only be used for things that are hard breaking changes.

@epage
Copy link
Contributor

epage commented Feb 15, 2018

So I'm not entirely sure what the right combination is here. Maybe writing up an RFC is a good idea, to solicit broader feedback.

Thanks! Will do.

Do we have any current policy or practice or warning compatibility? I feel like referencing that within my RFC would be helpful.

My personal opinion is that lints should be allowed to evolve independently of epochs. Epochs should really only be used for things that are hard breaking changes.

Could you elaborate how what ways lints need to be able to evolve and how maintaining compatibility on lint groups within epochs could hinder that?

Things I can think of

  • New or Removing lints
  • Expanding / narrowing lint scope
    • As in covering more or fewer cases than originally scoped
    • How often does this happen vs creating a new lint for them?
  • Bug fixes within lints, causing them to hit more cases

@Lymia
Copy link
Contributor

Lymia commented Mar 1, 2018

Was there ever an expectation that warnings would be stable over time? It sounds like a generic #[deny(warnings)] is basically inherently asking for your crate to break eventually in the first place and thus should be considered bad practice.

@SimonSapin
Copy link
Contributor

FWIW in Servo and Firefox we’ve been bitten by #[deny(warnings)] and have switched to using RUSTFLAGS="-Dwarnings" in CI.

@SimonSapin
Copy link
Contributor

Note though that if you have existing code using #[deny(warnings)] and broken by new/updated warnings, you can use RUSTFLAGS="--cap-lints=allow" to still be able to build.

@steveklabnik
Copy link
Member

It sounds like a generic #[deny(warnings)] is basically inherently asking for your crate to break eventually in the first place and thus should be considered bad practice.

This is a pretty huge, old debate, even outside of Rust. It's Complicated.

@Manishearth
Copy link
Member

The current stance is that warnings are not stable; code is not stable under deny(warnings).

However, dependencies get built with --cap-lints=allow so this cannot break anything other than crates you maintain.

@Centril Centril added the WG-epoch Working group: Epoch (2018) management label Mar 6, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 17, 2018

The current documentation in the cargo book (https://doc.rust-lang.org/beta/cargo/reference/unstable.html#edition) says that

You need to include the appropriate cargo-features.

But we are having problems in packed_simd about this because the latest nightly says (rust-lang/packed_simd#174 (comment)):

warning: the cargo feature edition is now stable and is no longer necessary to be listed in the manifest

@ehuss
Copy link
Contributor

ehuss commented Sep 17, 2018

@gnzlbg The edition field has been stabilized on nightly. Check the nightly documentation for the most up-to-date information: https://doc.rust-lang.org/nightly/cargo/reference/manifest.html#the-edition-field-optional

@jonas-schievink
Copy link
Contributor

Is there anything left to do for "Adjust documentation"? The book mentions them and links to the edition guide as well.

Other than that, this feature has long been stabilized (although as "editions", not "epochs"), so this should probably be closed.

@jonas-schievink jonas-schievink added the B-RFC-implemented Blocker: Approved by a merged RFC and implemented. label Mar 24, 2019
@Manishearth
Copy link
Member

Yep, this seems done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. B-RFC-implemented Blocker: Approved by a merged RFC and implemented. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-core Relevant to the core team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

No branches or pull requests