File not found for module error #39765

GuillaumeGomez · 2017-02-12T14:20:29Z

Maybe you want to take a look @pnkfelix?

frewsxcv · 2017-02-12T15:41:58Z

src/libsyntax/diagnostic_list.rs

+Erroneous code example:
+
+```compile_fail,E0583
+mod file_that_doesnt_exist; // error: ile not found for module


Typo: "ile not found for module"

Oups, thanks!

frgtn · 2017-02-12T14:49:08Z

src/libsyntax/diagnostic_list.rs

@@ -224,4 +291,5 @@ register_diagnostics! {
    E0555, // malformed feature attribute, expected #![feature(...)]
    E0556, // malformed feature, expected just one word
    E0557, // feature has been removed
+    E0584, // file for module `..` found at both .. and ..


Shouldn't the other newly added errors be registered as well?

Don't know how to trigger this one and not enough time to investigate. Someone will do it later on if I don't myself.

This error occurs for mod foo; when both foo.rs and foo/mod.rs exist. There's a test: cfail/mod_file_disambig.

durka · 2017-02-13T02:34:39Z

src/libsyntax/diagnostic_list.rs

+fn foo() {}
+```
+"##,
+


Feature flags will be needed in these examples, also the cfail tests for inclusive ranges.

GuillaumeGomez · 2017-02-13T10:53:59Z

@durka: I added the corresponding tests already. Take a look at the new files in compile-fail directory.

durka · 2017-02-13T15:39:18Z

Yeah but you are missing #![feature(inclusive_range_syntax)].

GuillaumeGomez · 2017-02-13T16:54:03Z

It's not needed as far as I can tell... ? At least when I test, I have no missing feature issue.

durka · 2017-02-13T16:59:28Z

Huh... I guess since this error is generated in the parser it occurs before the unstable feature check. So it's okay I guess.

…

On Mon, Feb 13, 2017 at 11:54 AM, Guillaume Gomez ***@***.***> wrote: It's not needed as far as I can tell... ? At least when I test, I have no missing feature issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#39765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n2GC6GEfxqv3beixvW_uCblHhPn5ks5rcIrIgaJpZM4L-f60> .

sophiajt · 2017-02-15T19:04:59Z

I'm cool with this change, but you probably want to ping someone on the compiler team to double-check the logic. I haven't done much with the module system, yet.

GuillaumeGomez · 2017-02-15T21:11:09Z

cc @rust-lang/compiler

jseyfried · 2017-02-16T02:04:30Z

src/libsyntax/diagnostic_list.rs

@@ -201,6 +201,73 @@ where appropriate is ongoing. Try using an unquoted name instead:
 pub fn something() {}
 ```
 "##,
+
+E0583: r##"
+A file wasn't found for a module import.


Do we want to call mod foo; a "module import"?
I've heard it called an "out-of-line module", but never heard "module import" except to refer to a use import of a module.

How would you reword it then?

I would call it a "module item" or an "out-of-line module".

jseyfried · 2017-02-16T02:07:01Z

src/libsyntax/diagnostic_list.rs

+fn main() {}
+```
+
+Please be sure that a file corresponding to the module really exist. So if you


nit: ~~really~~ exists. So

jseyfried · 2017-02-16T02:13:26Z

src/libsyntax/diagnostic_list.rs

+
+Please be sure that a file corresponding to the module really exist. So if you
+want to import a `file_that_doesnt_exist` module, you need to have a
+`file_that_doesnt_exist.rs` file at the same level.


or file_that_doesnt_exist/mod.rs

jseyfried · 2017-02-16T02:15:12Z

src/libsyntax/diagnostic_list.rs

+"##,
+
+E0585: r##"
+A documentation comment which doesn't comment anything was found.


nit: s/which/that/, s/comment/document/

jseyfried · 2017-02-16T02:17:27Z

src/libsyntax/diagnostic_list.rs

+}
+```
+
+Documentation comments need to be used on items like functions, types,


nit: s/used on/followed by/, s/like/including/

jseyfried · 2017-02-16T02:21:54Z

src/libsyntax/parse/parser.rs

+                mod_name: mod_name.clone(),
+                default_path: default_path_str,
+                secondary_path: secondary_path_str,
+                dir_path: format!("{}", dir_path.display()) }),


nit: add a newline before the }),

jseyfried · 2017-02-16T02:22:13Z

src/libsyntax/parse/parser.rs

+                dir_path: format!("{}", dir_path.display()) }),
+            (true, true) => Err(Errors::DuplicatePaths { mod_name: mod_name.clone(),
+                                                         default_path: default_path_str,
+                                                         secondary_path: secondary_path_str }),


nit: block indenting is more idiomatic here.

jseyfried · 2017-02-16T02:23:16Z

src/libsyntax/parse/parser.rs

+            match paths.result {
+                Ok(succ) => Ok(succ),
+                Err(err) => Err(self.span_fatal_err(id_sp, err)),
+            }


nit: we could use map_err here instead of match.

jseyfried · 2017-02-16T02:23:41Z

src/test/compile-fail/E0586.rs

+fn main() {
+    let tmp = vec![0, 1, 2, 3, 4, 4, 3, 3, 2, 1];
+    let x = &tmp[1...]; //~ ERROR E0586
+}


nit: no trailing new newline

jseyfried · 2017-02-16T02:25:59Z

src/libsyntax/parse/parser.rs

+                            dir_path: String },
+    DuplicatePaths { mod_name: String, default_path: String, secondary_path: String },
+    UselessDocComment,
+    InclusiveRangeWithNoEnd,


Do we need this variant? I think it would be simpler to just emit the error directly in the relevant parsing function.

I try to centralize the errors. Making them easier to spot and modify.

I think UselessDocComment and InclusiveRangeWithNoEnd would be more appropriate as methods on the Parser, since they don't have anything to do with with the first two variants (except that they are errors during parsing).

Then, the enum could be named ModulePathError -- clearer and more precise.

The point isn't to group same kind of errors but to centralize them.

GuillaumeGomez · 2017-02-19T15:36:08Z

@jseyfried: Is it ok for you now? ;)

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

src/libsyntax/diagnostic_list.rs

+```
+
+Please be sure that a file corresponding to the module exists. If you
+want to import a `file_that_doesnt_exist` module, you need to have a


We're not "import"ing here -- I would say "define", "declare", or "use" instead.

Also I would say "a module named file_that_doesnt_exist" instead of "a file_that_doesnt_exist module".

jseyfried · 2017-02-19T23:52:54Z

src/libsyntax/diagnostic_list.rs

+
+Please be sure that a file corresponding to the module exists. If you
+want to import a `file_that_doesnt_exist` module, you need to have a
+`file_that_doesnt_exist/mod.rs` file at the same level.


The user can have file_that_doesnt_exist.rs or file_that_doesnt_exist/mod.rs -- we need to mention both options here.

Also, I would say "in the same directory" instead of "at the same level".

jseyfried · 2017-02-19T23:54:26Z

src/libsyntax/diagnostic_list.rs

+}
+```
+
+Documentation comments need to be followed items including functions, types,


need to be followed by items**,** including
(add "by" and ",")

jseyfried · 2017-02-19T23:54:34Z

src/libsyntax/diagnostic_list.rs

+```
+
+Documentation comments need to be followed items including functions, types,
+modules, etc... Examples:


Just one "." here.

jseyfried · 2017-02-19T23:57:34Z

src/libsyntax/parse/parser.rs

-                                    "doc comments must come before what they document, maybe a \
-                                    comment was intended with `//`?");
+                                Errors::UselessDocComment.span_err(s.prev_span, s.diagnostic())
+                                                         .emit();


If we're not getting rid of span_fatal_err, shouldn't we use it here?

Absolutely!

GuillaumeGomez · 2017-02-20T17:35:14Z

Updated.

jseyfried

r=me modulo comments

jseyfried · 2017-02-20T22:20:18Z

src/libsyntax/diagnostic_list.rs

+
+Please be sure that a file corresponding to the module exists. If you
+want to use a module named `file_that_doesnt_exist`, you need to have a
+`file_that_doesnt_exist.rs` or a `file_that_doesnt_exist/mod.rs` file in the


nit: you need to have a file_that_doesnt_exist.rs or file_that_doesnt_exist/mod.rs ~~file~~ in the same directory.

or: you need to have a file named file_that_doesnt_exist.rs or file_that_doesnt_exist/mod.rs in the same directory.

jseyfried · 2017-02-20T22:22:09Z

src/libsyntax/parse/parser.rs

@@ -233,6 +233,63 @@ pub struct ModulePathError {
    pub help_msg: String,
 }

+pub enum Errors {


optional nit: Error would be a more idiomatic name

GuillaumeGomez · 2017-02-20T22:29:08Z

Ok, I fixed the two last nits. If you find anything else, don't hesitate to r-!

@bors: r=jseyfried

bors · 2017-02-20T22:29:08Z

📌 Commit a9f22d7 has been approved by jseyfried

jseyfried · 2017-02-20T22:57:22Z

src/libsyntax/parse/parser.rs

+    InclusiveRangeWithNoEnd,
+}
+
+impl Errors {


Errors -> Error (won't pass travis)

jseyfried · 2017-02-20T22:57:47Z

src/libsyntax/parse/parser.rs

+            Error::FileNotFoundForModule { ref mod_name,
+                                           ref default_path,
+                                           ref secondary_path,
+                                            ref dir_path } => {


nit: indent (one extra space)

jseyfried · 2017-02-20T22:58:39Z

src/libsyntax/parse/parser.rs

@@ -955,6 +1009,9 @@ impl<'a> Parser<'a> {
    pub fn span_fatal(&self, sp: Span, m: &str) -> DiagnosticBuilder<'a> {
        self.sess.span_diagnostic.struct_span_fatal(sp, m)
    }
+    pub fn span_fatal_err(&self, sp: Span, err: Errors) -> DiagnosticBuilder<'a> {


Also here (Errors -> Error), etc.

jseyfried · 2017-02-20T23:00:00Z

src/libsyntax/parse/parser.rs

+                                           ref default_path,
+                                           ref secondary_path,
+                                            ref dir_path } => {
+                let mut err = struct_span_er!(handler, sp, E0583,


jseyfried · 2017-02-20T23:00:55Z

@bors r-
I usually wait for Travis to pass before directing bors.

GuillaumeGomez · 2017-02-21T10:22:44Z

@jseyfried: Outch, my string replacement failed badly... Updated.

GuillaumeGomez · 2017-02-21T19:59:27Z

Tests seem ok this time so let's r+ it. Thanks @jseyfried!

@bors: r=jseyfried

bors · 2017-02-21T19:59:27Z

📌 Commit b6818be has been approved by jseyfried

bors · 2017-02-21T20:48:15Z

⌛ Testing commit b6818be with merge ab89a78...

bors · 2017-02-21T20:53:00Z

💔 Test failed - status-appveyor

alexcrichton · 2017-02-21T20:57:02Z

@bors: retry * network failure

…

On Tue, Feb 21, 2017 at 2:53 PM, bors ***@***.***> wrote: 💔 Test failed - status-appveyor <https://ci.appveyor.com/project/rust-lang/rust/build/1.0.2054> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#39765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95OAISJpoL3yPH4ZYO08TutVSLEPlks5re06ugaJpZM4L-f60> .

bors · 2017-02-21T21:08:16Z

⌛ Testing commit b6818be with merge 0f34b53...

@pnkfelix

…, r=jseyfried File not found for module error Fixes #39542. r? @jonathandturner Maybe you want to take a look @pnkfelix?

bors · 2017-02-21T23:46:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 0f34b53 to master...

rust-highfive assigned sophiajt Feb 12, 2017

frewsxcv reviewed Feb 12, 2017

View reviewed changes

frgtn reviewed Feb 12, 2017

View reviewed changes

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from ed430c8 to 57bc7de Compare February 12, 2017 17:34

durka reviewed Feb 13, 2017

View reviewed changes

jseyfried reviewed Feb 16, 2017

View reviewed changes

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from 57bc7de to 3235835 Compare February 17, 2017 20:45

jseyfried reviewed Feb 20, 2017

View reviewed changes

GuillaumeGomez added 2 commits February 20, 2017 17:47

Add error codes for errors in libsyntax

ea2a684

Add tests for newly added error codes

0a45dd7

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from 3235835 to 1e27c61 Compare February 20, 2017 17:35

jseyfried reviewed Feb 20, 2017

View reviewed changes

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from 1e27c61 to a9f22d7 Compare February 20, 2017 22:28

jseyfried reviewed Feb 20, 2017

View reviewed changes

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from a9f22d7 to 88c81c5 Compare February 21, 2017 10:22

Add long error explanations

b6818be

GuillaumeGomez force-pushed the file-not-found-for-module-error branch from 88c81c5 to b6818be Compare February 21, 2017 14:52

bors added a commit that referenced this pull request Feb 21, 2017

Auto merge of #39765 - GuillaumeGomez:file-not-found-for-module-error…

0f34b53

…, r=jseyfried File not found for module error Fixes #39542. r? @jonathandturner Maybe you want to take a look @pnkfelix?

bors merged commit b6818be into rust-lang:master Feb 21, 2017

This was referenced Feb 21, 2017

Allow foo.rs to be parent to foo/bar.rs #39702

Closed

Implementation of repr struct alignment RFC 1358. #39999

Merged

Fix invalid module suggestion #38255

Closed

GuillaumeGomez deleted the file-not-found-for-module-error branch February 22, 2017 11:40

File not found for module error #39765

File not found for module error #39765

Conversation

GuillaumeGomez commented Feb 12, 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

durka Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

durka Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 13, 2017

durka commented Feb 13, 2017

GuillaumeGomez commented Feb 13, 2017

durka commented Feb 13, 2017 via email

sophiajt commented Feb 15, 2017

GuillaumeGomez commented Feb 15, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 19, 2017

jseyfried Feb 19, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 20, 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

GuillaumeGomez commented Feb 20, 2017

bors commented Feb 20, 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

jseyfried commented Feb 20, 2017

GuillaumeGomez commented Feb 21, 2017

GuillaumeGomez commented Feb 21, 2017

bors commented Feb 21, 2017

bors commented Feb 21, 2017

bors commented Feb 21, 2017

alexcrichton commented Feb 21, 2017 via email

bors commented Feb 21, 2017

bors commented Feb 21, 2017

durka Feb 21, 2017 •

edited

Loading

durka Feb 13, 2017 •

edited

Loading

jseyfried Feb 16, 2017 •

edited

Loading

jseyfried Feb 19, 2017 •

edited

Loading