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

Expansion-driven outline module parsing #69838

Merged
merged 26 commits into from
Mar 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
bc75cba
submod_path_from_attr: simplify & document
Centril Mar 7, 2020
2899a58
extract error_cannot_declare_mod_here
Centril Mar 7, 2020
185c1d3
extract error_decl_mod_in_block
Centril Mar 7, 2020
2db5d49
simplify submod_path
Centril Mar 7, 2020
803de31
submod_path: use id.span
Centril Mar 7, 2020
7108b7f
extract parse_mod
Centril Mar 7, 2020
dfcefa4
extract error_on_circular_module
Centril Mar 7, 2020
ca098b1
detach submod_path from Parser
Centril Mar 8, 2020
53a633f
decouple push_directory from Parser
Centril Mar 8, 2020
98e71cd
decouple eval_src_mod from Parser
Centril Mar 8, 2020
b9e1b26
expand: use push_directory
Centril Mar 8, 2020
8bab88f
de-fatalize outline module parsing
Centril Mar 8, 2020
463995f
extract parse_external_module
Centril Mar 8, 2020
59bf8a0
extract error_on_circular_module
Centril Mar 8, 2020
83a757a
outline modules: parse -> expand.
Centril Mar 8, 2020
f1ca996
parse: module parsing -> item.rs
Centril Mar 8, 2020
ddcc8ec
move Directory -> parser::module
Centril Mar 8, 2020
3796fae
{rustc_parse::parser -> rustc_expand}::module
Centril Mar 8, 2020
31ee8e0
{rustc_parse -> rustc_expand}::config
Centril Mar 8, 2020
a6cb04f
add test for stripped nested outline module
Centril Mar 8, 2020
7d0e5bb
parser/expand: minor cleanup
Centril Mar 9, 2020
fe71342
tweak outline module parsing spans
Centril Mar 9, 2020
41a0b3e
use pretty-compare-only in a test
Centril Mar 10, 2020
5ee4f6f
fix pre-expansion linting infra
Centril Mar 15, 2020
e301291
fix rebase fallout
Centril Mar 15, 2020
8caf688
--bless windows test
Centril Mar 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/libpanic_unwind/emcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
//! Emscripten's runtime always implements those APIs and does not
//! implement libunwind.

#![allow(private_no_mangle_fns)]

use alloc::boxed::Box;
use core::any::Any;
use core::mem;
Expand Down
2 changes: 0 additions & 2 deletions src/libpanic_unwind/gcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@
//! Once stack has been unwound down to the handler frame level, unwinding stops
//! and the last personality routine transfers control to the catch block.

#![allow(private_no_mangle_fns)]

use alloc::boxed::Box;
use core::any::Any;

Expand Down
1 change: 0 additions & 1 deletion src/libpanic_unwind/seh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
//! [llvm]: http://llvm.org/docs/ExceptionHandling.html#background-on-windows-exceptions

#![allow(nonstandard_style)]
#![allow(private_no_mangle_fns)]

use alloc::boxed::Box;
use core::any::Any;
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_ast/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2153,7 +2153,7 @@ impl FnRetTy {
/// Module declaration.
///
/// E.g., `mod foo;` or `mod foo { .. }`.
#[derive(Clone, RustcEncodable, RustcDecodable, Debug)]
#[derive(Clone, RustcEncodable, RustcDecodable, Debug, Default)]
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
pub struct Mod {
/// A span from the first token past `{` to the last token until `}`.
/// For `mod foo;`, the inner span ranges from the first token
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_builtin_macros/proc_macro_harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub fn inject(
handler: &rustc_errors::Handler,
) -> ast::Crate {
let ecfg = ExpansionConfig::default("proc_macro".to_string());
let mut cx = ExtCtxt::new(sess, ecfg, resolver);
let mut cx = ExtCtxt::new(sess, ecfg, resolver, None);

let mut collect = CollectProcMacros {
macros: Vec::new(),
Expand Down
5 changes: 2 additions & 3 deletions src/librustc_builtin_macros/source_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use rustc_ast::tokenstream::TokenStream;
use rustc_ast_pretty::pprust;
use rustc_expand::base::{self, *};
use rustc_expand::panictry;
use rustc_parse::{self, new_sub_parser_from_file, parser::Parser, DirectoryOwnership};
use rustc_parse::{self, new_sub_parser_from_file, parser::Parser};
use rustc_session::lint::builtin::INCOMPLETE_INCLUDE;
use rustc_span::symbol::Symbol;
use rustc_span::{self, Pos, Span};
Expand Down Expand Up @@ -108,8 +108,7 @@ pub fn expand_include<'cx>(
return DummyResult::any(sp);
}
};
let directory_ownership = DirectoryOwnership::Owned { relative: None };
let p = new_sub_parser_from_file(cx.parse_sess(), &file, directory_ownership, None, sp);
let p = new_sub_parser_from_file(cx.parse_sess(), &file, None, sp);
Copy link
Member

@eddyb eddyb Mar 19, 2020

Choose a reason for hiding this comment

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

This apparently broke Servo's script crate, or at least the copy on perf.rust-lang.org.
cc @Mark-Simulacrum can you post some details?

IIUC, this can't be fixed locally either, because the mod bar; parsed by include!("directory/included.rs"); will be expanded later, so for expanding mod bar; to load directory/bar.rs (which is apparently the old behavior? how are we not testing for it), we'd need to detect that an include! happened, or something similar.

EDIT: adjusted example to fit @Mark-Simulacrum's reproduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 a minimal reproduction:

$ tree .
.
|-- directory
|   |-- bar.rs
|   `-- included.rs
`-- foo.rs
// foo.rs
include!("directory/included.rs");

fn main() {}

// directory/included.rs
mod bar;

// directory/bar.rs
// empty
$ rustc +3c6f982cc908aacc39c3ac97f31c989f81cc213c foo.rs
error[E0583]: file not found for module `bar`
 --> directory/included.rs:1:1
  |
1 | mod bar;
  | ^^^^^^^^
  |
  = help: to create the module `bar`, create file "bar.rs"

error: aborting due to previous error

For more information about this error, try `rustc --explain E0583`.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just recalled a related issue - #58818.
Could you check whether this PR fixed it?

Copy link
Contributor

Choose a reason for hiding this comment

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

we'd need to detect that an include! happened, or something similar.

FWIW, we have Definitions::expansions_that_defined that can link from the mod bar; item to the macro that produced it. Then we have ExpnData::parent that can link from that macro to the macro that produced it (recursively).

Perhaps using these data we can hack up something reproducing the old behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can only detect include comparing the macro's name in ExpnData with sym::include.
fn expansion_cause used in for a bunch of various stuff also hard-codes the name include like this, so it should be ok for now.

(The proper fix is a flag in struct SyntaxExtension, other file-including built-in macros should have it as well.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@petrochenkov So I tried two different approaches to set ownership before loading the external module. The first one was adding Incovation::is_current_expansion_macro_named defined like so:

    fn is_current_expansion_macro_named(&self, sym: Symbol) -> bool {
        let mut curr = self.cx.current_expansion.id;
        loop {
            let expn_data = curr.expn_data();
            if expn_data.kind == ExpnKind::Macro(MacroKind::Bang, sym) {
                // Stop going up the backtrace once `sym` is encountered.
                return true;
            }
            if expn_data.is_root() {
                // Reached root, need to bail to ensure termination.
                return false;
            }
            curr = expn_data.call_site.ctxt().outer_expn();
        }
    }

and then using it in flat_map_item before parse_external_mod happens:

                let pushed = &mut false; // Record `parse_external_mod` pushing so we can pop.
                let mut dir = Directory { ownership: orig_ownership, path: module.directory };

                if self.is_current_expansion_macro_named(sym::include) {
                    log::debug!("flat_map_item, is_current_expansion_macro_named = true");
                    dir.ownership = DirectoryOwnership::Owned { relative: None };
                }

The log message here does trigger on @Mark-Simulacrum's regression test, but this doesn't fix the issue.

I also tweaked SyntaxExtension, adding a flag foobar: bool which is used in fully_expand_fragment:

            let (expanded_fragment, new_invocations) = match res {
                InvocationRes::Single(ext) => match self.expand_invoc(invoc, &ext.kind) {
                    ExpandResult::Ready(fragment) => {
                        if ext.foobar {
                            self.cx.current_expansion.directory_ownership =
                                DirectoryOwnership::Owned { relative: None };
                            log::debug!("fully_expand_fragment, using foobar");
                        }
                        self.collect_invocations(fragment, &[])

and which is set in compile_macro:

        if result.is_builtin {
            // The macro was marked with `#[rustc_builtin_macro]`.
            if let Some(ext) = self.builtin_macros.remove(&item.ident.name) {
                // The macro is a built-in, replace its expander function
                // while still taking everything else from the source code.
                result.kind = ext.kind;

                if item.ident.name == sym::include {
                    result.foobar = true;
                    log::debug!("compile_macro, setting foobar");
                }

The log message also triggers here, but also does nothing to fix the issue, as the approach is the same as the other mechanism.

I'm not sure why it doesn't work. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found a fix, PR up at #70184.


struct ExpandResult<'a> {
p: Parser<'a>,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_builtin_macros/standard_library_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ pub fn inject(
let call_site = DUMMY_SP.with_call_site_ctxt(expn_id);

let ecfg = ExpansionConfig::default("std_lib_injection".to_string());
let cx = ExtCtxt::new(sess, ecfg, resolver);
let cx = ExtCtxt::new(sess, ecfg, resolver, None);

// .rev() to preserve ordering above in combination with insert(0, ...)
for &name in names.iter().rev() {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_builtin_macros/test_harness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ fn generate_test_harness(
let mut econfig = ExpansionConfig::default("test".to_string());
econfig.features = Some(features);

let ext_cx = ExtCtxt::new(sess, econfig, resolver);
let ext_cx = ExtCtxt::new(sess, econfig, resolver, None);

let expn_id = ext_cx.resolver.expansion_for_ast_pass(
DUMMY_SP,
Expand Down
9 changes: 7 additions & 2 deletions src/librustc_expand/base.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::expand::{self, AstFragment, Invocation};
use crate::module::DirectoryOwnership;

use rustc_ast::ast::{self, Attribute, Name, NodeId, PatKind};
use rustc_ast::mut_visit::{self, MutVisitor};
Expand All @@ -10,7 +11,7 @@ use rustc_attr::{self as attr, Deprecation, HasAttrs, Stability};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::sync::{self, Lrc};
use rustc_errors::{DiagnosticBuilder, DiagnosticId};
use rustc_parse::{self, parser, DirectoryOwnership, MACRO_ARGUMENTS};
use rustc_parse::{self, parser, MACRO_ARGUMENTS};
use rustc_session::parse::ParseSess;
use rustc_span::edition::Edition;
use rustc_span::hygiene::{AstPass, ExpnData, ExpnId, ExpnKind};
Expand Down Expand Up @@ -925,19 +926,23 @@ pub struct ExtCtxt<'a> {
pub resolver: &'a mut dyn Resolver,
pub current_expansion: ExpansionData,
pub expansions: FxHashMap<Span, Vec<String>>,
/// Called directly after having parsed an external `mod foo;` in expansion.
pub(super) extern_mod_loaded: Option<&'a dyn Fn(&ast::Crate)>,
}

impl<'a> ExtCtxt<'a> {
pub fn new(
parse_sess: &'a ParseSess,
ecfg: expand::ExpansionConfig<'a>,
resolver: &'a mut dyn Resolver,
extern_mod_loaded: Option<&'a dyn Fn(&ast::Crate)>,
) -> ExtCtxt<'a> {
ExtCtxt {
parse_sess,
ecfg,
root_path: PathBuf::new(),
resolver,
extern_mod_loaded,
root_path: PathBuf::new(),
current_expansion: ExpansionData {
id: ExpnId::root(),
depth: 0,
Expand Down
23 changes: 3 additions & 20 deletions src/librustc_parse/config.rs → src/librustc_expand/config.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
//! Process the potential `cfg` attributes on a module.
//! Also determine if the module should be included in this configuration.
//!
//! This module properly belongs in rustc_expand, but for now it's tied into
//! parsing, so we leave it here to avoid complicated out-of-line dependencies.
//!
//! A principled solution to this wrong location would be to implement [#64197].
//!
//! [#64197]: https://github.com/rust-lang/rust/issues/64197

use crate::{parse_in, validate_attr};
//! Conditional compilation stripping.

use rustc_ast::ast::{self, AttrItem, Attribute, MetaItem};
use rustc_ast::attr::HasAttrs;
use rustc_ast::mut_visit::*;
Expand All @@ -21,6 +12,7 @@ use rustc_feature::{Feature, Features, State as FeatureState};
use rustc_feature::{
ACCEPTED_FEATURES, ACTIVE_FEATURES, REMOVED_FEATURES, STABLE_REMOVED_FEATURES,
};
use rustc_parse::{parse_in, validate_attr};
use rustc_session::parse::{feature_err, ParseSess};
use rustc_span::edition::{Edition, ALL_EDITIONS};
use rustc_span::symbol::{sym, Symbol};
Expand Down Expand Up @@ -538,12 +530,3 @@ impl<'a> MutVisitor for StripUnconfigured<'a> {
fn is_cfg(attr: &Attribute) -> bool {
attr.check_name(sym::cfg)
}

/// Process the potential `cfg` attributes on a module.
/// Also determine if the module should be included in this configuration.
pub fn process_configure_mod(sess: &ParseSess, cfg_mods: bool, attrs: &mut Vec<Attribute>) -> bool {
// Don't perform gated feature checking.
let mut strip_unconfigured = StripUnconfigured { sess, features: None };
strip_unconfigured.process_cfg_attrs(attrs);
!cfg_mods || strip_unconfigured.in_cfg(&attrs)
}
92 changes: 58 additions & 34 deletions src/librustc_expand/expand.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use crate::base::*;
use crate::config::StripUnconfigured;
use crate::configure;
use crate::hygiene::{ExpnData, ExpnId, ExpnKind, SyntaxContext};
use crate::mbe::macro_rules::annotate_err_with_kind;
use crate::module::{parse_external_mod, push_directory, Directory, DirectoryOwnership};
use crate::placeholders::{placeholder, PlaceholderExpander};
use crate::proc_macro::collect_derives;

Expand All @@ -17,10 +19,8 @@ use rustc_ast_pretty::pprust;
use rustc_attr::{self as attr, is_builtin_attr, HasAttrs};
use rustc_errors::{Applicability, FatalError, PResult};
use rustc_feature::Features;
use rustc_parse::configure;
use rustc_parse::parser::Parser;
use rustc_parse::validate_attr;
use rustc_parse::DirectoryOwnership;
use rustc_session::lint::builtin::UNUSED_DOC_COMMENTS;
use rustc_session::lint::BuiltinLintDiagnostics;
use rustc_session::parse::{feature_err, ParseSess};
Expand Down Expand Up @@ -1427,59 +1427,83 @@ impl<'a, 'b> MutVisitor for InvocationCollector<'a, 'b> {
.make_items();
}

let mut attrs = mem::take(&mut item.attrs); // We do this to please borrowck.
let ident = item.ident;
let span = item.span;

match item.kind {
ast::ItemKind::MacCall(..) => {
item.attrs = attrs;
self.check_attributes(&item.attrs);
item.and_then(|item| match item.kind {
ItemKind::MacCall(mac) => self
.collect(
AstFragmentKind::Items,
InvocationKind::Bang { mac, span: item.span },
)
.collect(AstFragmentKind::Items, InvocationKind::Bang { mac, span })
.make_items(),
_ => unreachable!(),
})
}
ast::ItemKind::Mod(ast::Mod { inner, inline, .. })
if item.ident != Ident::invalid() =>
{
let orig_directory_ownership = self.cx.current_expansion.directory_ownership;
ast::ItemKind::Mod(ref mut old_mod @ ast::Mod { .. }) if ident != Ident::invalid() => {
let sess = self.cx.parse_sess;
let orig_ownership = self.cx.current_expansion.directory_ownership;
let mut module = (*self.cx.current_expansion.module).clone();
module.mod_path.push(item.ident);

if inline {
if let Some(path) = attr::first_attr_value_str_by_name(&item.attrs, sym::path) {
self.cx.current_expansion.directory_ownership =
DirectoryOwnership::Owned { relative: None };
module.directory.push(&*path.as_str());
} else {
module.directory.push(&*item.ident.as_str());
}
let pushed = &mut false; // Record `parse_external_mod` pushing so we can pop.
let dir = Directory { ownership: orig_ownership, path: module.directory };
let Directory { ownership, path } = if old_mod.inline {
// Inline `mod foo { ... }`, but we still need to push directories.
item.attrs = attrs;
push_directory(ident, &item.attrs, dir)
} else {
let path = self.cx.parse_sess.source_map().span_to_unmapped_path(inner);
let mut path = match path {
FileName::Real(path) => path,
other => PathBuf::from(other.to_string()),
// We have an outline `mod foo;` so we need to parse the file.
let (new_mod, dir) =
parse_external_mod(sess, ident, span, dir, &mut attrs, pushed);

let krate = ast::Crate {
span: new_mod.inner,
module: new_mod,
attrs,
proc_macros: vec![],
};
let directory_ownership = match path.file_name().unwrap().to_str() {
Some("mod.rs") => DirectoryOwnership::Owned { relative: None },
Some(_) => DirectoryOwnership::Owned { relative: Some(item.ident) },
None => DirectoryOwnership::UnownedViaMod,
if let Some(extern_mod_loaded) = self.cx.extern_mod_loaded {
extern_mod_loaded(&krate);
}

*old_mod = krate.module;
item.attrs = krate.attrs;
// File can have inline attributes, e.g., `#![cfg(...)]` & co. => Reconfigure.
item = match self.configure(item) {
Some(node) => node,
None => {
if *pushed {
sess.included_mod_stack.borrow_mut().pop();
}
return Default::default();
}
};
path.pop();
module.directory = path;
self.cx.current_expansion.directory_ownership = directory_ownership;
}
dir
};

// Set the module info before we flat map.
self.cx.current_expansion.directory_ownership = ownership;
module.directory = path;
module.mod_path.push(ident);
let orig_module =
mem::replace(&mut self.cx.current_expansion.module, Rc::new(module));

let result = noop_flat_map_item(item, self);

// Restore the module info.
self.cx.current_expansion.module = orig_module;
self.cx.current_expansion.directory_ownership = orig_directory_ownership;
self.cx.current_expansion.directory_ownership = orig_ownership;
if *pushed {
sess.included_mod_stack.borrow_mut().pop();
}
result
}

_ => noop_flat_map_item(item, self),
_ => {
item.attrs = attrs;
noop_flat_map_item(item, self)
}
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/librustc_expand/lib.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
#![feature(bool_to_option)]
#![feature(cow_is_borrowed)]
#![feature(crate_visibility_modifier)]
#![feature(decl_macro)]
#![feature(proc_macro_diagnostic)]
#![feature(proc_macro_internals)]
#![feature(proc_macro_span)]
#![feature(try_blocks)]

extern crate proc_macro as pm;

Expand Down Expand Up @@ -33,8 +35,10 @@ pub use mbe::macro_rules::compile_declarative_macro;
crate use rustc_span::hygiene;
pub mod base;
pub mod build;
#[macro_use]
pub mod config;
pub mod expand;
pub use rustc_parse::config;
pub mod module;
pub mod proc_macro;

crate mod mbe;
Expand Down
Loading