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

Remove gensym in format_args #63114

Merged
merged 3 commits into from
Aug 9, 2019
Merged

Conversation

matthewjasper
Copy link
Contributor

This also fixes some things to allow us to export opaque macros from libcore:

  • Don't consider items that are only reachable through opaque macros as public/exported (so they aren't linted as needing docs)
  • Mark private items reachable from the root of libcore as unstable - they are now reachable (in principle) in other crates via macros in libcore

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Jul 29, 2019

Oh, this is interesting.
This is a start/part of a larger work on converting built-in macros from semi-transparent to opaque.

It would be good to implement some pre-requisites before doing that.
I'm really bad at creating issues for this kind of stuff, so it always ends up as one-line notes in my private todo list.

  • First, the unstable annotations on private items you had to add.
    I wanted to avoid that.
    We already have a type privacy system that ensures that non-pub entities cannot be used from other crates in any "non-ephemeral" ways (affecting codegen or linkage).

    So, I think that we can raise reachability only for pub items in EmbargoVisitor::visit_macro_def.
    Any other private entities that can be reached through an opaque macro are "ephemeral" (modules, imports) and not marking them as reachable should have zero effect on other crates in theory.

@petrochenkov
Copy link
Contributor

  • Second, built-in macros are currently AST-based (even if it's not necessary for most of them) and are built using syntax::ext::build::AstBuilder, which is notoriously bad at keeping spans.
    It often just drops the ctxt part of the spans passed to it and uses SyntaxContext::empty() instead, losing all the hygiene, opaque or not.

    So, I thought, perhaps instead of fixing that obsolete piece of code, which AstBuilder is, perhaps we need to clone the proc_macro::quote macro into the compiler, modify it to produce syntax::tokenstream tokens stream rather than proc_macro token streams and use it to implement the built-in macros migrating to the token-based model simultaneously with opaque hygiene.

    That approach is also not without problems, e.g. we still don't have a token-based equivalent of ExprKind::Err for good error recovery.
    So it may be better to fix the parts of AstBuilder used by the converted macros, as a short-term measure.

@petrochenkov
Copy link
Contributor

So, I think the problem with #![unstable] should be solved before we start converting macros from semi-transparent to opaque, but the problems with AstBuilder can be fixed in one-by-one fashion if they arise (#63146 fixes one of them, btw).

@petrochenkov
Copy link
Contributor

Otherwise, LGTM.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2019
@matthewjasper
Copy link
Contributor Author

OK, so I've have a local rework of the handling of reachability through macros and ran into this case:

#![feature(decl_macro)]

mod n {
    pub struct B(pub(crate) p::C);
    impl B {
        pub fn new() -> Self {
            B(p::C)
        }
    }
    mod p {
        pub struct C;

        impl C {
            pub fn foo(&self) -> i32 {
                33
            }
        }
    }
}

// This macro can (currently) be called cross-crate
pub macro m() {
    n::B::new().0.foo()
}

I'm currently handling this by making the field of B reachable, but this would also make the fields of RangeInclusive in libcore reachable, and need stability attributes. The alternative would be to change field privacy to be unhygienic.

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 4, 2019
@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 4, 2019

Fields are also "ephemeral", but can link to non-ephemeral things like new reachable types with public methods.

The situation is pretty similar to modules, I think.

mod m {
    pub struct Z;
    impl Z {
        pub fn public() {}
    }
}
pub struct S {
    field: m::Z,
}

Both module m and field f are private and therefore not "reachable themselves", but they are reachable in the sense that their "contents" need to be visited and marked as reachable.
Is it possible to visit fields' and modules' contents to mark them reachable without marking the fields and modules themselves as reachable?

(An alternative would be to use two flags like "reachable for propagation" and "reachable for checking" or something.)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2019
@petrochenkov
Copy link
Contributor

Actually, it may be possible to just calculate reachability in two passes - first reach everything ignoring nominal visibilities like it's done now, then visit everything again and reset anything with non-pub nominal visibility to unreachable.
That leaves impls though, which don't have nominal visibility, so we'll have to recalculate impl reachability, which in its turn may render something else unreachable, leading to the conclusion that one (iterative) pass probably was a better idea.
Hmm.

* Allow items to be accessible through private modules and fields when a
  macro can access them.
* Don't mark type-private items as reachable.
* Never make items exported/public via macros
They were reachable through opaque macros defined in `core`
@petrochenkov
Copy link
Contributor

@matthewjasper
Is this ready? (Looks ready, but the label is still "waiting-on-author".)

@matthewjasper matthewjasper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2019
@matthewjasper
Copy link
Contributor Author

Yes, it's ready.

@petrochenkov
Copy link
Contributor

@bors r+

Sorry for the delay, I wanted to write some comments, but all of them are non-blocking, so I'll just write them later.

@bors
Copy link
Contributor

bors commented Aug 9, 2019

📌 Commit d9d9246 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 9, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 9, 2019
… r=petrochenkov

Remove gensym in format_args

This also fixes some things to allow us to export opaque macros from libcore:

* Don't consider items that are only reachable through opaque macros as public/exported (so they aren't linted as needing docs)
* Mark private items reachable from the root of libcore as unstable - they are now reachable (in principle) in other crates via macros in libcore

r? @petrochenkov
bors added a commit that referenced this pull request Aug 9, 2019
Rollup of 7 pull requests

Successful merges:

 - #62672 (Deprecate `try!` macro)
 - #62950 (Check rustbook links on all platforms when running locally)
 - #63114 (Remove gensym in format_args)
 - #63397 (Add tests for some ICEs)
 - #63403 (Improve test output)
 - #63404 (enable flt2dec tests in Miri)
 - #63407 (reduce some test sizes in Miri)

Failed merges:

r? @ghost
// No type privacy, so can be directly marked as reachable.
DefKind::Const
| DefKind::Macro(_)
| DefKind::Static
Copy link
Contributor

Choose a reason for hiding this comment

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

Statics are actually protected by a kind of extension to type privacy, since they are real items existing during linking:

// Additionally, until better reachability analysis for macros 2.0 is available,
// we prohibit access to private statics from other crates, this allows to give
// more code internal visibility at link time. (Access to private functions
// is already prohibited by type privacy for function types.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Other things in this list are also "ephemeral" and we need to "reach through" them rather than mark them as reachable themselves.
This optimization probably not too important though (no examples in libcore) and it would be better to somehow introduce the distinction between "reach including" and "reach excluding" nodes in the reachability visitor in general rather than in its macro part only.

@@ -826,7 +948,12 @@ impl DefIdVisitor<'tcx> for ReachEverythingInTheInterfaceVisitor<'_, 'tcx> {
fn tcx(&self) -> TyCtxt<'tcx> { self.ev.tcx }
fn visit_def_id(&mut self, def_id: DefId, _kind: &str, _descr: &dyn fmt::Display) -> bool {
if let Some(hir_id) = self.ev.tcx.hir().as_local_hir_id(def_id) {
self.ev.update(hir_id, self.access_level);
if let ((ty::Visibility::Public, ..), _)
| (_, Some(AccessLevel::ReachableFromImplTrait))
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the consequence is that we don't mark Priv as reachable in situations like this:

pub fn foo()  where Priv: Condition { ... }

This case is not covered by type privacy and foo can be used from other crates.
Is Priv required to be reachable at link time in this case? I think no, so what this PR does is ok.
Anyway, if something breaks we'll get bug reports and add them into the test suite.

@bors bors merged commit d9d9246 into rust-lang:master Aug 9, 2019
@matthewjasper matthewjasper deleted the hygienic-format-args branch August 9, 2019 16:55
Centril added a commit to Centril/rust that referenced this pull request Aug 21, 2019
Fix nested eager expansions in arguments of `format_args`

Fixes rust-lang#63460
Fixes rust-lang#63685 (regression from making `format_args` opaque - rust-lang#63114)

r? @matthewjasper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants