Collapse spans

[killercup]

We should consider collapsing mutliple spans/suggestions into one.

As @mcarton mentioned elsewhere, he might add/added lints that yield multiple suggestions to add e.g. #[derive(Default)] (IIRC), so it makes sense to let user accept the complete change in one step.

I'm not sure when to collapse spans, though.

This will look far better with a diff-like output (cf #13).

[mcarton]

The thing is that what was reported “change for i in 0..vec.len() { some(code); } to for <item> in &vec { .. }” (thus losing the loop's body) is now reported as “change i to <item> and 0..vec.len() to &vec”:

{
    "message": "the loop variable `i` is only used to index `vec`.",
    "code": null,
    "level": "error",
    "spans": [
        /* not important here */
    ],
    "children": [
        {
            "message": "lint level defined here",
            "code": null,
            "level": "note",
            "spans": [
                {
                    "file_name": "tests/compile-fail/for_loop.rs",
                    "byte_start": 2639,
                    "byte_end": 2658,
                    "line_start": 90,
                    "line_end": 90,
                    "column_start": 8,
                    "column_end": 27,
                    "is_primary": true,
                    "text": [
                        {
                            "text": "#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)]",
                            "highlight_start": 8,
                            "highlight_end": 27
                        }
                    ],
                    "label": null,
                    "suggested_replacement": null,
                    "expansion": null
                }
            ],
            "children": [

            ],
            "rendered": null
        },
        {
            "message": "consider using an iterator",
            "code": null,
            "level": "help",
            "spans": [ ← this is *one* suggestion, split into two different things to replace
                {
                    "file_name": "tests/compile-fail/for_loop.rs",
                    "byte_start": 3019,
                    "byte_end": 3020,
                    "line_start": 99,
                    "line_end": 99,
                    "column_start": 9,
                    "column_end": 10,
                    "is_primary": true,
                    "text": [
                        {
                            "text": "    for i in 0..vec.len() {",
                            "highlight_start": 9,
                            "highlight_end": 10
                        }
                    ],
                    "label": null,
                    "suggested_replacement": "<item>",
                    "expansion": null
                },
                {
                    "file_name": "tests/compile-fail/for_loop.rs",
                    "byte_start": 3024,
                    "byte_end": 3036,
                    "line_start": 99,
                    "line_end": 99,
                    "column_start": 14,
                    "column_end": 26,
                    "is_primary": true,
                    "text": [
                        {
                            "text": "    for i in 0..vec.len() {",
                            "highlight_start": 14,
                            "highlight_end": 26
                        }
                    ],
                    "label": null,
                    "suggested_replacement": "&vec",
                    "expansion": null
                }
            ],
            "children": [

            ],
            "rendered": "    for <item> in &vec {"
        },
        {
            "message": "for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop",
            "code": null,
            "level": "help",
            "spans": [

            ],
            "children": [

            ],
            "rendered": null
        }
    ],
    "rendered": null
}

In the future that lint should also be able to add a replacement for each vec[i] in the loop body.
That's the only kind of suggestions you should merge I think.
That kind of suggestion is also why I wanted Clippy to give useful placeholder.

You said

Why not just write several spans to replace only certain parts of the input?

, and that's exactly what I want to do and did for that lint. But Clippy cannot guess what the <item> should be named and rustfix should ask the user.

[killercup]

Ah, thanks, that placeholder makes much more sense now! I was quite
confused by this.

This means we still need a good way to detect lints whose children's
suggestions we want to merge (a hard-coded list counts as 'good way',
haha). Or do you think we can just always merge the children at that level?

Martin Carton [email protected] schrieb am Mo. 11. Juli 2016 um
16:40:

The thing is that what was reported “change for i in 0..vec.len() {
some(code); } to for in &vec { .. }” (thus losing the loop's body)
is now reported as “change i to and 0..vec.len() to &vec”:

{
"message": "the loop variable i is only used to index vec.",
"code": null,
"level": "error",
"spans": [
/* not important here */
],
"children": [
{
"message": "lint level defined here",
"code": null,
"level": "note",
"spans": [
{
"file_name": "tests/compile-fail/for_loop.rs",
"byte_start": 2639,
"byte_end": 2658,
"line_start": 90,
"line_end": 90,
"column_start": 8,
"column_end": 27,
"is_primary": true,
"text": [
{
"text": "#[deny(needless_range_loop, explicit_iter_loop, iter_next_loop, reverse_range_loop, explicit_counter_loop)]",
"highlight_start": 8,
"highlight_end": 27
}
],
"label": null,
"suggested_replacement": null,
"expansion": null
}
],
"children": [

        ],
        "rendered": null
    },
    {
        "message": "consider using an iterator",
        "code": null,
        "level": "help",
        "spans": [ ← this is *one* suggestion, split into two different things to replace
            {
                "file_name": "tests/compile-fail/for_loop.rs",
                "byte_start": 3019,
                "byte_end": 3020,
                "line_start": 99,
                "line_end": 99,
                "column_start": 9,
                "column_end": 10,
                "is_primary": true,
                "text": [
                    {
                        "text": "    for i in 0..vec.len() {",
                        "highlight_start": 9,
                        "highlight_end": 10
                    }
                ],
                "label": null,
                "suggested_replacement": "<item>",
                "expansion": null
            },
            {
                "file_name": "tests/compile-fail/for_loop.rs",
                "byte_start": 3024,
                "byte_end": 3036,
                "line_start": 99,
                "line_end": 99,
                "column_start": 14,
                "column_end": 26,
                "is_primary": true,
                "text": [
                    {
                        "text": "    for i in 0..vec.len() {",
                        "highlight_start": 14,
                        "highlight_end": 26
                    }
                ],
                "label": null,
                "suggested_replacement": "&vec",
                "expansion": null
            }
        ],
        "children": [

        ],
        "rendered": "    for <item> in &vec {"
    },
    {
        "message": "for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop",
        "code": null,
        "level": "help",
        "spans": [

        ],
        "children": [

        ],
        "rendered": null
    }
],
"rendered": null

}

In the future that lint should also be able to add a replacement for each
vec[i] in the loop body.
That's the only kind of suggestions you should merge I think.
That kind of suggestion is also why I wanted Clippy to give useful
placeholder https://github.com/Manishearth/rust-clippy/issues/1081.

You said

Why not just write several spans to replace only certain parts of the
input?

, and that's exactly what I want to do and did for that lint. But Clippy
cannot guess what the should be named and rustfix should ask the
user.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/killercup/rustfix/issues/14#issuecomment-231754724,
or mute the thread
https://github.com/notifications/unsubscribe/AABOX48Cz3bgyc1SLoNAjXYertymLU_kks5qUlXngaJpZM4JJaX8
.

[mcarton]

Replacement should be applied together iif they are in the same spans list.