[Pre-RFC] Include scope names in diagnostics

[da-x]

So far, rustc has only printed about filenames and line numbers for warnings and errors. I think it is rather missed, compared to gcc and other compilers, that useful context information such as function names and structs is not included.

I’d like to discuss these possible changes to rustc. I’ve also submitted a reference implementation ( https://github.com/rust-lang/rust/pull/49898 ) that I will rewrite following a merged RFC about this.

Short example:

    In function `test`:
    warning: unused variable: `a`

A larger example (still a bit buggy, for now):

struct OkStruct {
    i: u32,
}

enum OkEnum {
    ItemA,
    ItemB,
}

impl OkStruct {
    fn method() {
        let a = 1;
    }
}

fn test() {
    let a = 1;
}

fn test_nested() {
    let a = 1;

    fn nested() {
        let b = 2;
    }
}

fn main() {
    let a = 1;

    println!("{:?}", OkEnum::ItemA);
}

The output is:

In function `test`:
warning: unused variable: `a`
  --> various-warnings.rs:18:9
   |
18 |     let a = 1;
   |         ^ help: consider using `_a` instead
   |
   = note: #[warn(unused_variables)] on by default

In function `test_nested`:
warning: unused variable: `a`
  --> various-warnings.rs:22:9
   |
22 |     let a = 1;
   |         ^ help: consider using `_a` instead

In function `test_nested::nested`:
warning: unused variable: `b`
  --> various-warnings.rs:25:13
   |
25 |         let b = 2;
   |             ^ help: consider using `_b` instead

In function `main`:
warning: unused variable: `a`
  --> various-warnings.rs:30:9
   |
30 |     let a = 1;
   |         ^ help: consider using `_a` instead

In method `method::method`:
warning: unused variable: `a`
  --> various-warnings.rs:13:13
   |
13 |         let a = 1;
   |             ^ help: consider using `_a` instead

In struct `OkStruct`:
warning: struct is never used: `OkStruct`
 --> various-warnings.rs:1:1
  |
1 | struct OkStruct {
  | ^^^^^^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

In enum `OkEnum`:
warning: variant is never constructed: `ItemB`
 --> various-warnings.rs:8:5
  |
8 |     ItemB,
  |     ^^^^^

In method `method::method`:
warning: method is never used: `method`
  --> various-warnings.rs:12:5
   |
12 |     fn method() {
   |     ^^^^^^^^^^^

In function `test`:
warning: function is never used: `test`
  --> various-warnings.rs:17:1
   |
17 | fn test() {
   | ^^^^^^^^^

In function `test_nested`:
warning: function is never used: `test_nested`
  --> various-warnings.rs:21:1
   |
21 | fn test_nested() {
   | ^^^^^^^^^^^^^^^^

[Nokel81]

This would be very helpful for thinking about where it happens. I would also say that maybe also including the name of any named scopes, ie inner': would also be beneficial.

[kornel]

I’m all for more information, but extra lines are not great. How about adding it to the file/line information?

various-warnings.rs:30:9 in fn main 

[nikomatsakis]

Like the others, I do think having more information is good, but I don’t like how it is presented. I think my preference would be to include that information in the snippet:

warning: unused variable: `a`
  --> various-warnings.rs:18:9: 
   |
16 | fn main() {
...
18 |     let a = 1;
   |         ^ help: consider using `_a` instead
   |
   = note: #[warn(unused_variables)] on by default

or possibly in the --> line:

warning: unused variable: `a`
  --> various-warnings.rs:18:9: in fn main
   |
18 |     let a = 1;
   |         ^ help: consider using `_a` instead
   |
   = note: #[warn(unused_variables)] on by default

I’ve omitted any label for now, since it doesn’t seem worth underlining.

One other thing: if we are going to do this, I think we should be careful to track the information in such a way that it can also be used to localize errors. For example, we’ve long thought about using shorter path names where we can based on where the error occurs and so forth. Similarly, it would be nice to be able to get the names of the parameters in scope. I would have assumed we would “set” the current function using TLS or something for this purpose; but then again, maybe the span scheme is a good way to recover this information.

@eddyb and I discussed this at some point: I’m trying to remember what their idea was. It was good, whatever it was. =)

[nikomatsakis]

cc @ekuber

[H2CO3]

Very nice idea, I support it wholeheartedly. I also agree with the opinion that instead of extra lines, already existing almost-empty lines should be used instead.

[ekuber]

When I first saw this, I thought that it’d look better with @kornel’s suggestion, and it is indeed the least verbose addition. Seeing @nikomatsakis’ first suggestion, where diagnostics implicitly include the line of code for the enclosing scopes, I think it looks great, and would work well, but only in isolation. If you had 200 errors, you’d have at least 400 extra lines of output, with a lot of repetition. This would lead me to lean towards the simpler solution (although with the addition of a complete path, like --> various-warnings.rs:18:9 (foo::bar::baz)).

That being said, a more complex solution could be implemented to reduce the verbosity from including the scope spans. One thing could be unifying diagnostics (like we already do for imports or missing struct fields) so that, for example, unused variables in a given scope are all part of a single diagnostic:

warning: unused variables
  --> various-warnings.rs:18:9
   |
11 | impl OkStruct {
12 |     fn method() {
13 |         let a = 1;
   |             ^ help: consider using `_a` instead
...
17 | fn test() {
18 |     let a = 1;
   |         ^ help: consider using `_a` instead
...
21 | fn test_nested() {
22 |     let a = 1;
   |         ^ help: consider using `_a` instead
23 |
24 |     fn nested() {
25 |         let b = 2;
   |             ^ help: consider using `_b` instead
...
29 | fn main() {
30 |     let a = 1;
   |         ^ help: consider using `_a` instead
   |
   = note: #[warn(unused_variables)] on by default

warning: unused structs
 --> various-warnings.rs:1:1
  |
1 | struct OkStruct {
  | ^^^^^^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

warning: variant is never constructed: `ItemB`
 --> various-warnings.rs:8:5
  |
6 | enum OkEnum {
7 |     ItemA,
8 |     ItemB,
  |     ^^^^^

warning: method is never used: `method`
  --> various-warnings.rs:12:5
   |
11 | impl OkStruct {
12 |     fn method() {
   |     ^^^^^^^^^^^

warning: functions are never used: `test`, `test_nested` and `nested`
  --> various-warnings.rs:17:1
   |
17 | fn test() {
   | ^^^^^^^^^ unused
...
21 | fn test_nested() {
   | ^^^^^^^^^^^^^^^^ unused
...
24 |     fn nested() {
   |     ^^^^^^^^^^^ unused

Another option would be to not show the scopes if there are more than n diagnostics being emitted (not a fan of this option).

I think that this is an great idea that will make it into the compiler, but will need quite a bit of iteration and refinement before it is merged.

[da-x]

If I understand correctly regarding localized errors, do you mean that errors such as these:

error[E0308]: mismatched types
  --> main.rs:4:10
   |
 4 |     test(&Vec::<u32>::new());
   |          ^^^^^^^^^^^^^^^^^^ expected u16, found u32
   |
    = note: expected type `&std::vec::Vec<u16>`
               found type `&std::vec::Vec<u32>`

will have std::vec:: removed, provided that Vec can translate only to std::vec::Vec in the scope for which the error is printed? i.e:

error[E0308]: mismatched types
-> main.rs:4:10
   |
 4 |     test(&Vec::<u32>::new());
   |          ^^^^^^^^^^^^^^^^^^ expected u16, found u32
   |
   = note: expected type `&Vec<u16>`
              found type `&Vec<u32>`

Do you think it is applicable to the current complexity of the type system?

Also, how about adding "where Vec is std::vec::Vec"?

error[E0308]: mismatched types
 --> main.rs:4:10
  |
4 |     test(&Vec::<u32>::new());
  |          ^^^^^^^^^^^^^^^^^^ expected u16, found u32
  |
  = note: expected type `&Vec<u16>`
             found type `&Vec<u32>` where `Vec` is `std::vec::Vec`

A little secret - when I get type errors upon passing a value of a complex type to a new function, I sometimes put () temporarily as the type of a parameter, just for letting rustc to print the full type. Then I copy the full type to the declaration and remove the unneeded paths (or prefix them with :: if I’m lazy). Sort of like a type hole in GHC. It would be nice if rustc will shorten the paths for me

[ekuber]

Also, how about adding "where Vec is std::vec::Vec "?

That is I believe the path we’ll be taking once we finally implement this, looking very similar to your example.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.