Skip to content

Reorder span_suggestions to appear below main labels #40851 #41698

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

Closed
gaurikholkar-zz opened this issue May 2, 2017 · 21 comments
Closed

Reorder span_suggestions to appear below main labels #40851 #41698

gaurikholkar-zz opened this issue May 2, 2017 · 21 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gaurikholkar-zz
Copy link

gaurikholkar-zz commented May 2, 2017

In #40851, owing to the the fact that suggestions have been moved inline, no doubt is a good idea. But in the case where the span of the suggestion and the span of the main comment coincide, it currently causes the suggestion to appear before the main comment, which seems suboptimal. We should reorder suggestions so they appear below other labels.

Here is the example,

fn main() {
    let v = vec![String::from("oh no")];
    
    let e = v[0];
}
error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^
  |             |
  |             help: consider using a reference instead `&v[0]`
  |             cannot move out of indexed content

error: aborting due to previous error

In the move_error.rs

let initializer =
                    e.init.as_ref().expect("should have an initializer to get an error");
                if let Ok(snippet) = bccx.tcx.sess.codemap().span_to_snippet(initializer.span) {
                    err.span_suggestion(initializer.span,
                                        "consider using a reference instead",format!("&{}", snippet));
                }
            }

cc @nikomatsakis @estebank

@nikomatsakis
Copy link
Contributor

cc @oli-obk

@nikomatsakis
Copy link
Contributor

This seems like it could make a good mentorship issue. @estebank or @oli-obk, do you want to write up instructions?

@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 2, 2017
@oli-obk
Copy link
Contributor

oli-obk commented May 2, 2017

This is somewhat related to #41695

Here we are already duplicating the error in the main message and again in the note. (See https://github.com/rust-lang/rust/blob/master/src/librustc_borrowck/borrowck/gather_loans/move_error.rs#L138-L144 for the creation of the error and note)

In this case I'd just remove the duplication of the message and only show the suggestion inline. But I don't know the rules around this duplication stuff, although I personally dislike it if the message is duplicated.

Back to the topic:

I think the issue is that https://github.com/rust-lang/rust/blob/master/src/librustc_errors/emitter.rs#L322-L344 sorts the labels according to their start/end column position.

So if we have three labels AA, AB, BB where AA and AB have the same span, we might get AA, AB, BB after sorting, then reverse it to BB, AB, AA, but we wanted to get BB, AA, AB (everything with the same span should keep its original order)

Right now emitter does: sort notes, reverse notes.
So we probably need to: reverse notes, sort notes, reverse notes.

@nikomatsakis
Copy link
Contributor

@oli-obk yes, I agree that this is strongly related to #41695, though I guess that we can have the problem even if the messages aren't the same. It seems like suggestions ought to come last -- I wonder if the general rule should be that you add labels in order of precedence anyhow (i.e., given two labels with same span, we'd prefer the one added first?).

@oli-obk
Copy link
Contributor

oli-obk commented May 17, 2017

This can be tagged E-mentor and E-easy. Instructions are given in my previous comment. Feel free to ask me any questions about this here or on irc.

@petrochenkov petrochenkov added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels May 17, 2017
@imanti
Copy link

imanti commented May 23, 2017

I'd like to take this one if it's not yet assigned.

@oli-obk
Copy link
Contributor

oli-obk commented May 23, 2017

@imanti go ahead! It's all yours

@imanti
Copy link

imanti commented May 25, 2017

@oli-obk the problem with the first example is how the list of annotations is being sorted: since both annotation have the exact same values in all fields except the message string, it will always find the suggestion to be bigger. As you pointed out, in these lines https://github.com/rust-lang/rust/blob/master/src/librustc_errors/emitter.rs#L322-L344 the list is being sorted and then reversed. So no matter how's the order of the list before those two lines, it'll always end up with the suggestion first.

Consider this snippet (it has the real values for the annotations):

fn main() {

#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq)]
pub struct Annotation {
    pub start_col: u32,
    pub end_col: u32,
    pub is_primary: bool,
    pub label: Option<String>,
}

let mut a : Vec<Annotation> = Vec::new();
a.push(Annotation {start_col: 12, end_col: 16, is_primary: true, 
                   label: Some(String::from("help: consider using a reference instead `&v[0]`"))});
a.push(Annotation {start_col: 12, end_col: 16, is_primary: true, 
                   label: Some(String::from("cannot move out of indexed content"))});

println!("{:?}", a);
a.sort();
println!("{:?}", a);
}

I'm not an expert at all in rust code, but I can't think in a way of generically order these messages without knowing which one is a suggestion and which one not. So let me know your ideas.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2017

The sorting could be done with sort_by_key to not sort by the message string, which is a useless criterion for sorting anyway

@imanti
Copy link

imanti commented May 25, 2017

nice!! I'll try it tonigth at home and let you know.

@imanti
Copy link

imanti commented May 26, 2017

@oli-obk sorting by key works as expected:

error[E0507]: cannot move out of indexed content
 --> test_case.rc:4:13
  |
4 |     let e = v[0];
  |             ^^^^
  |             |
  |             cannot move out of indexed content
  |             help: consider using a reference instead `&v[0]`

error: aborting due to previous error

I created branch here with the changes (just two lines).
Do you think it is worth finding other examples where span ordering is wrong before doing the pull request?

@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2017

We'll find those examples when the tests fail 😉 could you run the ui tests and fix them with the command line the test failure gives you? Then we can see the changes in the PR.

@gaurikholkar-zz
Copy link
Author

Is someone still working on this?

@gaurikholkar-zz
Copy link
Author

gaurikholkar-zz commented Jun 28, 2017

@oli-obk I was trying to build , running ./x.py build --stage 1.
Gives me this error

@oli-obk
Copy link
Contributor

oli-obk commented Jun 28, 2017

hmm... I haven't seen that one, But I've noticed that x.py often breaks if the submodules are messed up. You can try running git submodule update --init --recursive.

If it still doesn't work, try starting out from a clean checkout of the rust repository

@gaurikholkar-zz
Copy link
Author

gaurikholkar-zz commented Jul 5, 2017

error[E0507]: cannot move out of indexed content
 --> e1.rs:3:9
  |
3 | let e = v[0];
  |         ^^^^
  |         |
  |         cannot move out of indexed content
  |         help: consider using a reference instead `&v[0]`

error: aborting due to previous error

Suggestions on the output?
Should I open a PR with the changed ui and compile-fail tests?

@gaurikholkar-zz
Copy link
Author

Here are the test fixes

@oli-obk
Copy link
Contributor

oli-obk commented Jul 5, 2017

Should I open a PR with the changed ui and compile-fail tests?

Yes. Please do so. The tests look much more readable after the changes

@evolarium
Copy link

@oli-obk I had a look at this earlier and was wondering if there was any reason to not do a reverse sort instead of the reverse, sort, reverse steps. Like so:

annotations.sort_by(|a,b| b.start_col.cmp(&a.start_col));

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2017

That's a great idea. It just never crossed my mind.

bors added a commit that referenced this issue Jul 20, 2017
Reorder span suggestions to appear below main labels

A fix to #41698

r? @nikomatsakis
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 26, 2017
@estebank
Copy link
Contributor

estebank commented Sep 1, 2017

This was fixed in #43251.

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. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. 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

8 participants