Skip to content
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

Stabilize rustdoc::bare_urls lint #81764

Merged
merged 5 commits into from
Apr 8, 2021
Merged

Stabilize rustdoc::bare_urls lint #81764

merged 5 commits into from
Apr 8, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 4, 2021

Closes #77501. Closes #83598.

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Feb 4, 2021
@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 4, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 4, 2021

@rfcbot fcp merge

This lint has been on nightly for about three months without issues. It seems
useful and in-scope, so this proposes stabilizing it.

What does this do?

  • Suggests turning [x](x) into <x> (including for reference links like [x]\n[x]: x):
    error: unneeded long form for URL
      --> $DIR/url-improvements.rs:3:5
       |
    LL | /// [http://aa.com](http://aa.com)
       |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use an automatic link instead: `<http://aa.com>
    
  • Suggests turning bare urls x into links <x>

Potential concerns

  • non_autolinks is kind of an odd name. Maybe we should rename it to
    missing_links or something?
  • Once we figure out the name, I need to rename the test files, right now they say 'url improvements' which isn't right.

@rfcbot
Copy link

rfcbot commented Feb 4, 2021

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

Concerns:

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!

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

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Feb 4, 2021
@Manishearth
Copy link
Member

I would call it bare_link or something, the standard lint guidance is to say what it's linting against in the name

@jyn514
Copy link
Member Author

jyn514 commented Feb 4, 2021

I would call it bare_link or something, the standard lint guidance is to say what it's linting against in the name

Sorry, I opened the FCP before realizing this does more than warn about bare links - this also warns about [x](x), which isn't a bare link.

@camelid
Copy link
Member

camelid commented Feb 4, 2021

  • non_autolinks is kind of an odd name. Maybe we should rename it to
    missing_links or something?

non_linkified_url? unlinked_url?

@jyn514
Copy link
Member Author

jyn514 commented Feb 4, 2021

  • non_autolinks is kind of an odd name. Maybe we should rename it to
    missing_links or something?

non_linkified_url? unlinked_url?

Both of those are less clear than non_autolinks that [x](x) also warns IMO.

@jyn514
Copy link
Member Author

jyn514 commented Feb 4, 2021

We don't have to change the name, btw, I think it's ok as is, just a little odd.

@camelid
Copy link
Member

camelid commented Feb 4, 2021

IMO non_autolinks doesn't really make sense unless you already know what it does though. Marking the concern:

@rfcbot concern naming

@camelid
Copy link
Member

camelid commented Feb 4, 2021

@rfcbot concern naming

(rfcbot seems pickier about when it recognizes a command)

@ogoffart
Copy link
Contributor

ogoffart commented Feb 5, 2021

Maybe the naming problem suggests that this could be split into two lints:

  • bare_url
  • redundant_link

The rationale to split into different lint is that one is just a style issue whose suggestion can always be applied and makes no difference in the generated docs; while the other is an actual problem making a real difference in the doc, and might have false positive if a clickable link was really not intended.

@Nemo157
Copy link
Member

Nemo157 commented Feb 5, 2021

Should this be delayed till #80527 merges, so that non_autolinks can start as a scoped lint and doesn't need to be registered as removed?

Though, it looks like the non_autolinks lint name has already leaked onto stable; #[warn(non_autolinks)] doesn't cause a "unknown lint" warning. Searching github I see a handful of users, mostly allow(non_autolinks) so any renaming may have to be done carefully if we want to not break their builds (though since it's just a lint change I think we're fine to just change it under the stability policy).

@Manishearth
Copy link
Member

Manishearth commented Feb 5, 2021

Sorry, I opened the FCP before realizing this does more than warn about bare links - this also warns about [x](x), which isn't a bare link.

Honestly I don't think we should care about the latter. Do people actually do that? Bare links are a real problem, this seems pretty rare and overall nearly harmless. Rustdoc/rust have a pretty high bar for lints.

But yeah, this points to needing two lints, if we still think this is necessary.

@rfcbot concern unnecessary-half-of-lint

@LeSeulArtichaut
Copy link
Contributor

@rfcbot concern unnecessary-half-of-lint

(Looks like @rfcbot missed the command)

@Manishearth
Copy link
Member

@rfcbot concern unnecessary-half-of-lint

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 11, 2021

I updated this to remove the "unneceesary long form lint". I'll wait until #80527 is merged before messing with renaming.

@jyn514 jyn514 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 11, 2021
jyn514 added 3 commits April 5, 2021 03:57
This also makes the implementation slightly more efficient by only
compiling the regex once.

See rust-lang#81764 (comment)
for why this was removed; essentially the benefit didn't seem great
enough to deserve a lint.
@GuillaumeGomez
Copy link
Member

Is there anything else to be done here? From the conversation, it seems to be ready to go.

@jyn514
Copy link
Member Author

jyn514 commented Apr 5, 2021

@GuillaumeGomez it's still in FCP: #81764 (comment)

Co-authored-by: Camelid <camelidcamel@gmail.com>
Co-authored-by: Nemo157 <github@nemo157.com>
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 8, 2021
@rfcbot
Copy link

rfcbot commented Apr 8, 2021

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@jyn514
Copy link
Member Author

jyn514 commented Apr 8, 2021

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Apr 8, 2021

📌 Commit ff245da has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2021
Rollup of 6 pull requests

Successful merges:

 - rust-lang#80733 (Improve links in inline code in `core::pin`.)
 - rust-lang#81764 (Stabilize `rustdoc::bare_urls` lint)
 - rust-lang#81938 (Stabilize `peekable_peek_mut`)
 - rust-lang#83980 (Fix outdated crate names in compiler docs)
 - rust-lang#83992 (Merge idents when generating source content)
 - rust-lang#84001 (Update Clippy)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9aed9c1 into rust-lang:master Apr 8, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 8, 2021
@jyn514 jyn514 deleted the lint-links branch April 8, 2021 23:40
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 9, 2021
Fix perf regression in rustdoc::bare_urls

This regressed in rust-lang#81764. After that PR, rustdoc compiled the regex for every single item in the crate: https://perf.rust-lang.org/compare.html?start=125505306744a0a5bb01d62337260a95d9ff8d57&end=2e495d2e845cf27740e3665f718acfd3aa17253e&stat=instructions%3Au

This would have been caught by `clippy::declare_interior_mutable_const` (cc rust-lang#77983).
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Apr 15, 2021
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Jun 20, 2021
Pkgsrc changes:
 * Bump bootstrap requirements to 1.52.1
 * Adjust patches, adapt to upstream changes, adjust cargo checksums
 * If using an external llvm, require >= 10.0

Upsteream changes:

Version 1.53.0 (2021-06-17)
============================

Language
-----------------------
- [You can now use unicode for identifiers.][83799] This allows
  multilingual identifiers but still doesn't allow glyphs that are
  not considered characters such as `#` (diamond) or `<U+1F980>`
  (crab). More specifically you can now use any identifier that
  matches the UAX #31 "Unicode Identifier and Pattern Syntax" standard. This
  is the same standard as languages like Python, however Rust uses NFC
  normalization which may be different from other languages.
- [You can now specify "or patterns" inside pattern matches.][79278]
  Previously you could only use `|` (OR) on complete patterns. E.g.
  ```rust
  let x = Some(2u8);
  // Before
  matches!(x, Some(1) | Some(2));
  // Now
  matches!(x, Some(1 | 2));
  ```
- [Added the `:pat_param` `macro_rules!` matcher.][83386] This matcher
  has the same semantics as the `:pat` matcher. This is to allow `:pat`
  to change semantics to being a pattern fragment in a future edition.

Compiler
-----------------------
- [Updated the minimum external LLVM version to LLVM 10.][83387]
- [Added Tier 3\* support for the `wasm64-unknown-unknown` target.][80525]
- [Improved debuginfo for closures and async functions on Windows MSVC.][83941]

\* Refer to Rust's [platform support page][platform-support-doc] for more
information on Rust's tiered platform support.

Libraries
-----------------------
- [Abort messages will now forward to `android_set_abort_message` on
  Android platforms when available.][81469]
- [`slice::IterMut<'_, T>` now implements `AsRef<[T]>`][82771]
- [Arrays of any length now implement `IntoIterator`.][84147]
  Currently calling `.into_iter()` as a method on an array will
  return `impl Iterator<Item=&T>`, but this may change in a
  future edition to change `Item` to `T`. Calling `IntoIterator::into_iter`
  directly on arrays will provide `impl Iterator<Item=T>` as expected.
- [`leading_zeros`, and `trailing_zeros` are now available on all
  `NonZero` integer types.][84082]
- [`{f32, f64}::from_str` now parse and print special values
  (`NaN`, `-0`) according to IEEE RFC 754.][78618]
- [You can now index into slices using `(Bound<usize>, Bound<usize>)`.][77704]
- [Add the `BITS` associated constant to all numeric types.][82565]

Stabilised APIs
---------------
- [`AtomicBool::fetch_update`]
- [`AtomicPtr::fetch_update`]
- [`BTreeMap::retain`]
- [`BTreeSet::retain`]
- [`BufReader::seek_relative`]
- [`DebugStruct::non_exhaustive`]
- [`Duration::MAX`]
- [`Duration::ZERO`]
- [`Duration::is_zero`]
- [`Duration::saturating_add`]
- [`Duration::saturating_mul`]
- [`Duration::saturating_sub`]
- [`ErrorKind::Unsupported`]
- [`Option::insert`]
- [`Ordering::is_eq`]
- [`Ordering::is_ge`]
- [`Ordering::is_gt`]
- [`Ordering::is_le`]
- [`Ordering::is_lt`]
- [`Ordering::is_ne`]
- [`OsStr::is_ascii`]
- [`OsStr::make_ascii_lowercase`]
- [`OsStr::make_ascii_uppercase`]
- [`OsStr::to_ascii_lowercase`]
- [`OsStr::to_ascii_uppercase`]
- [`Peekable::peek_mut`]
- [`Rc::decrement_strong_count`]
- [`Rc::increment_strong_count`]
- [`Vec::extend_from_within`]
- [`array::from_mut`]
- [`array::from_ref`]
- [`char::MAX`]
- [`char::REPLACEMENT_CHARACTER`]
- [`char::UNICODE_VERSION`]
- [`char::decode_utf16`]
- [`char::from_digit`]
- [`char::from_u32_unchecked`]
- [`char::from_u32`]
- [`cmp::max_by_key`]
- [`cmp::max_by`]
- [`cmp::min_by_key`]
- [`cmp::min_by`]
- [`f32::is_subnormal`]
- [`f64::is_subnormal`]

Cargo
-----------------------
- [Cargo now supports git repositories where the default `HEAD` branch is not
  "master".][cargo/9392] This also includes a switch to the version
  3 `Cargo.lock` format which can handle default branches correctly.
- [macOS targets now default to `unpacked` split-debuginfo.][cargo/9298]
- [The `authors` field is no longer included in `Cargo.toml` for new
  projects.][cargo/9282]

Rustdoc
-----------------------
- [Added the `rustdoc::bare_urls` lint that warns when you have URLs
  without hyperlinks.][81764]

Compatibility Notes
-------------------
- [Implement token-based handling of attributes during expansion][82608]
- [`Ipv4::from_str` will now reject octal format IP addresses in addition
  to rejecting hexadecimal IP addresses.][83652] The octal format can lead
  to confusion and potential security vulnerabilities and [is no
  longer recommended][ietf6943].

Internal Only
-------------
These changes provide no direct user facing benefits, but represent significant
improvements to the internals and overall performance of rustc and
related tools.

- [Rework the `std::sys::windows::alloc` implementation.][83065]
- [rustdoc: Don't enter an infer_ctxt in get_blanket_impls for
  impls that aren't blanket impls.][82864]
- [rustdoc: Only look at blanket impls in `get_blanket_impls`][83681]
- [Rework rustdoc const type][82873]

[83386]: rust-lang/rust#83386
[82771]: rust-lang/rust#82771
[84147]: rust-lang/rust#84147
[84082]: rust-lang/rust#84082
[83799]: rust-lang/rust#83799
[83681]: rust-lang/rust#83681
[83652]: rust-lang/rust#83652
[83387]: rust-lang/rust#83387
[82873]: rust-lang/rust#82873
[82864]: rust-lang/rust#82864
[82608]: rust-lang/rust#82608
[82565]: rust-lang/rust#82565
[80525]: rust-lang/rust#80525
[79278]: rust-lang/rust#79278
[78618]: rust-lang/rust#78618
[77704]: rust-lang/rust#77704
[83941]: rust-lang/rust#83941
[83065]: rust-lang/rust#83065
[81764]: rust-lang/rust#81764
[81469]: rust-lang/rust#81469
[cargo/9298]: rust-lang/cargo#9298
[cargo/9282]: rust-lang/cargo#9282
[cargo/9392]: rust-lang/cargo#9392
[`char::MAX`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.MAX
[`char::REPLACEMENT_CHARACTER`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.REPLACEMENT_CHARACTER
[`char::UNICODE_VERSION`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.UNICODE_VERSION
[`char::decode_utf16`]: https://doc.rust-lang.org/std/primitive.char.html#method.decode_utf16
[`char::from_u32`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_u32
[`char::from_u32_unchecked`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_u32_unchecked
[`char::from_digit`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_digit
[`AtomicBool::fetch_update`]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicBool.html#method.fetch_update
[`AtomicPtr::fetch_update`]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicPtr.html#method.fetch_update
[`BTreeMap::retain`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.retain
[`BTreeSet::retain`]: https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.retain
[`BufReader::seek_relative`]: https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek_relative
[`DebugStruct::non_exhaustive`]: https://doc.rust-lang.org/std/fmt/struct.DebugStruct.html#method.finish_non_exhaustive
[`Duration::MAX`]: https://doc.rust-lang.org/std/time/struct.Duration.html#associatedconstant.MAX
[`Duration::ZERO`]: https://doc.rust-lang.org/std/time/struct.Duration.html#associatedconstant.ZERO
[`Duration::is_zero`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.is_zero
[`Duration::saturating_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_add
[`Duration::saturating_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_mul
[`Duration::saturating_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_sub
[`ErrorKind::Unsupported`]: https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.Unsupported
[`Option::insert`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.insert
[`Ordering::is_eq`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_eq
[`Ordering::is_ge`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_ge
[`Ordering::is_gt`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_gt
[`Ordering::is_le`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_le
[`Ordering::is_lt`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_lt
[`Ordering::is_ne`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_ne
[`OsStr::eq_ignore_ascii_case`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.eq_ignore_ascii_case
[`OsStr::is_ascii`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.is_ascii
[`OsStr::make_ascii_lowercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.make_ascii_lowercase
[`OsStr::make_ascii_uppercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.make_ascii_uppercase
[`OsStr::to_ascii_lowercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.to_ascii_lowercase
[`OsStr::to_ascii_uppercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.to_ascii_uppercase
[`Peekable::peek_mut`]: https://doc.rust-lang.org/std/iter/struct.Peekable.html#method.peek_mut
[`Rc::decrement_strong_count`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.increment_strong_count
[`Rc::increment_strong_count`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.increment_strong_count
[`Vec::extend_from_within`]: https://doc.rust-lang.org/beta/std/vec/struct.Vec.html#method.extend_from_within
[`array::from_mut`]: https://doc.rust-lang.org/beta/std/array/fn.from_mut.html
[`array::from_ref`]: https://doc.rust-lang.org/beta/std/array/fn.from_ref.html
[`cmp::max_by_key`]: https://doc.rust-lang.org/beta/std/cmp/fn.max_by_key.html
[`cmp::max_by`]: https://doc.rust-lang.org/beta/std/cmp/fn.max_by.html
[`cmp::min_by_key`]: https://doc.rust-lang.org/beta/std/cmp/fn.min_by_key.html
[`cmp::min_by`]: https://doc.rust-lang.org/beta/std/cmp/fn.min_by.html
[`f32::is_subnormal`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_subnormal
[`f64::is_subnormal`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_subnormal
[ietf6943]: https://datatracker.ietf.org/doc/html/rfc6943#section-3.1.1
apyrgio added a commit to apyrgio/tindercrypt that referenced this pull request Jun 21, 2021
Fix some `rustdoc::bare_urls`` warnings, due to a `rustdoc` lint that
was recently introduced in stable Rust [1]. By default, bare URLs
trigger a warning and the developer must decide if they want to render
them as links, or as literal values.

In our case, we have two URLs in our footnotes that we want to render as
hyperlinks, so we enclose them in <...>.

[1]: rust-lang/rust#81764
apyrgio added a commit to apyrgio/tindercrypt that referenced this pull request Jun 21, 2021
Fix some rustdoc::bare_urls warnings that appeared due to a rustdoc
lint [1] that was recently introduced in stable Rust [2]. By default,
bare URLs trigger a warning, and the developer must decide if they want
to render them as hyperlinks or as literal values.

In our case, we have two URLs in our footnotes that we want to render as
hyperlinks, so we enclose them in <...>.

[1]: https://doc.rust-lang.org/rustdoc/lints.html#bare_urls
[2]: rust-lang/rust#81764
apyrgio added a commit to apyrgio/tindercrypt that referenced this pull request Jun 21, 2021
Fix some rustdoc::bare_urls warnings that appeared due to a rustdoc
lint [1] that was recently introduced in stable Rust [2]. By default,
bare URLs trigger a warning, and the developer must decide if they want
to render them as hyperlinks or as literal values.

In our case, we have two URLs in our footnotes that we want to render as
hyperlinks, so we enclose them in <...>.

[1]: https://doc.rust-lang.org/rustdoc/lints.html#bare_urls
[2]: rust-lang/rust#81764
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet