Fix invalid module suggestion #38255

GuillaumeGomez · 2016-12-09T00:26:25Z

Fixes #38110.

rust-highfive · 2016-12-09T00:26:37Z

r? @nrc

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

GuillaumeGomez · 2016-12-09T04:44:38Z

I have no idea why it failed. It seems there is no error. Or maybe I missed it?

jseyfried · 2016-12-09T23:18:40Z

src/libsyntax/parse/parser.rs

            err.span_note(id_sp,
                          &format!("maybe move this module `{0}` to its own directory \
                                     via `{0}/mod.rs`",
-                                    this_module));
+                                    paths.name));


This is supposed to be the name of the file currently being parsed, not the name of the erroneous mod.

For example, if we are parsing foo/bar.rs and we encounter a non-inline module mod baz;, we want to suggest moving foo/bar.rs to foo/bar/mod.rs. That way, bar would be a directory owner (c.f. #37602), so mod baz; would expect foo/bar/baz.rs (or foo/bar/baz/mod.rs) instead of being an error.

To get the name of the file currently being parsed, you could use self.root_module_name (skipping the note if it is None), or even better, remove the root_module_name field entirely and instead use self.sess.codemap().span_to_filename(id_sp) (skipping the note if id_sp is DUMMY_SP).

Hum, the issue where I got this didn't reflect such a case. I'll need to take a different look. Thanks for the help!

jseyfried · 2016-12-09T23:22:00Z

@GuillaumeGomez Also, could you add a regression test?

I have no idea why it failed

Travis has been broken for a couple of days, not sure why.

GuillaumeGomez · 2016-12-10T13:11:45Z

Sure.

GuillaumeGomez · 2017-01-02T23:15:26Z

So, I updated the code but I now need to figure out how to add a compile fail test for it. I need to create a folder and 3 files for it in order to test it (no problem) but tests fail if no failing pattern is in the file. More to come when I figure all this out!

jseyfried

The field root_module_name of Parser is now unused and can be removed.

jseyfried · 2017-01-03T06:30:37Z

src/libsyntax/parse/parser.rs

-                                    this_module));
+            if id_sp != syntax_pos::DUMMY_SP {
+                let path = self.sess.codemap().span_to_filename(id_sp);
+                let path = if path.ends_with(".rs") {


I think it would be better to always remove the extension here, e.g.

let path = Path::new(self.sess.codemap().span_to_filename(id_sp)).file_stem().unwrap();

Would be better, indeed.

jseyfried · 2017-01-03T06:35:48Z

@GuillaumeGomez You can put the helper files in compile-fail/auxiliary or add // ignore-test.

GuillaumeGomez · 2017-01-03T09:59:19Z

Thanks for the info @jseyfried!

GuillaumeGomez · 2017-01-03T11:20:12Z

@jseyfried: Is it how you had it in mind?

pnkfelix · 2017-01-03T21:10:01Z

src/libsyntax/parse/parser.rs

+                let parent = path.parent().unwrap_or(Path::new(""))
+                                          .to_str().unwrap_or("").to_owned();
+                let path = format!("{}/{}",
+                                   if parent.len() == 0 { "." } else { &parent },


out of curiosity, can you construct an example where the parent.len() is in fact equal to zero? I would have thought the context where this is arising will ensure that there is always a parent, but maybe I am missing a scenario.

In case you're compiling a file at the same level of rustc I guess? I'm not sure if it's an absolute or a relative path so better be sure than sorry.

pnkfelix · 2017-01-03T21:11:54Z

src/test/compile-fail/auxiliary/foo/bar.rs

+// except according to those terms.
+
+pub mod baz; //~ ERROR cannot declare a new module at this location
+             //~ NOTE maybe move this module `src/test/compile-fail/foo/bar` to its own directory via `src/test/compile-fail/foo/bar/mod.rs`


if the suggestion is saying move foo/bar to foo/bar/mod.rs, wouldn't it make more sense for it to suggesting moving foo/bar.rs (i.e. including the file extension) to foo/bar/mod.rs, so that the source and destination look consistent?

hmm I see that @jseyfried had earlier suggested removing the file extension.

If you're going to remove it from bar.rs, then you should probably remove it from mod.rs (again in the name of consistency between the origin and destination)

But I'm not so sure its clearer that way (versus having the .rs on both sides...)

@GuillaumeGomez @pnkfelix
I was suggesting removing the file extension only to produce the bar in foo/bar/mod.rs.
That is, I think we should suggest moving foo/bar.rs to foo/bar/mod.rs.

If the file is not .rs (possible via #[path]), I think we should suggest moving e.g. foo/bar.txt to foo/bar/mod.rs.
Note that mod.rs needs to be .rs due to #37602.

@GuillaumeGomez
I think the best way to do this is to

Convert the span_to_filename String into a PathBuf

Remove the extension (.rs or other)

e.g. let stem = path_buf.file_stem(); path_buf.set_file_name(stem);

Push "mod.rs" to the PathBuf

@jseyfried: I like this solution! I'll update in the evening.

GuillaumeGomez · 2017-01-08T20:29:07Z

Updated.

jseyfried · 2017-01-15T11:03:28Z

ping @GuillaumeGomez -- could you fix the test as described here?

GuillaumeGomez · 2017-01-15T17:39:58Z

Forgot this PR, sorry!

GuillaumeGomez · 2017-01-15T18:13:17Z

Even with the explanations and the changes, I still get:

---- [compile-fail] compile-fail/invalid-module-declaration.rs stdout ----

error: auxiliary build of "/Users/imperio/rust/rust/src/test/compile-fail/auxiliary/module-suggestion.rs" failed to compile:
status: exit code: 101
command: x86_64-apple-darwin/stage1/bin/rustc /Users/imperio/rust/rust/src/test/compile-fail/auxiliary/module-suggestion.rs -L x86_64-apple-darwin/test/compile-fail/ --target=x86_64-apple-darwin --error-format json --crate-type=dylib -L x86_64-apple-darwin/test/compile-fail/invalid-module-declaration.stage1-x86_64-apple-darwin.compile-fail.libaux -C prefer-dynamic --out-dir x86_64-apple-darwin/test/compile-fail/invalid-module-declaration.stage1-x86_64-apple-darwin.compile-fail.libaux --cfg rtopt -C rpath -O -L x86_64-apple-darwin/rt
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
{"message":"cannot declare a new module at this location","code":null,"level":"error","spans":[{"file_name":"/Users/imperio/rust/rust/src/test/compile-fail/auxiliary/foo/bar.rs","byte_start":1452,"byte_end":1455,"line_start":11,"line_end":11,"column_start":9,"column_end":12,"is_primary":true,"text":[{"text":"pub mod baz;","highlight_start":9,"highlight_end":12}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[{"message":"maybe move this module `/Users/imperio/rust/rust/src/test/compile-fail/auxiliary/foo/bar/mod.rs` to its own directory via `/Users/imperio/rust/rust/src/test/compile-fail/auxiliary/foo/bar/mod.rs`","code":null,"level":"note","spans":[{"file_name":"/Users/imperio/rust/rust/src/test/compile-fail/auxiliary/foo/bar.rs","byte_start":1452,"byte_end":1455,"line_start":11,"line_end":11,"column_start":9,"column_end":12,"is_primary":true,"text":[{"text":"pub mod baz;","highlight_start":9,"highlight_end":12}],"label":null,"suggested_replacement":null,"expansion":null}],"children":[],"rendered":null}],"rendered":null}
{"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":null}

------------------------------------------

thread '[compile-fail] compile-fail/invalid-module-declaration.rs' panicked at 'explicit panic', /Users/imperio/rust/rust/src/tools/compiletest/src/runtest.rs:2465

So completely useless... I think adding a test for this isn't this useful. Should I remove it?

jseyfried · 2017-01-15T19:08:13Z

@GuillaumeGomez

Should I remove [the test]?

No, we definitely need a test for this (compile-fail or ui).

Could you push what you have so far?
Based on the error message, it looks like you included // aux-build in the main test. This causes the test harness to compile the aux-build file first so that the main test can have extern crate auxiliary_file.

For this use case, you don't want // aux-build -- you instead reference the files in auxiliary with
#[path = "auxiliary/foo/mod.rs"] mod foo; or mod auxiliary { mod foo; }

jseyfried · 2017-01-15T19:13:00Z

src/test/compile-fail/auxiliary/foo/bar.rs

+// ignore-tidy-linelength
+
+pub mod baz; //~ ERROR cannot declare a new module at this location
+             //~ NOTE maybe move this module `src/test/compile-fail/foo/bar` to its own directory via `src/test/compile-fail/foo/bar/mod.rs`


Could this be

maybe move this module `src/test/compile-fail/foo/bar.rs` to its own directory via `src/test/compile-fail/foo/bar/mod.rs`

GuillaumeGomez · 2017-01-15T19:35:41Z

Pushed the changes into a second commit.

alexcrichton · 2017-02-04T21:56:17Z

ping @jseyfried, this look good to you now?

alexcrichton · 2017-02-04T21:56:48Z

@GuillaumeGomez looks like there may also be a failing test here that'll need fixing

GuillaumeGomez · 2017-02-05T08:47:32Z

@alexcrichton: Indeed, I'll try to update it in the next days.

GuillaumeGomez · 2017-02-14T17:52:16Z

@jseyfried: So I gave another try and I'm still completely unable to make it work. Any idea what I've done incorrectly (or missed)?

GuillaumeGomez · 2017-02-15T21:08:05Z

@jseyfried: Updated. Thanks for all your help!

jseyfried · 2017-02-16T01:55:10Z

src/libsyntax/parse/parser.rs

+                full_path.push("mod.rs");
+                err.span_note(id_sp,
+                              &format!("maybe move this module `{0}` to its own directory \
+                                        via `{0}`", full_path.to_str().unwrap_or("")));


Don't we want two paths here, e.g. maybe move this module `foo/bar.rs` to its own directory via `foo/bar/mod.rs` ?

Also, instead of unwrap_or(""), I would use to_string_lossy or just not emit the note if to_str is None.

jseyfried · 2017-02-16T01:57:13Z

src/test/compile-fail/invalid-module-declaration.rs

+// ignore-tidy-linelength
+
+// error-pattern: cannot declare a new module at this location
+// note-pattern: maybe move this module `src/test/compile-fail/auxiliary/foo/bar` to its own directory via `src/test/compile-fail/auxiliary/foo/bar/mod.rs`


// note-pattern has no meaning to the compile-fail tests -- use // error-pattern here instead (works just as well for notes).

Also, don't we want the pattern to be the following?

maybe move this module `src/test/compile-fail/auxiliary/foo/bar.rs` to its own directory via `src/test/compile-fail/auxiliary/foo/bar/mod.rs`

jseyfried · 2017-02-16T01:57:20Z

src/test/compile-fail/invalid-module-declaration.rs

+    mod foo;
+}
+
+fn main() {}


nit: add trailing newline

jseyfried · 2017-02-16T01:59:47Z

src/libsyntax/parse/parser.rs

-                                    this_module));
+            if id_sp != syntax_pos::DUMMY_SP {
+                let mut full_path = PathBuf::from(self.sess.codemap().span_to_filename(id_sp));
+                let full_path_copy = full_path.clone();


Could we give these more descriptive names, e.g. source_path/dest_path, current_path/owner_path, etc.?

GuillaumeGomez · 2017-02-17T00:29:32Z

Updated.

jseyfried · 2017-02-17T07:10:17Z

src/libsyntax/parse/parser.rs

+                if let Ok(cur_dir) = env::current_dir() {
+                    if let (Ok(src_path), Ok(dest_path)) =
+                        (Path::new(&src_path).strip_prefix(&cur_dir),
+                         Path::new(&dest_path).strip_prefix(&cur_dir)) {


I think we should fall back to the raw src_path and dest_path if env::current_dir() is None or strip_prefix is Err. That should fix the failing test.

I would mutate src_path and/or dest_path when strip_prefix is Ok and then err.span_note unconditionally.

jseyfried · 2017-02-17T07:13:34Z

src/libsyntax/parse/parser.rs

-                                    this_module));
+            if id_sp != syntax_pos::DUMMY_SP {
+                let mut src_path = PathBuf::from(self.sess.codemap().span_to_filename(id_sp));
+                let src_path_copy = src_path.clone();


We don't need src_path_copy.

jseyfried · 2017-02-17T07:15:28Z

src/libsyntax/parse/parser.rs

+            if id_sp != syntax_pos::DUMMY_SP {
+                let mut src_path = PathBuf::from(self.sess.codemap().span_to_filename(id_sp));
+                let src_path_copy = src_path.clone();
+                let stem = src_path_copy.file_stem().unwrap_or(OsStr::new(""));


If there's no file_stem, I think we should use the full file name or just not emit the note. I believe using "" here would produce a very confusing suggestion.

jseyfried · 2017-02-17T07:16:37Z

src/libsyntax/parse/parser.rs

+                let mut dest_path = src_path.clone();
+                dest_path.set_file_name(stem);
+                dest_path.push("mod.rs");
+                src_path.set_extension("rs");


Why do this? If the source path isn't .rs, we'll be telling the user to move a file that doesn't exist.

GuillaumeGomez · 2017-02-17T12:55:17Z

Updated.

GuillaumeGomez · 2017-02-17T20:27:07Z

It seems it's now a path issue in the error checkup. Any idea how I could fix this issue @jseyfried?

jseyfried · 2017-02-19T23:43:27Z

The issue is that we're producing /checkout/src/test/compile-fail/auxiliary/foo/bar.rs but expecting src/test/compile-fail/auxiliary/foo/bar.rs.

I would just change the test to expect /checkout/src/test/compile-fail/auxiliary/foo/bar.rs and get rid of the env::current_dir() / strip_prefix stuff (it's not getting tested, and I don't think it's useful in practice).

GuillaumeGomez · 2017-02-21T22:03:16Z

@jseyfried: It makes output more comfortable to read.

bors · 2017-02-21T23:49:18Z

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

jseyfried · 2017-02-22T21:03:50Z

@GuillaumeGomez

It makes output more comfortable to read.

Does it?

cargo calls rustc with paths that are already relative to the current directory (e.g. src/lib.rs if we cargo build in the project directory, lib.rs if we cargo build in src).

If people use rustc directly, then they will see the same kind of path they passed to rustc.

jseyfried · 2017-02-22T21:06:44Z

I think we should start with not doing env::current_dir() / strip_prefix and then maybe add it later if there are real-world use cases where it would improve diagnostics (I'm skeptical that there would be).

GuillaumeGomez · 2017-02-22T21:07:03Z

As you prefer, I'd just like this change to get merge so I'll remove the path prefix.

GuillaumeGomez · 2017-03-03T22:53:55Z

And updated once again.

jseyfried · 2017-03-04T00:28:54Z

src/libsyntax/parse/parser.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() {


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

jseyfried · 2017-03-04T00:31:13Z

src/test/compile-fail/invalid-module-declaration.rs

+// ignore-tidy-linelength
+
+// error-pattern: cannot declare a new module at this location
+// error-pattern: maybe move this module


~~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.~~

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?

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.

bors · 2017-03-31T05:56:37Z

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

alexcrichton · 2017-04-01T22:53:03Z

Closing due to inactivity, but feel free to resubmit with a rebase!

GuillaumeGomez · 2017-04-02T17:16:56Z

I'll when the testing issue will be solved.

rust-highfive assigned nrc Dec 9, 2016

jseyfried reviewed Dec 9, 2016

View reviewed changes

jseyfried assigned jseyfried and unassigned nrc Dec 9, 2016

GuillaumeGomez force-pushed the invalid_module_location branch from a598d84 to 67e321a Compare January 2, 2017 23:12

jseyfried reviewed Jan 3, 2017

View reviewed changes

GuillaumeGomez force-pushed the invalid_module_location branch from 67e321a to 6acb1ba Compare January 3, 2017 11:19

pnkfelix reviewed Jan 3, 2017

View reviewed changes

GuillaumeGomez force-pushed the invalid_module_location branch 2 times, most recently from 84b83fd to 1676314 Compare January 8, 2017 18:32

jseyfried reviewed Jan 15, 2017

View reviewed changes

GuillaumeGomez force-pushed the invalid_module_location branch from 7e2202a to 4e090dd Compare February 15, 2017 21:07

jseyfried reviewed Feb 16, 2017

View reviewed changes

GuillaumeGomez force-pushed the invalid_module_location branch from 4e090dd to 15cd2bc Compare February 17, 2017 00:28

jseyfried reviewed Feb 17, 2017

View reviewed changes

GuillaumeGomez force-pushed the invalid_module_location branch from 15cd2bc to e2e758d Compare February 17, 2017 12:49

GuillaumeGomez force-pushed the invalid_module_location branch from e2e758d to 2316f38 Compare February 25, 2017 16:09

GuillaumeGomez added 3 commits March 3, 2017 23:03

Fix invalid module suggestion

c7bcdeb

Add tests for module suggestions

970154e

Remove strip prefix

50461ef

GuillaumeGomez force-pushed the invalid_module_location branch from 2316f38 to 50461ef Compare March 3, 2017 22:53

jseyfried reviewed Mar 4, 2017

View reviewed changes

alexcrichton closed this Apr 1, 2017

Fix invalid module suggestion #38255

Fix invalid module suggestion #38255

Conversation

GuillaumeGomez commented Dec 9, 2016

rust-highfive commented Dec 9, 2016

GuillaumeGomez commented Dec 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Dec 9, 2016 • edited Loading

GuillaumeGomez commented Dec 10, 2016

GuillaumeGomez commented Jan 2, 2017

jseyfried left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried commented Jan 3, 2017

GuillaumeGomez commented Jan 3, 2017

GuillaumeGomez commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 8, 2017

jseyfried commented Jan 15, 2017

GuillaumeGomez commented Jan 15, 2017

GuillaumeGomez commented Jan 15, 2017

jseyfried commented Jan 15, 2017

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 15, 2017

alexcrichton commented Feb 4, 2017

alexcrichton commented Feb 4, 2017

GuillaumeGomez commented Feb 5, 2017

GuillaumeGomez commented Feb 14, 2017

GuillaumeGomez commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 17, 2017

GuillaumeGomez commented Feb 17, 2017

jseyfried commented Feb 19, 2017

GuillaumeGomez commented Feb 21, 2017

bors commented Feb 21, 2017

jseyfried commented Feb 22, 2017 • edited Loading

jseyfried commented Feb 22, 2017

GuillaumeGomez commented Feb 22, 2017

GuillaumeGomez commented Mar 3, 2017

Choose a reason for hiding this comment

jseyfried Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseyfried Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 31, 2017

alexcrichton commented Apr 1, 2017

GuillaumeGomez commented Apr 2, 2017

jseyfried commented Dec 9, 2016 •

edited

Loading

jseyfried Jan 4, 2017 •

edited

Loading

jseyfried Feb 16, 2017 •

edited

Loading

jseyfried commented Feb 22, 2017 •

edited

Loading

jseyfried Mar 4, 2017 •

edited

Loading

jseyfried Mar 4, 2017 •

edited

Loading