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

Improve how rustdoc warnings are displayed #44350

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

GuillaumeGomez
Copy link
Member

cc @rust-lang/dev-tools
r? @nrc

@QuietMisdreavus
Copy link
Member

Doesn't look like it worked. I'm investigating the check_attributes function, it looks like it could be improved.

image

(context: this was meant to catch this issue, where the two sides only differ by an "id" attribute where either both are "name-N" or one is just "name" and the other is "name-1")

@QuietMisdreavus
Copy link
Member

The following version of check_attributes works:

fn check_attributes(attrs1: &HashMap<String, String>, attrs2: &HashMap<String, String>) -> bool {
    /// For strings that match "something-N", returns "something", else returns the whole string
    fn extract_attr(value: &str) -> &str {
        let mut iter = value.rsplitn(2, '-');
        if let (Some(n), Some(tag)) = (iter.next(), iter.next()) {
            if n.parse::<usize>().is_ok() {
                tag
            } else {
                value
            }
        } else {
            value
        }
    }

    if let (Some(id1), Some(id2)) = (attrs1.get("id"), attrs2.get("id")) {
         let tag1 = extract_attr(id1);
         let tag2 = extract_attr(id2);

        tag1 == tag2
    } else {
        !attrs1.contains_key("id") && !attrs2.contains_key("id")
    }
}

@QuietMisdreavus
Copy link
Member

On the other hand, even with that change, it still prints the warning, just now it doesn't print the actual difference. You'll need to float whether to print the warning at all out of render_difference so it doesn't print a worthless warning with no information on it.

@ollie27
Copy link
Member

ollie27 commented Sep 6, 2017

This isn't a false positive, it's just a bug in rustdoc. The issue is that by rendering the Markdown twice USED_ID_MAP gets updated twice for each id.

@GuillaumeGomez
Copy link
Member Author

@ollie27: Yes, that's why we need to catch it (I precised it in the code change I think).
@QuietMisdreavus: Thanks! Updating the code.

@GuillaumeGomez
Copy link
Member Author

Updated.

@QuietMisdreavus
Copy link
Member

Fantastic! This cuts down on rendering warnings by an order of magnitude or more when rendering Rocket's dependencies (the first crate i checked).

@bors r+

@bors
Copy link
Contributor

bors commented Sep 6, 2017

📌 Commit 884989a has been approved by QuietMisdreavus

@ollie27
Copy link
Member

ollie27 commented Sep 6, 2017

I've submitted a PR to actually fix the bug so that none of this is necessary: #44368.

@QuietMisdreavus
Copy link
Member

Ooh, nice!

@bors r-

While i check out this other PR. The "don't print the warning heading" thing is still useful, but the bit about checking the header numbering may be better served with the new PR.

@GuillaumeGomez
Copy link
Member Author

The second commit will still be relevant though. ;) (for other checks)

@QuietMisdreavus
Copy link
Member

Since #44368 solves the "headers with diverging IDs" problem in a nicer fashion than the check introduced here, i want to pull that one in. However, like we've both said, this PR does more than that now. If you can yank out that first commit (good thinking, separating them like that 😛) and just keep the second one, we can make this PR focus on that instead.

@alexcrichton alexcrichton added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 6, 2017
@bors
Copy link
Contributor

bors commented Sep 10, 2017

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

@alexcrichton
Copy link
Member

@GuillaumeGomez ping to make sure this stays on your radar

@GuillaumeGomez
Copy link
Member Author

@alexcrichton: We now need to confirm that the other PR made this one useless. Waiting for your confirmation @QuietMisdreavus! ;)

@QuietMisdreavus
Copy link
Member

Yes, the bit that compared the IDs is superfluous now; #44368 took care of that. However, this PR as a whole isn't "useless", per se:

However, like we've both said, this PR does more than that now. If you can yank out that first commit (good thinking, separating them like that 😛) and just keep the second one, we can make this PR focus on that instead.

@GuillaumeGomez
Copy link
Member Author

@QuietMisdreavus: Doing it!

@GuillaumeGomez GuillaumeGomez force-pushed the id-false-positive branch 2 times, most recently from 5ac512f to 74652b7 Compare September 17, 2017 17:25
@GuillaumeGomez GuillaumeGomez changed the title Remove small id false positive Improve how rustdoc warnings are displayed Sep 17, 2017
@GuillaumeGomez
Copy link
Member Author

Updated.

ref elem_attributes,
ref opposite_elem_attributes,
.. } => {
if !check_attributes(elem_attributes, opposite_elem_attributes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...i thought we agreed that check_attributes didn't need to be there any more >_>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah my bad, completely misunderstood. ><

@QuietMisdreavus
Copy link
Member

The PR was patched up to take out the (now-superfluous) check_attributes, so now it's good. Strictly speaking, it's not really doing anything visible, but in case we find another false positive we need to filter out (and can't find other means to correct it) then this will be good to have.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 20, 2017

📌 Commit 7aa5367 has been approved by QuietMisdreavus

@bors
Copy link
Contributor

bors commented Sep 20, 2017

⌛ Testing commit 7aa5367 with merge 5499683...

bors added a commit that referenced this pull request Sep 20, 2017
…eavus

Improve how rustdoc warnings are displayed

cc @rust-lang/dev-tools
r? @nrc
@bors
Copy link
Contributor

bors commented Sep 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 5499683 to master...

@bors bors merged commit 7aa5367 into rust-lang:master Sep 20, 2017
@GuillaumeGomez GuillaumeGomez deleted the id-false-positive branch September 20, 2017 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants