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

check for unbalanced tick pairs in doc-markdown lint #7357

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

ebobrow
Copy link
Contributor

@ebobrow ebobrow commented Jun 15, 2021

fixes #6753

changelog: check for unbalanced tick pairs in doc-markdown lint

@rust-highfive
Copy link

r? @phansch

(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 Jun 15, 2021
@ebobrow ebobrow force-pushed the unbalanced-tick branch 2 times, most recently from 0fdcaa8 to bcf64f1 Compare June 16, 2021 14:16
@xFrednet
Copy link
Member

xFrednet commented Jun 16, 2021

Hey, have you decided where you want to span the lint after your changes? I think that that would affect the implementation a bit. The example output would suggest to me that your change points to the first back-tick. Is that correct?

I was thinking that the implementation could be simplified by spanning the lint over the complete paragraph instead of a single backtick. The bug ticket is actually more about the fact that the wrong spans have been marked. I believe the issue would also count as fixed when the whole thing would be marked with a message like: the backticks are unbalanced, one might be missing a pair. The implementation might then be pretty straight forward, as you only have to check if text contains backticks.

I've done some testing by printing the parsed tokens and these are the results:
/// Normal valid paragraph (Ok)
///
/// One missing `backtick (Lint)
///
/// Double unbalanced backtick from ``here to here` the end (Lint)
///
/// Double balanced back ticks ``start end`` looks good (Ok)
///
/// A longer `paragraph-where` the mistake is `here the `rest-is` valid again (Lint)

Results:

/// Normal valid paragraph (Ok)
    Start(Paragraph)
    Text(Borrowed("Normal valid paragraph (Ok)"))
    End(Paragraph)

/// One missing `backtick (Lint)
    Start(Paragraph)
    Text(Boxed("One missing `backtick (Lint)"))
    End(Paragraph)

/// Double unbalanced backtick from ``here to here` the end (Lint)
    Start(Paragraph)
    Text(Boxed("Double unbalanced backtick from ``here to here` the end (Lint)"))
    End(Paragraph)

/// Double balanced back ticks ``start end`` looks good (Ok)
    Start(Paragraph)
    Text(Borrowed("Double balanced back ticks "))
    Code(Borrowed("start end"))
    Text(Borrowed(" looks good (Ok)"))
    End(Paragraph)

/// A longer `paragraph-where` the mistake is `here the `rest-is` valid again (Lint)
    Start(Paragraph)
    Text(Borrowed("A longer "))
    Code(Borrowed("paragraph-where"))
    Text(Borrowed(" the mistake is "))
    Code(Borrowed("here the "))
    Text(Boxed("rest-is` valid again (Lint)"))
    End(Paragraph)

@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 17, 2021

Yes, currently I have the span only covering the first backtick. The other issue I was having is that if there are other errors after the unbalanced backtick, the spans on their lints are not correct because pulldown_cmark is treating them as code, but clippy is treating them as text. For example, the failing test:

error: you should put `unbalanced_tick` between ticks in the documentation
  --> $DIR/doc.rs:208:23
   |
LL | /// Because of the initial unbalanced_tick pair, the error message is
   |                       ^^^^^^^^^^^^^^^

I'm not sure if this is possible to fix. Should I just stop clippy from linting on a paragraph once it discovers unbalanced ticks?

I was thinking that the implementation could be simplified by spanning the lint over the complete paragraph instead of a single backtick.

That's a good idea, thanks!

@xFrednet
Copy link
Member

I'm not sure if this is possible to fix. Should I just stop clippy from linting on a paragraph once it discovers unbalanced ticks?

This is not necessary but would be a very nice feature. It could otherwise happen that a single change like adding one tick produces several other lint messages. I think that having this lint shadow all/most other doc lint's for one pharagraph would be good! :)

@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 17, 2021

Oops, not so sure what happened there. Sorry about that. Is there a way to remove those extra commits or does it not matter?

@xFrednet
Copy link
Member

xFrednet commented Jun 17, 2021

Yes there is, you can try this: #7270 (review). I know that it worked for someone else :). Rebasing can be a bit tricky at times. Let me know if you need some more help 🙃

@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 17, 2021

Thank you! That worked. Seems like I just went too far back in the commit history.

I think this PR is ready for review now. Thanks for your help!

@ebobrow ebobrow marked this pull request as ready for review June 17, 2021 17:59
@ebobrow ebobrow changed the title WIP: check for unbalanced tick pairs in doc-markdown check for unbalanced tick pairs in doc-markdown lint Jun 17, 2021
@xFrednet
Copy link
Member

xFrednet commented Jun 17, 2021

You're welcome 🙃

I would also take over the review of this PR if that works with you? @flip1995 🙃

r? @flip1995

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The implementation before and after your changes ignores HTML, could you add that as a note to the lint documentation in the Known Problems section.

The rest of the implementation looks good 👍 🙃

tests/ui/doc.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc.rs Show resolved Hide resolved
clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
@ebobrow ebobrow force-pushed the unbalanced-tick branch 2 times, most recently from 562200c to 54467ca Compare June 18, 2021 02:13
@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 18, 2021

Thanks for the review! I've applied your changes. I also had to change some tests to pass CI because there is a 200 line limit on .stderr files. I removed some parts that seemed redundant, but if it's better I can split the tests into two files to keep them all.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Thanks for the review! I've applied your changes. I also had to change some tests to pass CI because there is a 200 line limit on .stderr files. I removed some parts that seemed redundant, but if it's better I can split the tests into two files to keep them all.

It's usually better to add a second test file in such a case, that will also make it easier in the future to expand the tests if needed. For instance, to add lint support in headlines or edge cases.

Thank you for applying the requested changes so quickly. The implementation looks good to me besides the marked places. 👍

clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

I've notices one last false negative by running cargo lintcheck. The current implementation ignores items inside lists, as those use the Start(Item) and End(Item) events. Here is a simple reproduction:

#![deny(clippy::doc_markdown)]

/// - machine: x86_64,
pub fn last_fp() {}
// The x86_64 should trigger the lint validation for identifiers as it not inside backticks

I would approve of this PR once this last FP is fixed and test are added for it. The rest looks wonderful to me 🙃

The tool otherwise noted one correct unbalanced backtick here.

@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 19, 2021

Thank you! While I'm at it, are there any other tags I should include? The full list is fonud here.

@ebobrow
Copy link
Contributor Author

ebobrow commented Jun 19, 2021

Nevermind, none of the other tags can have backticks in them. I've added support for list items.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

LGTM

cc: @flip1995 could you double check my review?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Just one question

clippy_lints/src/doc.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

Thanks!

@bors r=xFrednet,flip1995

@bors
Copy link
Collaborator

bors commented Jun 21, 2021

📌 Commit 20cb1bc has been approved by xFrednet,flip1995

@bors
Copy link
Collaborator

bors commented Jun 21, 2021

⌛ Testing commit 20cb1bc with merge 48fa1dc...

@bors
Copy link
Collaborator

bors commented Jun 21, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet,flip1995
Pushing 48fa1dc to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unbalanced tick marks lead to confusing/inverted error messages from clippy::doc-markdown
6 participants