Auto merge of #37602 - jseyfried:directory_ownership, r=nikomatsakis

parser: simplify directory ownership semantics

This PR simplifies the semantics of "directory ownership". After this PR,
 - a non-inline module without a `#[path]` attribute (e.g. `mod foo;`) is allowed iff its parent module/block (whichever is nearer) is a directory owner,
 - an non-inline module is a directory owner iff its corresponding file is named `mod.rs` (c.f. [comment](https://github.com/rust-lang/rust/issues/32401#issuecomment-201021902)),
 - a block is never a directory owner (c.f. #31534), and
 - an inline module is a directory owner iff either
   - its parent module/block is a directory owner (again, c.f. #31534), or
   - it has a `#[path]` attribute (c.f. #36789).

These semantics differ from today's in three orthogonal ways:
 - `#[path = "foo.rs"] mod foo;` is no longer a directory owner. This is a [breaking-change].
 - #36789 is generalized to apply to modules that are not directory owners in addition to blocks.
 - A macro-expanded non-inline module is only allowed where an ordinary non-inline module would be allowed. Today, we incorrectly allow macro-expanded non-inline modules in modules that are not directory owners (but not in blocks). This is a [breaking-change].

Fixes #32401.
r? @nikomatsakis
This commit is contained in:
bors 2016-11-21 21:15:48 -06:00 committed by GitHub
commit 1c11ea3aed
20 changed files with 199 additions and 88 deletions

View File

@ -204,6 +204,13 @@ declare_lint! {
"detects extra requirements in impls that were erroneously allowed"
}
declare_lint! {
pub LEGACY_DIRECTORY_OWNERSHIP,
Warn,
"non-inline, non-`#[path]` modules (e.g. `mod foo;`) were erroneously allowed in some files \
not named `mod.rs`"
}
/// Does nothing as a lint pass, but registers some `Lint`s
/// which are used by other parts of the compiler.
#[derive(Copy, Clone)]
@ -242,7 +249,8 @@ impl LintPass for HardwiredLints {
LIFETIME_UNDERSCORE,
SAFE_EXTERN_STATICS,
PATTERNS_IN_FNS_WITHOUT_BODY,
EXTRA_REQUIREMENT_IN_IMPL
EXTRA_REQUIREMENT_IN_IMPL,
LEGACY_DIRECTORY_OWNERSHIP
)
}
}

View File

@ -232,6 +232,10 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
id: LintId::of(EXTRA_REQUIREMENT_IN_IMPL),
reference: "issue #37166 <https://github.com/rust-lang/rust/issues/37166>",
},
FutureIncompatibleInfo {
id: LintId::of(LEGACY_DIRECTORY_OWNERSHIP),
reference: "issue #37872 <https://github.com/rust-lang/rust/issues/37872>",
},
]);
// Register renamed and removed lints

View File

@ -207,6 +207,13 @@ impl<'a> Visitor for AstValidator<'a> {
ItemKind::Mod(_) => {
// Ensure that `path` attributes on modules are recorded as used (c.f. #35584).
attr::first_attr_value_str_by_name(&item.attrs, "path");
if let Some(attr) =
item.attrs.iter().find(|attr| attr.name() == "warn_directory_ownership") {
let lint = lint::builtin::LEGACY_DIRECTORY_OWNERSHIP;
let msg = "cannot declare a new module at this location";
self.session.add_lint(lint, item.id, item.span, msg.to_string());
attr::mark_used(attr);
}
}
ItemKind::Union(ref vdata, _) => {
if !vdata.is_struct() {

View File

@ -18,7 +18,7 @@ use errors::DiagnosticBuilder;
use ext::expand::{self, Expansion};
use ext::hygiene::Mark;
use fold::{self, Folder};
use parse::{self, parser};
use parse::{self, parser, DirectoryOwnership};
use parse::token;
use ptr::P;
use symbol::Symbol;
@ -568,9 +568,7 @@ pub struct ExpansionData {
pub depth: usize,
pub backtrace: ExpnId,
pub module: Rc<ModuleData>,
// True if non-inline modules without a `#[path]` are forbidden at the root of this expansion.
pub no_noninline_mod: bool,
pub directory_ownership: DirectoryOwnership,
}
/// One of these is made during expansion and incrementally updated as we go;
@ -601,7 +599,7 @@ impl<'a> ExtCtxt<'a> {
depth: 0,
backtrace: NO_EXPANSION,
module: Rc::new(ModuleData { mod_path: Vec::new(), directory: PathBuf::new() }),
no_noninline_mod: false,
directory_ownership: DirectoryOwnership::Owned,
},
}
}

View File

@ -21,7 +21,7 @@ use ext::base::*;
use feature_gate::{self, Features};
use fold;
use fold::*;
use parse::{ParseSess, PResult, lexer};
use parse::{ParseSess, DirectoryOwnership, PResult, lexer};
use parse::parser::Parser;
use parse::token;
use print::pprust;
@ -727,9 +727,10 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
}
fn fold_block(&mut self, block: P<Block>) -> P<Block> {
let no_noninline_mod = mem::replace(&mut self.cx.current_expansion.no_noninline_mod, true);
let old_directory_ownership = self.cx.current_expansion.directory_ownership;
self.cx.current_expansion.directory_ownership = DirectoryOwnership::UnownedViaBlock;
let result = noop_fold_block(block, self);
self.cx.current_expansion.no_noninline_mod = no_noninline_mod;
self.cx.current_expansion.directory_ownership = old_directory_ownership;
result
}
@ -768,7 +769,7 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
return noop_fold_item(item, self);
}
let orig_no_noninline_mod = self.cx.current_expansion.no_noninline_mod;
let orig_directory_ownership = self.cx.current_expansion.directory_ownership;
let mut module = (*self.cx.current_expansion.module).clone();
module.mod_path.push(item.ident);
@ -779,23 +780,28 @@ impl<'a, 'b> Folder for InvocationCollector<'a, 'b> {
if inline_module {
if let Some(path) = attr::first_attr_value_str_by_name(&item.attrs, "path") {
self.cx.current_expansion.no_noninline_mod = false;
self.cx.current_expansion.directory_ownership = DirectoryOwnership::Owned;
module.directory.push(&*path.as_str());
} else {
module.directory.push(&*item.ident.name.as_str());
}
} else {
self.cx.current_expansion.no_noninline_mod = false;
module.directory =
let mut path =
PathBuf::from(self.cx.parse_sess.codemap().span_to_filename(inner));
module.directory.pop();
let directory_ownership = match path.file_name().unwrap().to_str() {
Some("mod.rs") => DirectoryOwnership::Owned,
_ => DirectoryOwnership::UnownedViaMod(false),
};
path.pop();
module.directory = path;
self.cx.current_expansion.directory_ownership = directory_ownership;
}
let orig_module =
mem::replace(&mut self.cx.current_expansion.module, Rc::new(module));
let result = noop_fold_item(item, self);
self.cx.current_expansion.module = orig_module;
self.cx.current_expansion.no_noninline_mod = orig_no_noninline_mod;
self.cx.current_expansion.directory_ownership = orig_directory_ownership;
return result;
}
// Ensure that test functions are accessible from the test harness.

View File

@ -13,7 +13,7 @@ use syntax_pos::{self, Pos, Span};
use ext::base::*;
use ext::base;
use ext::build::AstBuilder;
use parse::token;
use parse::{token, DirectoryOwnership};
use parse;
use print::pprust;
use ptr::P;
@ -90,7 +90,8 @@ pub fn expand_include<'cx>(cx: &'cx mut ExtCtxt, sp: Span, tts: &[tokenstream::T
};
// The file will be added to the code map by the parser
let path = res_rel_file(cx, sp, Path::new(&file));
let p = parse::new_sub_parser_from_file(cx.parse_sess(), &path, true, None, sp);
let directory_ownership = DirectoryOwnership::Owned;
let p = parse::new_sub_parser_from_file(cx.parse_sess(), &path, directory_ownership, None, sp);
struct ExpandResult<'a> {
p: parse::parser::Parser<'a>,

View File

@ -19,7 +19,7 @@ use ext::tt::macro_parser::{MatchedSeq, MatchedNonterminal};
use ext::tt::macro_parser::{parse, parse_failure_msg};
use parse::ParseSess;
use parse::lexer::new_tt_reader;
use parse::parser::{Parser, Restrictions};
use parse::parser::Parser;
use parse::token::{self, NtTT, Token};
use parse::token::Token::*;
use print;
@ -117,11 +117,12 @@ fn generic_extension<'cx>(cx: &'cx ExtCtxt,
let trncbr =
new_tt_reader(&cx.parse_sess.span_diagnostic, Some(named_matches), rhs);
let mut p = Parser::new(cx.parse_sess(), Box::new(trncbr));
p.directory = cx.current_expansion.module.directory.clone();
p.restrictions = match cx.current_expansion.no_noninline_mod {
true => Restrictions::NO_NONINLINE_MOD,
false => Restrictions::empty(),
};
let module = &cx.current_expansion.module;
p.directory.path = module.directory.clone();
p.directory.ownership = cx.current_expansion.directory_ownership;
p.root_module_name =
module.mod_path.last().map(|id| (*id.name.as_str()).to_owned());
p.check_unknown_macro_variable();
// Let the context choose how to interpret the result.
// Weird, but useful for X-macros.

View File

@ -76,6 +76,19 @@ impl ParseSess {
}
}
#[derive(Clone)]
pub struct Directory {
pub path: PathBuf,
pub ownership: DirectoryOwnership,
}
#[derive(Copy, Clone)]
pub enum DirectoryOwnership {
Owned,
UnownedViaBlock,
UnownedViaMod(bool /* legacy warnings? */),
}
// a bunch of utility functions of the form parse_<thing>_from_<source>
// where <thing> includes crate, expr, item, stmt, tts, and one that
// uses a HOF to parse anything, and <source> includes file and
@ -152,11 +165,11 @@ pub fn new_parser_from_file<'a>(sess: &'a ParseSess, path: &Path) -> Parser<'a>
/// On an error, use the given span as the source of the problem.
pub fn new_sub_parser_from_file<'a>(sess: &'a ParseSess,
path: &Path,
owns_directory: bool,
directory_ownership: DirectoryOwnership,
module_name: Option<String>,
sp: Span) -> Parser<'a> {
let mut p = filemap_to_parser(sess, file_to_filemap(sess, path, Some(sp)));
p.owns_directory = owns_directory;
p.directory.ownership = directory_ownership;
p.root_module_name = module_name;
p
}

View File

@ -38,7 +38,7 @@ use ast::{Ty, TyKind, TypeBinding, TyParam, TyParamBounds};
use ast::{ViewPath, ViewPathGlob, ViewPathList, ViewPathSimple};
use ast::{Visibility, WhereClause};
use ast::{BinOpKind, UnOp};
use ast;
use {ast, attr};
use codemap::{self, CodeMap, Spanned, spanned, respan};
use syntax_pos::{self, Span, BytePos, mk_sp};
use errors::{self, DiagnosticBuilder};
@ -49,7 +49,7 @@ use parse::common::SeqSep;
use parse::lexer::{Reader, TokenAndSpan};
use parse::obsolete::ObsoleteSyntax;
use parse::token::{self, MatchNt, SubstNt};
use parse::{new_sub_parser_from_file, ParseSess};
use parse::{new_sub_parser_from_file, ParseSess, Directory, DirectoryOwnership};
use util::parser::{AssocOp, Fixity};
use print::pprust;
use ptr::P;
@ -68,7 +68,6 @@ bitflags! {
flags Restrictions: u8 {
const RESTRICTION_STMT_EXPR = 1 << 0,
const RESTRICTION_NO_STRUCT_LITERAL = 1 << 1,
const NO_NONINLINE_MOD = 1 << 2,
}
}
@ -200,12 +199,9 @@ pub struct Parser<'a> {
/// extra detail when the same error is seen twice
pub obsolete_set: HashSet<ObsoleteSyntax>,
/// Used to determine the path to externally loaded source files
pub directory: PathBuf,
pub directory: Directory,
/// Stack of open delimiters and their spans. Used for error message.
pub open_braces: Vec<(token::DelimToken, Span)>,
/// Flag if this parser "owns" the directory that it is currently parsing
/// in. This will affect how nested files are looked up.
pub owns_directory: bool,
/// Name of the root module this parser originated from. If `None`, then the
/// name is not known. This does not change while the parser is descending
/// into modules, and sub-parsers have new values for this name.
@ -245,8 +241,9 @@ pub struct ModulePath {
}
pub struct ModulePathSuccess {
pub path: ::std::path::PathBuf,
pub owns_directory: bool,
pub path: PathBuf,
pub directory_ownership: DirectoryOwnership,
warn: bool,
}
pub struct ModulePathError {
@ -296,9 +293,8 @@ impl<'a> Parser<'a> {
quote_depth: 0,
parsing_token_tree: false,
obsolete_set: HashSet::new(),
directory: PathBuf::new(),
directory: Directory { path: PathBuf::new(), ownership: DirectoryOwnership::Owned },
open_braces: Vec::new(),
owns_directory: true,
root_module_name: None,
expected_tokens: Vec::new(),
tts: Vec::new(),
@ -310,8 +306,8 @@ impl<'a> Parser<'a> {
parser.token = tok.tok;
parser.span = tok.sp;
if parser.span != syntax_pos::DUMMY_SP {
parser.directory = PathBuf::from(sess.codemap().span_to_filename(parser.span));
parser.directory.pop();
parser.directory.path = PathBuf::from(sess.codemap().span_to_filename(parser.span));
parser.directory.path.pop();
}
parser
}
@ -3966,9 +3962,11 @@ impl<'a> Parser<'a> {
}
} else {
// FIXME: Bad copy of attrs
let restrictions = self.restrictions | Restrictions::NO_NONINLINE_MOD;
match self.with_res(restrictions,
|this| this.parse_item_(attrs.clone(), false, true))? {
let old_directory_ownership =
mem::replace(&mut self.directory.ownership, DirectoryOwnership::UnownedViaBlock);
let item = self.parse_item_(attrs.clone(), false, true)?;
self.directory.ownership = old_directory_ownership;
match item {
Some(i) => Stmt {
id: ast::DUMMY_NODE_ID,
span: mk_sp(lo, i.span.hi),
@ -5271,38 +5269,53 @@ impl<'a> Parser<'a> {
self.bump();
if in_cfg {
// This mod is in an external file. Let's go get it!
let (m, attrs) = self.eval_src_mod(id, &outer_attrs, id_span)?;
Ok((id, m, Some(attrs)))
let ModulePathSuccess { path, directory_ownership, warn } =
self.submod_path(id, &outer_attrs, id_span)?;
let (module, mut attrs) =
self.eval_src_mod(path, directory_ownership, id.to_string(), id_span)?;
if warn {
let attr = ast::Attribute {
id: attr::mk_attr_id(),
style: ast::AttrStyle::Outer,
value: ast::MetaItem {
name: Symbol::intern("warn_directory_ownership"),
node: ast::MetaItemKind::Word,
span: syntax_pos::DUMMY_SP,
},
is_sugared_doc: false,
span: syntax_pos::DUMMY_SP,
};
attr::mark_known(&attr);
attrs.push(attr);
}
Ok((id, module, Some(attrs)))
} else {
let placeholder = ast::Mod { inner: syntax_pos::DUMMY_SP, items: Vec::new() };
Ok((id, ItemKind::Mod(placeholder), None))
}
} else {
let directory = self.directory.clone();
let restrictions = self.push_directory(id, &outer_attrs);
let old_directory = self.directory.clone();
self.push_directory(id, &outer_attrs);
self.expect(&token::OpenDelim(token::Brace))?;
let mod_inner_lo = self.span.lo;
let attrs = self.parse_inner_attributes()?;
let m = self.with_res(restrictions, |this| {
this.parse_mod_items(&token::CloseDelim(token::Brace), mod_inner_lo)
})?;
self.directory = directory;
Ok((id, ItemKind::Mod(m), Some(attrs)))
let module = self.parse_mod_items(&token::CloseDelim(token::Brace), mod_inner_lo)?;
self.directory = old_directory;
Ok((id, ItemKind::Mod(module), Some(attrs)))
}
}
fn push_directory(&mut self, id: Ident, attrs: &[Attribute]) -> Restrictions {
if let Some(path) = ::attr::first_attr_value_str_by_name(attrs, "path") {
self.directory.push(&*path.as_str());
self.restrictions - Restrictions::NO_NONINLINE_MOD
fn push_directory(&mut self, id: Ident, attrs: &[Attribute]) {
if let Some(path) = attr::first_attr_value_str_by_name(attrs, "path") {
self.directory.path.push(&*path.as_str());
self.directory.ownership = DirectoryOwnership::Owned;
} else {
self.directory.push(&*id.name.as_str());
self.restrictions
self.directory.path.push(&*id.name.as_str());
}
}
pub fn submod_path_from_attr(attrs: &[ast::Attribute], dir_path: &Path) -> Option<PathBuf> {
::attr::first_attr_value_str_by_name(attrs, "path").map(|d| dir_path.join(&*d.as_str()))
attr::first_attr_value_str_by_name(attrs, "path").map(|d| dir_path.join(&*d.as_str()))
}
/// Returns either a path to a module, or .
@ -5317,8 +5330,16 @@ impl<'a> Parser<'a> {
let secondary_exists = codemap.file_exists(&secondary_path);
let result = match (default_exists, secondary_exists) {
(true, false) => Ok(ModulePathSuccess { path: default_path, owns_directory: false }),
(false, true) => Ok(ModulePathSuccess { path: secondary_path, owns_directory: true }),
(true, false) => Ok(ModulePathSuccess {
path: default_path,
directory_ownership: DirectoryOwnership::UnownedViaMod(false),
warn: false,
}),
(false, true) => Ok(ModulePathSuccess {
path: secondary_path,
directory_ownership: DirectoryOwnership::Owned,
warn: false,
}),
(false, false) => Err(ModulePathError {
err_msg: format!("file not found for module `{}`", mod_name),
help_msg: format!("name the file either {} or {} inside the directory {:?}",
@ -5346,13 +5367,20 @@ impl<'a> Parser<'a> {
id: ast::Ident,
outer_attrs: &[ast::Attribute],
id_sp: Span) -> PResult<'a, ModulePathSuccess> {
if let Some(p) = Parser::submod_path_from_attr(outer_attrs, &self.directory) {
return Ok(ModulePathSuccess { path: p, owns_directory: true });
if let Some(path) = Parser::submod_path_from_attr(outer_attrs, &self.directory.path) {
return Ok(ModulePathSuccess {
directory_ownership: match path.file_name().and_then(|s| s.to_str()) {
Some("mod.rs") => DirectoryOwnership::Owned,
_ => DirectoryOwnership::UnownedViaMod(true),
},
path: path,
warn: false,
});
}
let paths = Parser::default_submod_path(id, &self.directory, self.sess.codemap());
let paths = Parser::default_submod_path(id, &self.directory.path, self.sess.codemap());
if self.restrictions.contains(Restrictions::NO_NONINLINE_MOD) {
if let DirectoryOwnership::UnownedViaBlock = self.directory.ownership {
let msg =
"Cannot declare a non-inline module inside a block unless it has a path attribute";
let mut err = self.diagnostic().struct_span_err(id_sp, msg);
@ -5362,10 +5390,15 @@ impl<'a> Parser<'a> {
err.span_note(id_sp, &msg);
}
return Err(err);
} else if !self.owns_directory {
} else if let DirectoryOwnership::UnownedViaMod(warn) = self.directory.ownership {
if warn {
if let Ok(result) = paths.result {
return Ok(ModulePathSuccess { warn: true, ..result });
}
}
let mut err = self.diagnostic().struct_span_err(id_sp,
"cannot declare a new module at this location");
let this_module = match self.directory.file_name() {
let this_module = match self.directory.path.file_name() {
Some(file_name) => file_name.to_str().unwrap().to_owned(),
None => self.root_module_name.as_ref().unwrap().clone(),
};
@ -5378,8 +5411,10 @@ impl<'a> Parser<'a> {
&format!("... or maybe `use` the module `{}` instead \
of possibly redeclaring it",
paths.name));
}
return Err(err);
return Err(err);
} else {
return Err(err);
};
}
match paths.result {
@ -5390,25 +5425,11 @@ impl<'a> Parser<'a> {
/// Read a module from a source file.
fn eval_src_mod(&mut self,
id: ast::Ident,
outer_attrs: &[ast::Attribute],
path: PathBuf,
directory_ownership: DirectoryOwnership,
name: String,
id_sp: Span)
-> PResult<'a, (ast::ItemKind, Vec<ast::Attribute> )> {
let ModulePathSuccess { path, owns_directory } = self.submod_path(id,
outer_attrs,
id_sp)?;
self.eval_src_mod_from_path(path,
owns_directory,
id.to_string(),
id_sp)
}
fn eval_src_mod_from_path(&mut self,
path: PathBuf,
owns_directory: bool,
name: String,
id_sp: Span) -> PResult<'a, (ast::ItemKind, Vec<ast::Attribute> )> {
let mut included_mod_stack = self.sess.included_mod_stack.borrow_mut();
if let Some(i) = included_mod_stack.iter().position(|p| *p == path) {
let mut err = String::from("circular modules: ");
@ -5423,7 +5444,8 @@ impl<'a> Parser<'a> {
included_mod_stack.push(path.clone());
drop(included_mod_stack);
let mut p0 = new_sub_parser_from_file(self.sess, &path, owns_directory, Some(name), id_sp);
let mut p0 =
new_sub_parser_from_file(self.sess, &path, directory_ownership, Some(name), id_sp);
let mod_inner_lo = p0.span.lo;
let mod_attrs = p0.parse_inner_attributes()?;
let m0 = p0.parse_mod_items(&token::Eof, mod_inner_lo)?;

View File

@ -0,0 +1,21 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// error-pattern: cannot declare a new module at this location
// error-pattern: will become a hard error
// error-pattern: compilation successful
#![feature(rustc_attrs)]
#[path="mod_file_not_owning_aux3.rs"]
mod foo;
#[rustc_error]
fn main() {}

View File

@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// compile-flags: -Z parse-only
// error-pattern: cannot declare a new module at this location
mod mod_file_not_owning_aux1;

View File

@ -10,4 +10,7 @@
// ignore-test this is not a test
mod mod_file_not_owning_aux2;
macro_rules! m {
() => { mod mod_file_not_owning_aux2; }
}
m!();

View File

@ -0,0 +1,13 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// ignore-test this is not a test
mod mod_file_not_owning_aux2;

View File

@ -0,0 +1,15 @@
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// error-pattern: cannot declare a new module at this location
// This is not a directory owner since the file name is not "mod.rs".
#[path = "mod_file_not_owning_aux1.rs"]
mod foo;

View File

@ -12,6 +12,7 @@
// ignore-test: this is an auxiliary file for circular-modules-main.rs
#[path = "circular_modules_main.rs"]
mod circular_modules_main;
pub fn say_hello() {