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

Implement non-mod.rs mod statements #46531

Merged
merged 4 commits into from
Dec 21, 2017
Merged

Conversation

cramertj
Copy link
Member

@cramertj cramertj commented Dec 6, 2017

Fixes #45385, cc #44660

This will fail tidy right now because it doesn't recognize my UI tests as feature-gate tests. However, I'm not sure if compile-fail will work out either because compile-fail usually requires there to be error patterns in the top-level file, which isn't possible with this feature. What's the recommended way to handle this?

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@zackmdavis
Copy link
Member

because compile-fail usually requires there to be error patterns in the top-level file, which isn't possible with this feature. What's the recommended way to handle this?

There's a special rustc_error attribute (gated by rustc_attrs) that you can use to make the test artificially fail.

@est31
Copy link
Member

est31 commented Dec 6, 2017

This will fail tidy right now because it doesn't recognize my UI tests as feature-gate tests.

You can also add the ui directory to the places where tidy searches for feature gate tests. Look into src/tools/tidy/src/features.rs. Then you need to name the files after the format that tidy recognizes.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 6, 2017
@nikomatsakis
Copy link
Contributor

There's a special rustc_error attribute (gated by rustc_attrs) that you can use to make the test artificially fail.

You can use // must-compile-successfully, which I believe is now the preferred approach.

@nikomatsakis
Copy link
Contributor

That said, what @est31 said sounds better. =)

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

Owned,
Owned {
// None if `mod.rs`, `Some(path)` if we're in
// `foo.rs` and there's a corresponding `foo/...`
Copy link
Contributor

@nikomatsakis nikomatsakis Dec 6, 2017

Choose a reason for hiding this comment

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

This comment makes it sound like we check for the existence of such a directory, but we don't right? Maybe "Some("foo") if we're in the top-level of foo.rs"?

@est31
Copy link
Member

est31 commented Dec 6, 2017

btw this PR motivated me to move over some of the currently existing feature gate tests: #46532

for &span in &*self.context.parse_sess.non_modrs_mods.borrow() {
gate_feature_post!(
&self, non_modrs_mods, span,
"mod declarations in non-mod.rs files are experimental");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we want to add some extra notes here, given that this is a common source of confusion? Something like:

on stable builds, rename foo.rs to foo/mod.rs to enable submodules

not sure how hard that would be to do

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

looks pretty good to me -- I'd like to see a ui test for what happens when there is a mod foo with no matching file (what kind of error do we give)

@cramertj
Copy link
Member Author

cramertj commented Dec 7, 2017

@nikomatsakis

I'd like to see a ui test

I've added one, but currently it just suggests to try creating the file, and once the file is created it will give you the stability error.

|
11 | pub mod baz;
| ^^^
= help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "$DIR/auxiliary/foo"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 7, 2017

📌 Commit 5a821e3 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 8, 2017

⌛ Testing commit 5a821e3a7738ee9d04ac1fd8c91956309f425f7d with merge c41ec675c6e876c7f032cb9179af6750638f65be...

@bors
Copy link
Contributor

bors commented Dec 8, 2017

💔 Test failed - status-appveyor

@cramertj
Copy link
Member Author

cramertj commented Dec 8, 2017

failures:
---- [ui] ui\invalid-module-declaration\invalid-module-declaration.rs stdout ----
	normalized stderr:
error[E0583]: file not found for module `baz`
  --> $DIR/auxiliary/foo/bar.rs:11:9
   |
11 | pub mod baz;
   |         ^^^
   |
   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "C:/projects/rust/src/test/ui/invalid-module-declaration/auxiliary/foo"
error: aborting due to previous error
expected stderr:
error[E0583]: file not found for module `baz`
  --> $DIR/auxiliary/foo/bar.rs:11:9
   |
11 | pub mod baz;
   |         ^^^
   |
   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "$DIR/auxiliary/foo"
error: aborting due to previous error
diff of stderr:
 error[E0583]: file not found for module `baz`
   --> $DIR/auxiliary/foo/bar.rs:11:9
    |
 11 | pub mod baz;
    |         ^^^
    |
-   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "$DIR/auxiliary/foo"
+   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "C:/projects/rust/src/test/ui/invalid-module-declaration/auxiliary/foo"
 
 error: aborting due to previous error

Looks like the $DIR paths aren't handled correctly on Windows. Anyone know what's up here? The existing UI tests seem to all use $DIR with no problem. Is it maybe something to do with the fact that it's in quotes?

@kennytm
Copy link
Member

kennytm commented Dec 8, 2017

@cramertj The quotes shouldn't matter. However, if the original output is C:/projects/rust/src/… instead of C:\projects\rust\src\… it won't perform the replacement.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2017
@cramertj
Copy link
Member Author

cramertj commented Dec 8, 2017

@kennytm Hm... it seems like the slashes are going the right direction, though. It's in MSYS. The diff is this part:

-   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "$DIR/auxiliary/foo"
+   = help: name the file either bar/baz.rs or bar/baz/mod.rs inside the directory "C:/projects/rust/src/test/ui/invalid-module-declaration/auxiliary/foo"

It looks like the replacement just isn't happening.

@kennytm
Copy link
Member

kennytm commented Dec 8, 2017

@cramertj just to clarify, the original needs to be backslashes.

@bors
Copy link
Contributor

bors commented Dec 10, 2017

⌛ Testing commit af87dd4131b44e5c0f94aacb3270d08794c9ed2f with merge 2b9c672a1d2cc46ceb916b6b3d164f754eb5a3b0...

@bors
Copy link
Contributor

bors commented Dec 10, 2017

💔 Test failed - status-travis

@cramertj
Copy link
Member Author

Failed pretty-printing tests:

[01:51:02] failures:
[01:51:02] 
[01:51:02] ---- [pretty] run-pass/non_modrs_mods/non_modrs_mods.rs stdout ----
[01:51:02] 	
[01:51:02] error: pretty-printed source does not typecheck
[01:51:02] status: exit code: 101
[01:51:02] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "-" "-Zno-trans" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/non_modrs_mods/non_modrs_mods.pretty-out" "--target=x86_64-unknown-linux-gnu" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/non_modrs_mods/non_modrs_mods.stage2-x86_64-unknown-linux-gnu.pretty.aux" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
[01:51:02] stdout:
[01:51:02] ------------------------------------------
[01:51:02] 
[01:51:02] ------------------------------------------
[01:51:02] stderr:
[01:51:02] ------------------------------------------
[01:51:02] error[E0433]: failed to resolve. Could not find `inner_modrs_mod` in `modrs_mod`
[01:51:02]   --> <anon>:19:16
[01:51:02]    |
[01:51:02] 19 |     modrs_mod::inner_modrs_mod::innest::foo();
[01:51:02]    |                ^^^^^^^^^^^^^^^ Could not find `inner_modrs_mod` in `modrs_mod`
[01:51:02] 
[01:51:02] error[E0433]: failed to resolve. Could not find `inner_foors_mod` in `modrs_mod`
[01:51:02]   --> <anon>:20:16
[01:51:02]    |
[01:51:02] 20 |     modrs_mod::inner_foors_mod::innest::foo();
[01:51:02]    |                ^^^^^^^^^^^^^^^ Could not find `inner_foors_mod` in `modrs_mod`
[01:51:02] 
[01:51:02] error[E0433]: failed to resolve. Could not find `inner_modrs_mod` in `foors_mod`
[01:51:02]   --> <anon>:21:16
[01:51:02]    |
[01:51:02] 21 |     foors_mod::inner_modrs_mod::innest::foo();
[01:51:02]    |                ^^^^^^^^^^^^^^^ Could not find `inner_modrs_mod` in `foors_mod`
[01:51:02] 
[01:51:02] error[E0433]: failed to resolve. Could not find `inner_foors_mod` in `foors_mod`
[01:51:02]   --> <anon>:22:16
[01:51:02]    |
[01:51:02] 22 |     foors_mod::inner_foors_mod::innest::foo();
[01:51:02]    |                ^^^^^^^^^^^^^^^ Could not find `inner_foors_mod` in `foors_mod`
[01:51:02] 
[01:51:02] error[E0433]: failed to resolve. Could not find `inner_modrs_mod` in `attr_mod`
[01:51:02]   --> <anon>:23:15
[01:51:02]    |
[01:51:02] 23 |     attr_mod::inner_modrs_mod::innest::foo();
[01:51:02]    |               ^^^^^^^^^^^^^^^ Could not find `inner_modrs_mod` in `attr_mod`
[01:51:02] 
[01:51:02] error: aborting due to 5 previous errors
[01:51:02] 
[01:51:02] 
[01:51:02] ------------------------------------------
[01:51:02] 
[01:51:02] thread '[pretty] run-pass/non_modrs_mods/non_modrs_mods.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2770:8
[01:51:02] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:51:02] 
[01:51:02] 
[01:51:02] failures:
[01:51:02]     [pretty] run-pass/non_modrs_mods/non_modrs_mods.rs
[01:51:02] 
[01:51:02] test result: �[31mFAILED�(B�[m. 2810 passed; 1 failed; 40 ignored; 0 measured; 0 filtered out

@nikomatsakis
Copy link
Contributor

@cramertj do you know what might be causing that? (I don't know, not without looking into it)

@cramertj
Copy link
Member Author

No, but I'll investigate this afternoon.

@bors
Copy link
Contributor

bors commented Dec 13, 2017

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

@nikomatsakis
Copy link
Contributor

After some discussion on IRC -- our hypothesis is that the pretty typeck runner is putting files in the wrong place. Most likely outcome is that we will skip pretty printing for this test, but @cramertj is going to confirm the hypothesis.

@cramertj
Copy link
Member Author

Rebased and ignored pretty-printing, as this is issue #37195.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 20, 2017

📌 Commit 1d5977b has been approved by nikomatsakis

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2017
@tcr tcr mentioned this pull request Dec 21, 2017
@bors
Copy link
Contributor

bors commented Dec 21, 2017

⌛ Testing commit 1d5977b with merge 957dc8d...

bors added a commit that referenced this pull request Dec 21, 2017
Implement non-mod.rs mod statements

Fixes #45385, cc #44660

This will fail tidy right now because it doesn't recognize my UI tests as feature-gate tests. However, I'm not sure if compile-fail will work out either because compile-fail usually requires there to be error patterns in the top-level file, which isn't possible with this feature. What's the recommended way to handle this?
@bors
Copy link
Contributor

bors commented Dec 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 957dc8d to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants