[clarfonthey]

View all comments

cc #164574

This effectively delegates the meaning of this macro to the compiler team, and only guarantees consistency between individual compiler runs.

It also updates the following locations to specifically reference these guarantees:

• std::panic::Location::{line, column}
• proc_macro::Span::{line, column}

r? libs

[theemathas]

This says to consult "the compiler documentation", but I don't think we have such documentation that talks about what column!() does, right?

[clarfonthey]

Correct. There is no such documentation, but the idea is that if we made it more complicated, such docs could be made.

[estebank]

I believe that instead of "just" changing the documentation to encode rustc's current column rendering nuances, to instead change Location::caller to always use byte offsets from the beginning of the line. That is the only behavior that is really maintainable on stable: unicode codepoints can have different "columnar" position than byte offset position between versions!

[clarfonthey]

I believe that instead of "just" changing the documentation to encode rustc's current column rendering nuances, to instead change Location::caller to always use byte offsets from the beginning of the line. That is the only behavior that is really maintainable on stable: unicode codepoints can have different "columnar" position than byte offset position between versions!

Note that this is particularly contradicted by the fact that proc_macro::Span explicitly offers an (unstable) byte_offset field for this purpose, and we should not overload the meaning of column here for it. The proposal would be to make column explicitly not reliable for any string-indexing and instead only represent a helpful diagnostic for the user, with the compiler able to offer more guarantees if the compiler team wants to stabilize the algorithm it uses for this purpose.

Note that we could add a byte_offset!() macro for this purpose in the future to match what Span does. That said, gonna just FCP this and we can bring up various concerns also:

@rfcbot merge libs

[clarfonthey]

Adding compiler nomination so that the compiler team can be aware of libs' general mentality here that this is a compiler issue, not a libs issue. Compiler diagnostic positions are not stable, and libs' opinion is the stability of this macro should be tied to the compiler diagnostics instead.

[clarfonthey]

Actually, why not add lang here too? Everyone can join the party. I'm not sure if we consider this macro to be a lang issue.

[clarfonthey]

Race condition, Max, but you did comment that after I made a change actually updating those docs to this PR.

[thomcc]

I believe that instead of "just" changing the documentation to encode rustc's current column rendering nuances, to instead change Location::caller to always use byte offsets from the beginning of the line. That is the only behavior that is really maintainable on stable: unicode codepoints can have different "columnar" position than byte offset position between versions!

It seems more beneficial for this to match compiler error output and editor notions of columns than it to be a stable value between versions. That said, I don't feel that strongly about it.

[estebank]

I will insist that the appropriate thing to do here is to "just" change

rust/compiler/rustc_middle/src/mir/consts.rs

Line 547 in df74b1f

caller.col_display as u32 + 1,

to be caller.col.0 as u32 + 1,.

Did a bit of digging and we've been using col_display since at least 2019.

[clarfonthey]

Note that changing the column!() macro to use byte indices, while I would disagree with it, is technically allowed under this FCP. The main thing we're changing here is that it should reflect whatever compiler diagnostics use, so that users can provide diagnostics of their own that are consistent with this.

And, well, the compiler does not use byte indices for this purpose. So, you'd need to change that too.

[nia-e]

@rfcbot cancel

[nia-e]

@rfcbot merge lang,libs

[rust-rfcbot]

@nia-e proposal cancelled.

[Mark-Simulacrum]

Suggested change

	/// The exact counting scheme for columns is only guaranteed to be the same across a single
	/// compiler version and configuration. For example, the handling of the "width" of characters
	/// For example, the handling of the "width" of characters

I think our intent is that we shouldn't make the guarantee on how we count. The compiler needs to provide a sound implementation (and so e.g. cross-crate expansion of the "same" constant may need to be the same for a sound type system), but otherwise it's up to the compiler.

View changes since the review

[tmandry]

I think weakening the guarantees and making the exact behavior of column!() implementation-defined is a good idea. I think #164574 continues to be a bug; constants should have the same value regardless of which crate observes them.

@rfcbot reviewed

[rust-rfcbot]

@nia-e proposal cancelled.

[clarfonthey]

@rfcbot merge lang,libs

EDIT: We can follow up on Zulip: #t-infra > rfcbot broken on restarted FCP @ 💬

[rust-rfcbot]

@clarfonthey has proposed to merge this. The next step is review by the rest of the tagged team members:

• @ChrisDenton
• @Mark-Simulacrum
• @cuviper
• @dtolnay
• @joshtriplett
• @nia-e
• @nikomatsakis
• @scottmcm
• @the8472
• @thomcc
• @tmandry
• @traviscross

Concerns:

• ralfj-can-this-be-soundly-implemented (Change column!() macro guarantees #164645 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
cc @rust-lang/libs-ping: FCP proposed for libs, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[clarfonthey]

Hid the old FCPs and moved over checkboxes so that people don't get confused.

[RalfJung]

I will insist that the appropriate thing to do here is to "just" change

rust/compiler/rustc_middle/src/mir/consts.rs

Line 547 in df74b1f

caller.col_display as u32 + 1,

to be caller.col.0 as u32 + 1,.

Did a bit of digging and we've been using col_display since at least 2019.

@clarfonthey you 👎 'd this and the new docs seem to contradict it.
However, as far as I know it is the only fix anyone has proposed so far for #164574. Do you have any suggestion for how the newly documented guarantee about Location::caller().column() can be implemented in a sound way?

As a lang advisor, I'd like to register an FCP concern (but the bot won't listen to me)
@rfcbot concern can-this-be-soundly-implemented

[traviscross]

Proxying @RalfJung's lang-advisor concern:

@rfcbot concern ralfj-can-this-be-soundly-implemented

[clarfonthey]

@clarfonthey you 👎 'd this and the new docs seem to contradict it. However, as far as I know it is the only fix anyone has proposed so far for #164574. Do you have any suggestion for how the newly documented guarantee about Location::caller().column() can be implemented in a sound way?

So, to clarify here, I did look into the bug a bit more and it turns out that not only did I misunderstand what the proposed code was doing, but also, the proposed code does not fix the issue.

The issue is in the way we store this information in the compiler. Effectively, the ingredients are:

1. All positions in the source are stored as byte offsets (BytePos). In order to convert such a position to a line and column, we must do a lookup.
2. For each source file, we sometimes store the source code and but always store the byte offsets for every line.
3. So, when trying to create a panic::Location, we convert our absolute byte offset to a line number and line-relative byte offset, and attempt to convert that line-relative byte offset to a column.
4. In Loc, the struct we use for constructing this data, we store two column numbers: col_display, which is a computed width used for diagnostics, and col, which is a CharPos count of code points. Both of these must be computed from the source code, since neither can be directly converted from a byte offset.
5. Since we do not always have the source code, if we do not have the source code, we give up and use the line-relative byte offset as both the character-count column and the display column, both of which are incorrect except for non-tab ASCII characters.
6. My favourite part of this is that there is a relevant comment that says this is a "problem for another day," and hello, the day has come.

So, the reality here is that the problem cannot be solved except in one of the following cases:

1. We replace the col: CharPos with col: BytePos and always use byte offsets for columns in reporting. (what @estebank recommended, but for which the provided change would not do)
2. We update the compiler to store both the byte offsets of every line, and the columns for explicit byte offsets where it is deemed relevant, like for panic locations. col: CharPos is deleted and col_display: usize is always used in every case.

Note that the second option effectively amounts to ensuring that, for any given crate, we explicitly save the conversion between byte positions and (line, column) for every panic location so that we can later do this lookup in a consistent manner. And I particularly like this change because it is effectively what we are already doing, just not completely: we save the beginning of line <-> byte offset mappings, but not the byte offset <-> (line, col) mappings for relevant positions.

---

So, tl;dr:

1. I still think that libs and lang should tell users that this is intended to be similar to compiler diagnostics. Ultimately, even if this is not exactly identical to the way compiler diagnostics work, those are unspecified in behaviour and can sometimes make trade-offs anyway, and the end result is that users will think "I should only treat this as a helpful number for errors, and not a load-bearing positional value." In particular, columns may be equivalent for different positions for zero-width characters like combining accents, although you'd need proc macros' granularity of spans to actually accomplish that.
2. Because of Location::column being const, we've effectively enshrined in the implementation, permanently, that the returned column is constant. This cannot be changed and it must remain the same for soundness reasons. The (compiler version, settings) tuple being the requirement for consistency seems reasonable to all the various ways we tell people you should compile code today.
3. I do not think that this cannot be fully implemented as intended, based upon this analysis. Plus, we'd ultimately improve the compiler by ditching this weird char-counting scheme we "guaranteed" that people should absolutely not rely on. That said, I think that such a change in implementation should be associated with a crater run regardless; the removal of the test in std should remove the only thing that might break, I hope.

[RalfJung]

Thanks, that makes sense!

This cannot be changed and it must remain the same for soundness reasons. The (compiler version, settings) tuple being the requirement for consistency seems reasonable to all the various ways we tell people you should compile code today.

Note that -C settings can differ across the crate graph and hence can not be used anywhere in const-eval as that would again be unsound (see e.g. #164601).

I do not think that this cannot be fully implemented as intended, based upon this analysis

Now I am confused, did you not suggest an implementation strategy a few paragraphs above?

[clarfonthey]

Note the double negative, I don't think it's not possible, meaning it's possible.