Unclosed html tag lint #77119

GuillaumeGomez · 2020-09-23T18:28:31Z

Part of #67799.

I think @ollie27 will be interested (@Manishearth too since they opened the issue ;) ).

r? @jyn514

jyn514

I might have gone overboard on the comments 😆

compiler/rustc_session/src/lint/builtin.rs

src/test/rustdoc-ui/unclosed-tags.rs

jyn514 · 2020-09-23T19:48:06Z

src/test/rustdoc-ui/unclosed-tags.rs

+/// <script>
+//~^ ERROR Unclosed HTML tag `unknown`
+//~^^ ERROR Unclosed HTML tag `script`
+/// <img><input>


Can you add some more tests?

test these outside of the <script> to make sure they still don't warn

add some nested tags that should be closed and make sure they don't warn inside of <script>

add nested tags that should be closed and make sure they do warn outside of <script> (maybe <del> since that's what inspired me to look at the issue)

src/librustdoc/passes/html_tags.rs

GuillaumeGomez · 2020-09-24T20:17:14Z

Updated!

Manishearth · 2020-09-24T22:07:10Z

We should absolutely not be doing our own HTML parsing here, HTML parsing is extremely complicated even in cases like this one where HTML is rare.

If we can get this info from pulldown that's fine, but a hand rolled bespoke HTML parser is all but guaranteed to have problems.

Manishearth · 2020-09-24T22:09:40Z

A different approach: perhaps we can look for very specific strings, e.g. <script>, <audio>, <video>, i.e. cases where the tag will slurp up and hide all proceeding content, and make sure that the number of such instances is equally matched with the number of closing tags

Manishearth · 2020-09-24T22:19:35Z

Actually thinking about this more I guess it's fine if we parse a very limited subset of HTML. @jyn514 what is your take on this? I'd rather not do anything brittle here.

jyn514 · 2020-09-24T23:07:31Z

Actually thinking about this more I guess it's fine if we parse a very limited subset of HTML. @jyn514 what is your take on this? I'd rather not do anything brittle here.

I think we absolutely shouldn't stabilize the lint with a bespoke HTML parser. I think it's ok to land this as allow by default on nightly, though.

Before merging we should definitely reach out to pulldown-cmark about how hard it would be to add this info in the API.

camelid

Just a nit regarding the lint docs :)

compiler/rustc_session/src/lint/builtin.rs

GuillaumeGomez · 2020-09-25T21:50:28Z

Updated!

src/librustdoc/passes/html_tags.rs

src/doc/rustdoc/src/lints.md

src/librustdoc/passes/html_tags.rs

src/test/rustdoc-ui/invalid-html-tags.stderr

GuillaumeGomez · 2020-09-26T11:48:24Z

No, this is the reason that failed links on intra-doc re-exports don't warn: #77200

@jyn514 The issue is that the reexport item itself should be here, which isn't the case currently for some reason. In my opinion, this check is valid.

GuillaumeGomez · 2020-09-26T13:19:51Z

Updated!

jyn514

The issue is that the reexport item itself should be here, which isn't the case currently for some reason. In my opinion, this check is valid.

Ok, I opened #77230 for this. No need to block this PR on it.

src/librustdoc/passes/html_tags.rs

compiler/rustc_session/src/lint/builtin.rs

src/test/rustdoc-ui/invalid-html-tags.stderr

GuillaumeGomez · 2020-09-26T20:52:03Z

Updated!

jyn514 · 2020-09-26T22:01:41Z

The last thing is adding a test for tags inside <script>, which I think isn't handled currently: #77119 (comment)

Other than that you just need to run x.py fmt.

 -                        let mut r = Range { start: range.start + start_pos, end: range.start + pos };
+                        let mut r =
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
+                            Range { start: range.start + start_pos, end: range.start + pos };

GuillaumeGomez · 2020-10-05T12:07:05Z

So should I mark this PR as fix or not?

jyn514 · 2020-10-05T14:24:41Z

Let's remove it for now so we can get the initial implementation in, we can always close the issue later. r=me with that done.

jyn514 · 2020-10-05T14:52:02Z

@bors r+

We can make fixes in follow up PRs if necessary.

bors · 2020-10-05T14:52:03Z

📌 Commit d3b7b7e has been approved by jyn514

ollie27 · 2020-10-05T23:26:11Z

I'm strongly against adding really precise heuristics for catching broken HTML, we'd need a markdown-compatible HTML parser with recovery and that would be a mess for very marginal gain.

Hmm, I agree that we don't want this code to get too complicated. So yeah, maybe just catching some common cases might be the best we should do. Ideally we could actually fix close the unclosed tags so the rendering of the rest of the page isn't completely broken but that might be getting way too complicated.

I do think #67799 should to be left open at least until this lint is stabilized and warn by default though. At the very least we need a tracking issue for doing that and reusing #67799 seems like the easiest choice.

GuillaumeGomez · 2020-10-06T12:31:58Z

I do think #67799 should to be left open at least until this lint is stabilized and warn by default though. At the very least we need a tracking issue for doing that and reusing #67799 seems like the easiest choice.

Agreed. I'll let @Manishearth update the issue then.

Manishearth · 2020-10-06T20:23:38Z

I do think #67799 should to be left open at least until this lint is stabilized and warn by default though. At the very least we need a tracking issue for doing that and reusing #67799 seems like the easiest choice.

Yeah that seems fine. When this lands someone should do that.

bors · 2020-10-06T20:23:50Z

⌛ Testing commit d3b7b7e with merge f2ef98f6f9ac42571c84c5f0d68c9d295f2216d9...

bors · 2020-10-06T21:10:58Z

💔 Test failed - checks-actions

GuillaumeGomez · 2020-10-06T21:12:57Z

I can't see why it failed. Maybe it's a CI issue? cc @pietroalbini

camelid · 2020-10-06T22:08:26Z

Yeah, I always get confused when it fails at "mark the job as a failure". Where are the logs of the actual failure?

pietroalbini · 2020-10-07T08:27:57Z

The weirdness was probably https://www.githubstatus.com/incidents/t34640ccrmfs.

GuillaumeGomez · 2020-10-07T08:40:53Z

Oh okay. Well, let's try again then!

@bors: retry

bors · 2020-10-07T09:56:57Z

⌛ Testing commit d3b7b7e with merge 8ae3b50...

rust-log-analyzer · 2020-10-07T11:39:36Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

bors · 2020-10-07T11:51:43Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 8ae3b50 to master...

rust-highfive assigned jyn514 Sep 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2020

GuillaumeGomez force-pushed the unclosed-html-tag-lint branch from 5479228 to 0f0632f Compare September 23, 2020 18:30

jyn514 added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 23, 2020

jyn514 suggested changes Sep 23, 2020

View reviewed changes

GuillaumeGomez force-pushed the unclosed-html-tag-lint branch 3 times, most recently from 6d8e95a to c043897 Compare September 24, 2020 20:17

jyn514 mentioned this pull request Sep 24, 2020

Parse HTML tags, not just markdown tags pulldown-cmark/pulldown-cmark#484

Closed

camelid reviewed Sep 25, 2020

View reviewed changes

compiler/rustc_session/src/lint/builtin.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the unclosed-html-tag-lint branch from c043897 to c86c223 Compare September 25, 2020 12:04

jyn514 reviewed Sep 25, 2020

View reviewed changes

GuillaumeGomez force-pushed the unclosed-html-tag-lint branch from 54d6b20 to f9546af Compare September 26, 2020 13:18

jyn514 mentioned this pull request Sep 26, 2020

Rustdoc lints don't fire for pub re-exports #77230

Open

jyn514 reviewed Sep 26, 2020

View reviewed changes

src/librustdoc/passes/html_tags.rs Outdated Show resolved Hide resolved

src/librustdoc/passes/html_tags.rs Show resolved Hide resolved

compiler/rustc_session/src/lint/builtin.rs Show resolved Hide resolved

src/test/rustdoc-ui/invalid-html-tags.stderr Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the unclosed-html-tag-lint branch from f9546af to 2ce2f0a Compare September 26, 2020 20:51

This comment has been minimized.

Sign in to view

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 Sep 27, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 5, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 6, 2020

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 Oct 7, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 7, 2020

bors merged commit 8ae3b50 into rust-lang:master Oct 7, 2020

rustbot added this to the 1.49.0 milestone Oct 7, 2020

GuillaumeGomez deleted the unclosed-html-tag-lint branch October 7, 2020 11:54

bors mentioned this pull request Oct 7, 2020

Add missing_docs lint to the rustdoc lint group #77364

Closed

jyn514 mentioned this pull request Oct 9, 2020

Tracking issue for check_invalid_html_tags #67799

Closed

5 tasks

GuillaumeGomez mentioned this pull request Sep 13, 2022

Change default level of INVALID_HTML_TAGS to warning and stabilize it #101720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unclosed html tag lint #77119

Unclosed html tag lint #77119

GuillaumeGomez commented Sep 23, 2020 •

edited

Loading

jyn514 left a comment

jyn514 Sep 23, 2020 •

edited

Loading

GuillaumeGomez commented Sep 24, 2020

Manishearth commented Sep 24, 2020

Manishearth commented Sep 24, 2020

Manishearth commented Sep 24, 2020

jyn514 commented Sep 24, 2020

camelid left a comment

GuillaumeGomez commented Sep 25, 2020

GuillaumeGomez commented Sep 26, 2020

GuillaumeGomez commented Sep 26, 2020

jyn514 left a comment

GuillaumeGomez commented Sep 26, 2020

jyn514 commented Sep 26, 2020

This comment has been minimized.

GuillaumeGomez commented Oct 5, 2020

jyn514 commented Oct 5, 2020

jyn514 commented Oct 5, 2020

bors commented Oct 5, 2020

ollie27 commented Oct 5, 2020

GuillaumeGomez commented Oct 6, 2020

Manishearth commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

GuillaumeGomez commented Oct 6, 2020

camelid commented Oct 6, 2020

pietroalbini commented Oct 7, 2020

GuillaumeGomez commented Oct 7, 2020

bors commented Oct 7, 2020

rust-log-analyzer commented Oct 7, 2020

bors commented Oct 7, 2020

Unclosed html tag lint #77119

Unclosed html tag lint #77119

Conversation

GuillaumeGomez commented Sep 23, 2020 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 24, 2020

Manishearth commented Sep 24, 2020

Manishearth commented Sep 24, 2020

Manishearth commented Sep 24, 2020

jyn514 commented Sep 24, 2020

camelid left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 25, 2020

GuillaumeGomez commented Sep 26, 2020

GuillaumeGomez commented Sep 26, 2020

jyn514 left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 26, 2020

jyn514 commented Sep 26, 2020

This comment has been minimized.

GuillaumeGomez commented Oct 5, 2020

jyn514 commented Oct 5, 2020

jyn514 commented Oct 5, 2020

bors commented Oct 5, 2020

ollie27 commented Oct 5, 2020

GuillaumeGomez commented Oct 6, 2020

Manishearth commented Oct 6, 2020

bors commented Oct 6, 2020

bors commented Oct 6, 2020

GuillaumeGomez commented Oct 6, 2020

camelid commented Oct 6, 2020

pietroalbini commented Oct 7, 2020

GuillaumeGomez commented Oct 7, 2020

bors commented Oct 7, 2020

rust-log-analyzer commented Oct 7, 2020

bors commented Oct 7, 2020

GuillaumeGomez commented Sep 23, 2020 •

edited

Loading

jyn514 Sep 23, 2020 •

edited

Loading