-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Add lint for intra link resolution failure #51382
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
Nice! Can you add a help text telling people how to escape stuff like |
Good idea! |
This comment has been minimized.
This comment has been minimized.
70ae786
to
6135267
Compare
@killercup: I added the help to escape the character. Should be working fine now. |
src/librustdoc/clean/mod.rs
Outdated
}; | ||
diag.help("to escape `[` and `]` characters, either put them into \"`[]`\" or \ | ||
use HTML values `[` and `]`"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd write
put them between back ticks (like "`[]`") or
Also, doesn't backslash escaping work, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It totally does haha. Just writing this then!
This comment has been minimized.
This comment has been minimized.
@killercup: What about this help message? |
This comment has been minimized.
This comment has been minimized.
}; | ||
diag.help("to escape `[` and `]` characters, just add '\\' before them like \ | ||
`\\[` or `\\]`"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should better use span_suggestion
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many cases, it'll really be a link resolution failure. Not sure it'd be a nice idea to suggest that to remove the warning, they should not use auto-linking. :p
a8edbc6
to
39061ff
Compare
@GuillaumeGomez new help text is great, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments. I'm glad you got this working!
src/librustc/lint/builtin.rs
Outdated
declare_lint! { | ||
pub INTRA_LINK_RESOLUTION_FAILURE, | ||
Warn, | ||
"warn about documentation intra links resolution failure" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to move away from calling this feature "intra links" because it doesn't mean much on its own. Instead, i've started opting for "type links". I feel like that says a bit more about the feature. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you prefer, I don't have a strong opinion on the naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intra links […] doesn't mean much on its own
What is this "intra links" thing you are talking about? Did you mean "intra doc links" which has actual context for what the links are between/within and for which we also have an RFC?
(And don't get me started on how these can also link to items that are, strictly speaking, not types!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuietMisdreavus Why "type links"? It also works for functions and macros etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soooooo, should I rename it? If so, how? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I have never, since the RFC was accepted, referred to this feature with its full title in internal discussions. The shortened title has been used so much that the full one has fallen out of our collective memory, as evidenced in this lint title, this PR title, and my comment here.
- Calling it "type links" implies that it can link to "things that rustc cares about", even if the name is overly restrictive. Yes, i'm aware that the links can go to more than just "types".
INTRA_DOC_LINK_RESOLUTION_FAILURES
is exceedingly verbose, but a quick look atrustc +nightly -W help
says it's in good company, so use that.
lint_array!( | ||
WHILE_TRUE, | ||
BOX_POINTERS, | ||
NON_SHORTHAND_FIELD_PATTERNS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did this list of lints come from? I see a similar list in register_builtins
farther down, but this one seems to be missing some of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this file. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, now i see it. Thanks for pointing that out.
src/libstd/sys/windows/ext/io.rs
Outdated
@@ -10,6 +10,8 @@ | |||
|
|||
#![stable(feature = "rust1", since = "1.0.0")] | |||
|
|||
//! Windows' IO module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these Windows IO comments doing in a rustdoc PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It broke the build otherwise. There is a #![deny(missing_docs)]
and rustdoc broke because of missing docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't have happened. All of std::sys::windows
(and std::os
, where it's re-exported) should be covered by #[allow(missing_docs)]
:
If not having these docs broke the build, that means we just broke #[allow(missing_docs)]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I'll look further then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the problem is a bit more complicated than that. Parser doesn't take into account windows mods and therefore doesn't apply its rules. However, rustdoc applies a cgf which it to access... ext
! Therefore, ext
doesn't have what the windows.rs
has.
4220ff4
to
3ab2f40
Compare
3ab2f40
to
231c61a
Compare
| | ||
11 | #![deny(intra_doc_link_resolution_failure)] | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
= help: to escape `[` and `]` characters, just add '/' before them like `/[` or `/]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really how it comes out when the error appears? Or is it an artifact of the UI testing? It would be really worrying if we were accidentally suggesting the wrong thing.
EDIT: I mean it's showing a /
when it should be showing a \
, based on the help messages in the code above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuietMisdreavus Just an artifact of UI test normalization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew, i was getting nervous there. Imperio said he got the proper output when he ran the test locally, too, so i'll let this slide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, last-minute thing i noticed.
} else { | ||
vec![] | ||
}, | ||
lint_cap: Some(lint::Forbid), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation: lint_cap
here is what was suppressing the compilation warnings that i mentioned in #41574 (comment). Changing this probably will make the --display-warnings
flag useful on doc builds.
.filter_map(|lint| { | ||
if lint.name == warnings_lint_name || | ||
lint.name == intra_link_resolution_failure_name { | ||
None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to be excluding warnings
in the lints being allow
ed here? It seems to be doing the opposite of the old code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the opposite, we're allowing everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that allowing a bunch of lints individually will act the same as allowing warnings
all at once? Can we be sure that all warnings that can occur during a rustdoc run will always be placed in one of the lists you're chaining here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set all warnings at once using this one, then the intra-doc lint stop working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhh, that makes sense, i forgot about that. In that case, this is fine.
@bors r+ |
Legit failure
@bors r- |
Ah damn, same issue as windows. |
Let's hope there won't be another platform failure... @bors: r=QuietMisdreavus |
📌 Commit 6a03884 has been approved by |
⌛ Testing commit 6a03884 with merge 3609834b0302af24ecf798e50ef6cd8b351f1b71... |
💔 Test failed - status-appveyor |
clippy is failing to build? I'm not sure it's related to my changes this time... cc @Manishearth |
Clippy failing is expected. However, there were a few doctests in the reference and nomicon that failed?
The failures in question
...but those also look like they should have been caught before this? Not totally sure what's going on. |
@bors retry |
Add lint for intra link resolution failure This PR is almost done, just remains this note: ``` note: requested on the command line with `-W intra-link-resolution-failure` ``` I have no idea why my lint is considered as being passed through command line and wasn't able to find where it was set. If anyone has an idea, it'd be very helpful! cc @QuietMisdreavus
☀️ Test successful - status-appveyor, status-travis |
This PR is almost done, just remains this note:
I have no idea why my lint is considered as being passed through command line and wasn't able to find where it was set. If anyone has an idea, it'd be very helpful!
cc @QuietMisdreavus