`unintended_html_in_doc_comment` - add to recommended set
Closed this issue · 18 comments
Describe the rule you'd like to see added and to what rule set
https://dart.dev/tools/linter-rules/unintended_html_in_doc_comment
Additional context
It's almost ALWAYS helpful. Trouble imagining false negatives.
Need to wait until Dart 3.5 is stable, though.
Does it have exceptions? (Fx <sub> and <sup> have no markdown equivalent in Commonmark or GitHub markdown, and HTML is valid inside Commonmark).
The name is "unintended...". It's there intended uses? If so, what are they?
Is there a way to opt out?
Can I do
/// If _X_<sub>2</sub> If not, can I do <!-- ignore:... --> to ignore for this or the next line, or will I have to ignore_for_file?
Does it have exceptions?
<sub> and <sup> are in the allowlist.
The name is "unintended...". It's there intended uses? If so, what are they?
Just the allowlist, I suppose.
Is there a way to opt out?
Nothing beyond the standard recommended way (with // ignore or // ignore_for_file comments).
Can I do
/// If _X_<sub>2</sub>
Yes.
Ack. The documentation should probably at least mention that there is an allowlist, preferably include the list.
May also want to say why
/// <http://foo.bar.baz>
is considered "good" and that it's not parsed as HTML, counter to what the first sentence says.
You are allowed to use custom tags in Commonmark, but I don't see any use for that in dartdoc, so there is probably no need for ignores.
(If a need shows up, we may consider allowing ignores inside <!-- --> comments, unless we already allow interleaving // comments inside /// doc blocks.)
👍 if the docs are updated.
From an off-line discussion:
- google3 data indicates most lints are triggering on real issues
- between now and the next publish, it would be nice to get some data on ext. impact (i.e., any false positives?)
- this does prevent real issues - preventing dartdoc from generating malformed or unintended html
resolution: OK to include in core w/ the next major release
OK to include in core w/ the next major release
The lint is still listed as experimental. We'll want to graduate it.
Also, the documentation/specification must be updated before we do that. It's misleading as written.
(I treat the documentation of lints as their specification, since there is no other place to find a specification. The documentation should be good enough to be used as a specification.)
The lint is still listed as experimental. We'll want to graduate it. ...
Thanks, make sense. I opened dart-lang/linter#5050 for the doc work on the lint.
Oh yeah, thanks for looking at it @lrhn.
I noticed the first line of the documentation is just completely wrong haha. We didn't catch that last review.
I'll update it and send it out.
Docs should be updated now.
We can move forward with including it in the core set.
We're trying on the lint in package:dart_flutter_team_lints; it looks like there are some false positives - relatively infrequent html entites that are actually used in dartdoc comments - things we don't want escaped. Srujan hit this in package:web and opened dart-lang/sdk#56450 to track.
We also ran into issues with this lint in flutter: dart-lang/linter#5065
I'm satisfied with docs and implementation. If there are any remaining edge cases, we can try to fix them too.
(But if we don't use a full Markdown parser for the input, just line-by-line RegExp matching, we will always be able to create false positives.)
From additional discussion, we think the false positive on generics ([Tween<Offset>] from dart-lang/linter#5065) should be addressed before we can add this lint to the set. In that tween example, we'd be happy with the Tween type being linked, but would not need the generic parameter to be.
What would the desired solution be? Ignore any <...> inside [...]? (I can definitely add that to the RegExps.)
But it should probably only be ignored inside a [...] not followed by [ or (.
Which means matching (?<!\])\[[^]]*\](?=[^(\[]), and ignoring its content like we do for code blocks and HTML comments.
Basically, treat [....] that is not a Markdown link, so not [...](...) or [...][...], as a likely DartDoc source name link, and ignore it.
That might have false positives for:
Help with [C<up>14</up>] dating.
[C<up>14</up>]: http://example.com/carbon-datingwhere I miswrote <sup>. This is (per specification) a Markdown link. You can also write it as [C<up>14</up>][], but you don't have to in plain Markdown.
If you do in DartDoc, then just ignoring a stand-alone [...] seems like a good solution.
That might have false positives for:
Hmm, I'm not sure we'll run across those usages in practice, whereas we have run into false positives for linked generics.
Basically, treat [....] ... as a likely DartDoc source name link, and ignore it.
Yes, I think that's reasonable; for this lint we would assume that any angle brackets in there are used for generics and not as html. Separately, the analyzer could itself warn if those references couldn't resolve.
I'm not sure we are handling the embedded HTML correctly, or at least consistently.
If I write /// A link to a [List<Li>] that is great on a declaration, and hover the declared name in VSCode, the rendered DOC is shown as
A link to a [List
] that is great.
That is, the analysis server is serving HTML that interprets a [List<int>] as having an <int> HTML tag.
Since that's not a recognized tag, it's erased, so you never notice, but if you have a [List<Table>], you will get it rendered as HTML.
In that case, the lint is correct. A [List<int>] is an unrecognized HTML tag, and it's interpreted as such by some tools.
The DartDoc tool, as run by dart doc, does generate [List<Li>] in the HTML, so it works correctly there.
This is probably a bug in the analyzer's DartDoc interpretation, not a reason to not adapt the lint.
(It's also why [List<int>] shows as [List] in the analysis server docs, and [List<int>] in the generated DartDoc.)
With the fix in https://dart-review.googlesource.com/c/sdk/+/384840 I think the last blocker for adding this lint has been addressed.