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

Fix invalid module suggestion #38255

Closed
Closed
Changes from all commits
Commits
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
29 changes: 17 additions & 12 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
@@ -5131,25 +5131,30 @@ impl<'a> Parser<'a> {
}
let mut err = self.diagnostic().struct_span_err(id_sp,
"cannot declare a new module at this location");
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(),
};
err.span_note(id_sp,
&format!("maybe move this module `{0}` to its own directory \
via `{0}/mod.rs`",
this_module));
if id_sp != syntax_pos::DUMMY_SP {
let src_path = PathBuf::from(self.sess.codemap().span_to_filename(id_sp));
if let Some(stem) = src_path.clone().file_stem() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need the .clone() here.

let mut dest_path = src_path.clone();
dest_path.set_file_name(stem);
dest_path.push("mod.rs");
err.span_note(id_sp,
&format!("maybe move this module `{}` to its own \
directory via `{}`", src_path.to_string_lossy(),
dest_path.to_string_lossy()));
}
}
if paths.path_exists {
err.span_note(id_sp,
&format!("... or maybe `use` the module `{}` instead \
of possibly redeclaring it",
paths.name));
Err(err)
} else {
Err(err)
}
Err(err)
} else {
paths.result.map_err(|err| self.span_fatal_err(id_sp, err))
match paths.result {
Ok(succ) => Ok(succ),
Err(err) => Err(self.span_fatal_err(id_sp, err)),
}
}
}

11 changes: 11 additions & 0 deletions src/test/compile-fail/auxiliary/foo/bar.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2017 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.

pub mod baz;
11 changes: 11 additions & 0 deletions src/test/compile-fail/auxiliary/foo/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2017 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.

pub mod bar;
20 changes: 20 additions & 0 deletions src/test/compile-fail/invalid-module-declaration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// 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-tidy-linelength

// error-pattern: cannot declare a new module at this location
// error-pattern: maybe move this module
Copy link
Contributor

@jseyfried jseyfried Mar 4, 2017

Choose a reason for hiding this comment

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

Could you include the module paths here so that they are tested? It looks like this test would pass before the PR.
It doesn't matter if the paths are global and verbose here.

Copy link
Member Author

Choose a reason for hiding this comment

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

As you asked, I removed the strip_prefix, so now it's the full path starting from /. Therefore, if I put it, it won't work because it'll be mine. Or am I missing something?

Copy link
Contributor

@jseyfried jseyfried Mar 4, 2017

Choose a reason for hiding this comment

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

I removed the strip_prefix, so now it's the full path starting from /

I believe it was the full path starting from / even with strip_prefix -- that is, the strip_prefix stuff didn't make a difference in this case.

Therefore, if I put it, it won't work because it'll be mine

Good point -- the global path will depend on what platform is running the tests.

It looks like we'll need a ui test, or if that doesn't work either a run-make test.


mod auxiliary {
mod foo;
}

fn main() {}
Copy link
Contributor

@jseyfried jseyfried Feb 16, 2017

Choose a reason for hiding this comment

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

nit: add trailing newline

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.