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

Shorter type-names on type errors #50310

Closed
lukaslueg opened this issue Apr 29, 2018 · 7 comments
Closed

Shorter type-names on type errors #50310

lukaslueg opened this issue Apr 29, 2018 · 7 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lukaslueg
Copy link
Contributor

Type errors (E0308) are a common occurrence in rustc. Whenever I confuse myself about the type of something, I tend to assign it to some x: i32 and let the compiler blame me with a hint about what type x actually is. The note-section attached to the error message contains the fully qualified type name, which is technically as correct as one can get. However, this also produces a lot of clutter, because the type-name that I'm actually interested in is much more concise if types in scope of the offending statement are taken into account. For example:

use std::collections::HashMap;

fn foobar(x: &i32) {
    
}

fn main() {
    let mut foo = HashMap::new();
    foo.insert("foo", Ok(Some("bar")));
    foo.insert("oof", Err("Oh noes!"));
    
    foobar(&foo);
    
    println!("{:#?}", foo);
}

The note says

found type std::collections::HashMap<&str, std::result::Result<std::option::Option<&str>, &str>>

Again, while this is as precise as it can get, most of this is just clutter, especially with deeply nested types: I know that HashMap is from std::collections, I also know that Option refers to std::option::Option. If I had some other type shadowing those names, I'd be aware of it. Reading the note, I have to demangle the type-name to

found type HashMap<&str, Result<Option<&str>, &str>>

which is what the type-name would be if I wanted to write it down, considering all the uses.

Could we at least have an option (or an extra note) for E0308 where the type-names get uncluttered by considering the types that are in scope for the offending statement? This would reduce the intellectual burden and may enable one to just copy&paste the type-name from the note.

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Apr 30, 2018
@estebank
Copy link
Contributor

cc #21934

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 2, 2018
@najamelan
Copy link
Contributor

As I said in #58088 , I would suggest banning all path names from types in error messages and to include use statements in the error message to avoid ambiguity and to allow the user to verify the path if needed.

It's exponentially useful when the same types are referred to more than once, which is quite common. The problem seems also especially bad with generics, which are common in rust as well.

@hellow554
Copy link
Contributor

Sorry, but I'm not the same oppnion as you are. Let's suggest you have to structs called Foo in different paths in your code. You only import/use one at the moment. Now you compile your project and your compiler will complain something related to Foo but because there are no paths you will likely end up digging at the wrong Foo.

I don't think that removing paths from the error is a good solution!

@najamelan
Copy link
Contributor

najamelan commented Apr 26, 2019

@hellow554 That's why I'm suggesting use statements to be included in the error messages, with full, unambiguous paths. I don't know, but I feel that unfortunately the screenshot I posted speaks louder than a thousand words, and I mean literally speaking... that can't be a desirable UX, no? Maybe we can agree on that to start? I find myself writing post processing scripts to make any sense out of the error messages by removing all the paths.

To make a more concrete proposition, it might look something like:

using the types:
- std::collections::HashMap
- std::result::Result
- std::option::Option

[The erronous code]

error: found type HashMap<&str, Result<Option<&str>, &str>> ...

@ExpHP
Copy link
Contributor

ExpHP commented Jun 19, 2019

@najamelan Error messages like that would suggest that it is idiomatic to use std::io::Error;, which is not something I want to see encouraged.

Furthermore there could be cases where multiple identical names show up in the same error message. (e.g. HCons<site::Settings, HCons<ui::Settings, HCons<project::Settings, HNil>>>)

@kamirr
Copy link
Contributor

kamirr commented Apr 3, 2020

Furthermore there could be cases where multiple identical names show up in the same error message. (e.g. HCons<site::Settings, HCons<ui::Settings, HCons<project::Settings, HNil>>>)

That's a non-issue, the compiler should just simplify the paths as much as possible without introducing ambiguities. For example:

std::collections::HashMap<serde_json::Result<u32>, std::io::Result<u32>>

Would become:

use std::collections::HashMap;
use std::io;

HashMap<serde_json::Result<u32>, io::Result<u32>>

There's not much difference here, but it would help considerably in cases like this:

error[E0271]: type mismatch resolving `<impl futures_core::stream::TryStream as futures_core::stream::Stream>::Item == std::result::Result<(bytes::bytes::Bytes, std::net::SocketAddr), std::io::Error>`
   --> src/main.rs:220:13
    |
220 |     let _ = task::spawn(foo);
    |             ^^^^^^^^^^^ expected associated type, found enum `std::result::Result`
    |
    = note: expected associated type `<impl futures_core::stream::TryStream as futures_core::stream::Stream>::Item`
                          found enum `std::result::Result<(bytes::bytes::Bytes, std::net::SocketAddr), std::io::Error>`
    = note: consider constraining the associated type `<impl futures_core::stream::TryStream as futures_core::stream::Stream>::Item` to `std::result::Result<(bytes::bytes::Bytes, std::net::SocketAddr), std::io::Error>` or calling a method that returns `<impl futures_core::stream::TryStream as futures_core::stream::Stream>::Item`
    = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
    = note: required because of the requirements on the impl of `core::future::future::Future` for `futures_util::stream::stream::forward::Forward<impl futures_core::stream::TryStream, futures_util::sink::map_err::SinkMapErr<futures_util::sink::fanout::Fanout<futures_channel::mpsc::UnboundedSender<(bytes::bytes::Bytes, std::net::SocketAddr)>, futures_channel::mpsc::UnboundedSender<(bytes::bytes::Bytes, std::net::SocketAddr)>>, [closure@src/main.rs:218:47: 218:101]>>`

And errors like this are rather common while using futures.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 3, 2020
diagnostics: shorten paths of unique symbols

This is a step towards implementing a fix for rust-lang#50310, and continuation of the discussion in [Pre-RFC: Nicer Types In Diagnostics - compiler - Rust Internals](https://internals.rust-lang.org/t/pre-rfc-nicer-types-in-diagnostics/11139). Impressed upon me from previous discussion in rust-lang#21934 that an RFC for this is not needed, and I should just come up with code.

The recent improvements to `use` suggestions that I've contributed have given rise to this implementation. Contrary to previous suggestions, it's rather simple logic, and I believe it only reduces the amount of cognitive load that a developer would need when reading type errors.

-----

If a symbol name can only be imported from one place, and as long as it was not glob-imported anywhere in the current crate, we can trim its printed path to the last component.

This has wide implications on error messages with types, for example, shortening `std::vec::Vec` to just `Vec`, as long as there is no other `Vec` importable from anywhere.
@lukaslueg
Copy link
Contributor Author

Closed in #73996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants