Stabilize the panic_col feature

[est31]

I've added the panic_col feature in PR #42938.
Now it's time to stabilize it!
Closes #42939.

[rust-highfive]

r? @TimNN

(rust_highfive has picked a reviewer for you, use r? to override)

[TimNN]

cc @rust-lang/libs: There seems nothing more to do here, does this require any kind of FCP?

[alexcrichton]

@rfcbot fcp merge

Yeah for stabilizations we do indeed like to go through FCP!

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler

Concerns:

• 0 resolved by Stabilize the panic_col feature #46762 (comment)

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[dtolnay]

Is it intentional that line numbers in a panic start at 1 but column numbers start at 0? I see that this is what line!() and column!() do as well. What are the advantages? Is it too late to change?

fn main() {
panic!("line 2 column 0");
}

@rfcbot concern 0

[sfackler]

vim seems to start at 1 for lines and columns.

[est31]

Compiler errors in both Rust and gcc start at 1 for columns as well. If you have code like:

fn main() {
 panic!("line 2 column 0");
}

Rust will say the column is 1 and if you do kate src/main.rs:linenum:1 then it jumps to the space, not the panic.

I'd say this is a off-by-one error and a bug in the actual code that emits the panic column info. I'd also say that it's a bug with the column!() macro but what to do about that one might be something for a later discussion.

[est31]

Note that this code gives you the correct column number:

fn main() {
let v = [1,2,3];
v[7];
}

This is because this panic is directly emitted by the compiler and thus uses a separate way to obtain the column.

I guess I'll just add one to the column value for the panic macro case.

[est31]

Not sure how to add a regression test for this. Any ideas?

[sfackler]

You should be able to make an rpass test that sets a panic handle that looks at the column.

[est31]

@sfackler good idea, will do

[dtolnay]

@rfcbot resolved 0

[dtolnay]

We discussed this with the libs team and we would be interested in fixing the column!() macro as well. I filed #46868 to follow up in a separate PR.

[est31]

I've marked this as WIP until the column issue is resolved.

[est31]

#46977 is merged now!

I have added a test to verify that the returned column is 1 based.

@aturon @sfackler could you have a look and r+ the proposed FCP? Thanks!

[alexcrichton]

ping @aturon @sfackler, y'all may be interested in this!

[sfackler]

LGTM

[est31]

@sfackler can you do rfcbot reviewed? Or can someone check it on @sfackler 's behalf?

[carols10cents]

ping @aturon this is waiting for your ticky box here!

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[alexcrichton]

@bors: r+

[bors]

📌 Commit 131a9d8 has been approved by alexcrichton

[goodmanjonathan]

Shouldn't this be since = "1.25" now?

[est31]

Indeed.

[est31]

re-r? @alexcrichton

[alexcrichton]

@bors: r+

[bors]

📌 Commit 2491814 has been approved by alexcrichton

[frewsxcv]

@bors rollup

[bors]

☔ The latest upstream changes (presumably #47308) made this pull request unmergeable. Please resolve the merge conflicts.