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

Add error message for anonymous impl for types not declared in the current module #17321

Merged
merged 1 commit into from
Sep 29, 2014

Conversation

apoelstra
Copy link
Contributor

Followup to RFC 57.

Fixes #7607
Fixes #8767
Fixes #12729
Fixes #15060

@alexcrichton
Copy link
Member

What's the error for something like this?

mod Foo {} 
impl Foo {}

@apoelstra
Copy link
Contributor Author

It's still

break.rs:3:1: 3:11 error: duplicate definition of type or module `Foo`
break.rs:3 mod Foo {}
           ^~~~~~~~~~
break.rs:2:1: 2:12 note: first definition of type or module `Foo` here
break.rs:2 impl Foo {}
           ^~~~~~~~~~~
error: aborting due to previous error

@alexcrichton
Copy link
Member

I'm a little wary about closing all those issues without seeing tests for them as well. Do any of these error messages improve?


#7607

pub mod a {
    pub struct Foo { a: uint }
}

pub mod b {
    use a::Foo;
    impl Foo { // ERROR: found value name used as type
        fn bar(&self) { }
    }
}

pub fn main() { }
foo.rs:6:9: 6:15 error: import `Foo` conflicts with type in this module
foo.rs:6     use a::Foo;
                 ^~~~~~
foo.rs:7:5: 9:6 note: note conflicting type here
foo.rs:7     impl Foo { // ERROR: found value name used as type
foo.rs:8         fn bar(&self) { }
foo.rs:9     }
error: aborting due to previous error

#8767

impl B {
}

fn main() {
}
foo.rs:1:6: 1:7 error: found module name used as a type: impl B::B (id=4)
foo.rs:1 impl B {
              ^

#12729

pub struct Foo;

mod bar {
    use Foo;

    impl Foo {
        fn baz(&self) {}
    }
}
fn main() {}
foo.rs:4:9: 4:12 error: import `Foo` conflicts with type in this module
foo.rs:4     use Foo;
                 ^~~
foo.rs:6:5: 8:6 note: note conflicting type here
foo.rs:6     impl Foo {
foo.rs:7         fn baz(&self) {}
foo.rs:8     }
error: aborting due to previous error

@apoelstra
Copy link
Contributor Author

The error from the comments on #7607 you posted changes to "import Foo conflicts with type in this module", which (a) is not my doing, and (b) is not an improvement. The error in the original bug report is improved.

:/ this is embarrasing: #12729 does not change at all. (An error posted in the comments for that one is fixed, which I think is how it got swept up; but that error was actually a dupe of #7607 I think.)

#8767 and #15060 both trigger the improved error message. I'll add compile-fail tests for these.

@apoelstra
Copy link
Contributor Author

I see how to fix the error messages for #12729 and #7607. Should I do those in this PR or submit a separate one?

@alexcrichton
Copy link
Member

It's ok to do them as part of this PR as well

@apoelstra
Copy link
Contributor Author

Sorry for the delay, I was out of town. I've updated to fix the remaining error messages, and added compile-fail tests for all of them.


pub mod b {
use a::Foo; //~ERROR inherent implementations are only allowed on types
// defined in the current module
Copy link
Member

Choose a reason for hiding this comment

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

For the compiletest runner you can't break up the error message on multiple lines. If the line runs long you can tag the test with `// ignore-tidy-linelength.

Also, how come this error message is on the import rather than the impl itself? would it be possible to be on the impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the import error is hit first "import conflicts with existing symbol", which I override by checking that the conflict is actually with an impl. I think if I just suppress the error in this case, it should give the correct error later on when it tries to resolve the impl and sees there is no struct. But then if there is a struct, so the impl "succeeds" I'm not sure what would happen.

As for broken lines, thanks, I'll fix all of those.

Edit: Oh, obviously I can just switch the spans for the error and the note to put the error on the impl. Will do this.

@apoelstra
Copy link
Contributor Author

New push: puts error messages all on one line in compile-fail tests, also moves errors on use statements to the conflicting impl, where they logically belong.

@apoelstra
Copy link
Contributor Author

Oops, missed one two-line compile-fail error message, sorry!

bors added a commit that referenced this pull request Sep 29, 2014
@bors bors closed this Sep 29, 2014
@bors bors merged commit bb58079 into rust-lang:master Sep 29, 2014
@apoelstra apoelstra deleted the error-on-unknown-impl branch September 29, 2014 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants