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

Rework stability annotation pass #29083

Merged
merged 4 commits into from
Nov 18, 2015
Merged

Conversation

petrochenkov
Copy link
Contributor

What this patch does:

  • Stability annotations are now based on "exported items" supplied by rustc_privacy and not "public items". Exported items are as accessible for external crates as directly public items and should be annotated with stability attributes.
  • Trait impls require annotations now.
  • Reexports require annotations now.
  • Crates themselves didn't require annotations, now they do.
  • Exported macros are annotated now, but these annotations are not used yet.
  • Some useless annotations are detected and result in errors
  • Finally, some small bugs are fixed - deprecation propagates from stable deprecated parents, items in blocks are traversed correctly (fixes Stability attributes are required for some inaccessible items #29034) + some code cleanup.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

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

@alexcrichton
Copy link
Member

Should be good for a rebase now!

@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov
Copy link
Contributor Author

Oh, wait.
Making unstability inherited only from immediate parents was a bad idea after all. This way annotation of trait impls and their items will require huge work. I'll revert it tomorrow.

@petrochenkov
Copy link
Contributor Author

Disregard the previous message. Everything will have to be annotated anyway when libcore root is stabilized. Better not to wait until that moment.

@alexcrichton
Copy link
Member

I'm a little wary about using exported items because as you've found it's pretty lossy and requires annotating a little too much, but can you elaborate on how it deals with trait items differently? I'm always a little hazy on the difference between public and exported items, but it may just be a bug that trait items aren't listed as public.

#[doc(hidden)]
#[unstable(feature = "libstd_sys_internals",
reason = "this struct is considered exported for some reason",
issue = "0")]
Copy link
Member

Choose a reason for hiding this comment

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

This is actually intended to be stable (good that this PR caught the bug!), so could you tag it as stable since 1.0.0?

@alexcrichton
Copy link
Member

Also starting a crater run to see what the impact is (hopefully small!)

@alexcrichton
Copy link
Member

Crater reports six regressions, 4 of which are legitimate and 2 of which seem unrelated but also legit?

@petrochenkov
Copy link
Contributor Author

Sorry for the delay.

Crater reports six regressions, 4 of which are legitimate and 2 of which seem unrelated but also legit?

4 legitimately regressed crates are on unstable. Unstable Fn::call and Peekable::is_empty were guarded by feature(core), now they are guarded by their own features - fn_call and iter_peekable. I guess it's an acceptable breakage, but it's possible to annotate them with feature(core) to avoid it.

2 unrelated regressions look... completely unrelated.

@petrochenkov
Copy link
Contributor Author

I'm a little wary about using exported items because as you've found it's pretty lossy and requires annotating a little too much, but can you elaborate on how it deals with trait items differently? I'm always a little hazy on the difference between public and exported items, but it may just be a bug that trait items aren't listed as public.

Trait items, impls and impl items are not public, but exported, so they can be left without annotations and therefore lead to "use of unmarked library feature" errors. We don't see these errors now, because the checking pass doesn't look at impls and impl items currently and all trait items have explicit or inherited stability by luck.

The annotation overhead from using exported set is not so serious. It just looks large here, because everything is gathered in one commit. We have approximately 4000 annotated items and only 12 of them are "spurious". Statistically, almost nothing.

On the other hand, it's certainly possible to extend the public set with exported trait items, impls and impl items. In fact, this was the first thing I did! But then I realized three things - 1) after this adjustment public set becomes suspiciously similar to the exported set, 2) I don't actually understand what the public set is supposed to be. 3) I'm still not sure if non-public exported traits (or their items, more importantly) need stability or not. So, I decided not to disrupt the public set with my extra additions, but to use the exported set instead. (It's interesting, that stability annotation pass was actually the only user of the public set besides rustdoc, everything else uses the exported set).

@petrochenkov
Copy link
Contributor Author

In principle, annotation pass can build its own node set, or use exported set for everything except for traits and public set for traits, but it would be additional cruft for little gain, IMO.

@petrochenkov
Copy link
Contributor Author

I'm currently annotating trait impls and various pieces of internal code keep requiring annotations :(
I have an impression now, that extending the public set does make more sense, but not with all exported impls, but only with impls with public trait and self type. Otherwise methods of these impls can't be explicitly called from external code (modulo privacy bugs).

@petrochenkov
Copy link
Contributor Author

Updated with annotations on trait impls.
Okay, now I'm going to extend the public node set and use it to get rid of the annotations on internal items.

@petrochenkov
Copy link
Contributor Author

Blocked on #29291

@bors
Copy link
Contributor

bors commented Oct 25, 2015

☔ The latest upstream changes (presumably #29254) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton alexcrichton added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 2, 2015
bors added a commit that referenced this pull request Nov 2, 2015
The public set is expanded with trait items, impls and their items, foreign items, exported macros, variant fields, i.e. all the missing parts. Now it's a subset of the exported set.
This is needed for #29083 because stability annotation pass uses the public set and all things listed above need to be annotated.
Rustdoc can now be migrated to the public set as well, I guess.

Exported set is now slightly more correct with regard to exported items in blocks - 1) blocks in foreign items are considered and 2) publicity is not inherited from the block's parent - if a function is public it doesn't mean structures defined in its body are public.

r? @alexcrichton or maybe someone else
@alexcrichton
Copy link
Member

@petrochenkov do you want to rebase this now that #29291 has landed?

@petrochenkov
Copy link
Contributor Author

Updated.

@alexcrichton
Copy link
Member

@bors: r+ 8357584

@bors
Copy link
Contributor

bors commented Nov 18, 2015

⌛ Testing commit 8357584 with merge aa7132f...

@bors
Copy link
Contributor

bors commented Nov 18, 2015

💔 Test failed - auto-win-gnu-32-opt

@petrochenkov
Copy link
Contributor Author

Updated.

@alexcrichton
Copy link
Member

@bors: r+ e938e8ea2eaf9093bb92abe25f550a48a4288441

@bors
Copy link
Contributor

bors commented Nov 18, 2015

⌛ Testing commit e938e8e with merge 9514f21...

@bors
Copy link
Contributor

bors commented Nov 18, 2015

💔 Test failed - auto-linux-64-x-android-t

@petrochenkov
Copy link
Contributor Author

Updated.

@alexcrichton
Copy link
Member

@bors: r+

On Wed, Nov 18, 2015 at 10:17 AM, Vadim Petrochenkov <
notifications@github.com> wrote:

Updated.


Reply to this email directly or view it on GitHub
#29083 (comment).

@bors
Copy link
Contributor

bors commented Nov 18, 2015

📌 Commit 64b90f8 has been approved by alexcrichton

bors added a commit that referenced this pull request Nov 18, 2015
What this patch does:
- Stability annotations are now based on "exported items" supplied by rustc_privacy and not "public items". Exported items are as accessible for external crates as directly public items and should be annotated with stability attributes.
- Trait impls require annotations now.
- Reexports require annotations now.
- Crates themselves didn't require annotations, now they do.
- Exported macros are annotated now, but these annotations are not used yet.
- Some useless annotations are detected and result in errors
- Finally, some small bugs are fixed - deprecation propagates from stable deprecated parents, items in blocks are traversed correctly (fixes #29034) + some code cleanup.
@bors
Copy link
Contributor

bors commented Nov 18, 2015

⌛ Testing commit 64b90f8 with merge 22e31f1...

@bors bors merged commit 64b90f8 into rust-lang:master Nov 18, 2015
@petrochenkov petrochenkov deleted the stability3 branch November 22, 2015 11:40
petrochenkov referenced this pull request in petrochenkov/rust Dec 14, 2015
bors added a commit that referenced this pull request Jan 15, 2016
This wasn't done in #29083 because attributes weren't parsed on fields of tuple variant back then.

r? @alexcrichton
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.

Stability attributes are required for some inaccessible items
4 participants