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

rustdoc: properly calculate spans for intra-doc link resolution errors #55962

Merged
merged 1 commit into from Nov 19, 2018

Conversation

Projects
None yet
3 participants
@QuietMisdreavus
Member

QuietMisdreavus commented Nov 15, 2018

Fixes #55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the markdown (as the links are only found on the final markdown text) to one relative to the source code (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in trailing whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

@QuietMisdreavus

This comment has been minimized.

Member

QuietMisdreavus commented Nov 15, 2018

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Nov 15, 2018

Thanks!

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 15, 2018

📌 Commit aa3d7a4 has been approved by GuillaumeGomez

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 15, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 15, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

bors added a commit that referenced this pull request Nov 15, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 16 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55871 (codegen_llvm_back: improve allocations)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 16, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 16 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55871 (codegen_llvm_back: improve allocations)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 16, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

bors added a commit that referenced this pull request Nov 16, 2018

Auto merge of #55990 - pietroalbini:rollup, r=pietroalbini
Rollup of 15 pull requests

Successful merges:

 - #55527 (Implement checked_add_duration for SystemTime)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55921 (Add placeholder types)
 - #55936 (save-analysis: be even more aggressive about ignorning macro-generated defs)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)

Failed merges:

r? @ghost

kennytm added a commit to kennytm/rust that referenced this pull request Nov 17, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

bors added a commit that referenced this pull request Nov 17, 2018

Auto merge of #56026 - kennytm:rollup, r=kennytm
Rollup of 17 pull requests

Successful merges:

 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55672 (miri: accept extern types in structs if they are the only field)
 - #55767 (Disable some pretty-printers when gdb is rust-enabled)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55867 (do not panic just because cargo failed)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55999 (Update Cargo submodule)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56017 (std: Add debugging for a failing test on appveyor)

Failed merges:

 - #56010 (fix intra-link resolution spans in block comments)

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 18, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

bors added a commit that referenced this pull request Nov 19, 2018

Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55935 (appveyor: Use VS2017 for all our images)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 19, 2018

Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor usfeful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55935 (appveyor: Use VS2017 for all our images)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Nov 19, 2018

Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
Rollup of 26 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor useful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55935 (appveyor: Use VS2017 for all our images)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)
 - #56059 (Increase `Duration` approximate equal threshold to 1us)

bors added a commit that referenced this pull request Nov 19, 2018

Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
Rollup of 26 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor useful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55935 (appveyor: Use VS2017 for all our images)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)
 - #56059 (Increase `Duration` approximate equal threshold to 1us)

kennytm added a commit to pietroalbini/rust that referenced this pull request Nov 19, 2018

Rollup merge of rust-lang#55962 - QuietMisdreavus:tricky-spans, r=Gui…
…llaumeGomez

rustdoc: properly calculate spans for intra-doc link resolution errors

Fixes rust-lang#55723

When rustdoc is reporting a resolution error for intra-doc links, it needs to convert a span from one relative to the *markdown* (as the links are only found on the final markdown text) to one relative to the *source code* (as the error reporting is meant to show where the line is in the source, so the user can fix it). However, a calculation for how much "offset" to apply had a subtle error: it trimmed the whole line when attempting to account for leading indentation. This caused it to add in *trailing* whitespace into this calculation, which created an incorrect span.

In a lot of situations, this isn't a problem - the span will be shifted in the code slightly, but the warning will still be displayed and mostly legible. However, there is one important situation where this can cause an ICE: multi-byte codepoints. If a shifted span now has a starting point in the middle of a multi-byte codepoint, libsyntax will panic when trying to track what source item it corresponds to. This flew under our radar because trailing whitespace and multi-byte codepoints are both situations that we don't run into in the compiler repo.

(There is one more situation where this can error, that will be much harder to fix: block-style doc comments. Lines in a block-style doc comment have a zero-or-more (usually one) character offset per line, causing this calculation to be way off. I'm punting that to another issue, though...)

bors added a commit that referenced this pull request Nov 19, 2018

Auto merge of #56051 - pietroalbini:rollup, r=pietroalbini
Rollup of 25 pull requests

Successful merges:

 - #55562 (Add powerpc- and powerpc64-unknown-linux-musl targets)
 - #55564 (test/linkage-visibility: Ignore on musl targets)
 - #55827 (A few tweaks to iterations/collecting)
 - #55834 (Forward the ABI of the non-zero sized fields of an union if they have the same ABI)
 - #55857 (remove unused dependency)
 - #55862 (in which the E0618 "expected function" diagnostic gets a makeover)
 - #55867 (do not panic just because cargo failed)
 - #55894 (miri enum discriminant handling: Fix treatment of pointers, better error when it is undef)
 - #55916 (Make miri value visitor useful for mutation)
 - #55919 (core/tests/num: Simplify `test_int_from_str_overflow()` test code)
 - #55923 (reword #[test] attribute error on fn items)
 - #55949 (ty: return impl Iterator from Predicate::walk_tys)
 - #55952 (Update to Clang 7 on CI.)
 - #55953 (#53488 Refactoring UpvarId)
 - #55962 (rustdoc: properly calculate spans for intra-doc link resolution errors)
 - #55963 (Stress test for MPSC)
 - #55968 (Clean up some non-mod-rs stuff.)
 - #55970 (Miri backtrace improvements)
 - #56007 (CTFE: dynamically make sure we do not call non-const-fn)
 - #56011 (Replace data.clone() by Arc::clone(&data) in mutex doc.)
 - #56012 (avoid shared ref in UnsafeCell::get)
 - #56016 (Add VecDeque::resize_with)
 - #56027 (docs: Add missing backtick in object_safety.rs docs)
 - #56043 (remove "approx env bounds" if we already know from trait)
 - #56059 (Increase `Duration` approximate equal threshold to 1us)

@bors bors merged commit aa3d7a4 into rust-lang:master Nov 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@QuietMisdreavus QuietMisdreavus deleted the QuietMisdreavus:tricky-spans branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment