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

unintended_html_in_doc_comment - add to recommended set #826

Closed
kevmoo opened this issue Jun 25, 2024 · 18 comments · Fixed by dart-lang/lints#211
Closed

unintended_html_in_doc_comment - add to recommended set #826

kevmoo opened this issue Jun 25, 2024 · 18 comments · Fixed by dart-lang/lints#211

Comments

@kevmoo
Copy link
Member

kevmoo commented Jun 25, 2024

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.

@lrhn
Copy link
Member

lrhn commented Jul 22, 2024

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?

@devoncarew
Copy link
Member

cc @kallentu and @srawlins who may have the most context with this lint

@srawlins
Copy link
Member

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.

@lrhn
Copy link
Member

lrhn commented Jul 23, 2024

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.

@devoncarew
Copy link
Member

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

@lrhn
Copy link
Member

lrhn commented Aug 6, 2024

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.)

@pq
Copy link
Member

pq commented Aug 6, 2024

/fyi @kallentu @srawlins @parlough

@devoncarew
Copy link
Member

The lint is still listed as experimental. We'll want to graduate it. ...

Thanks, make sense. I opened dart-lang/sdk#59501 for the doc work on the lint.

@kallentu
Copy link
Member

kallentu commented Aug 8, 2024

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.

@kallentu
Copy link
Member

Docs should be updated now.
We can move forward with including it in the core set.

@devoncarew
Copy link
Member

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.

@goderbauer
Copy link
Contributor

We also ran into issues with this lint in flutter: dart-lang/sdk#59516

@lrhn
Copy link
Member

lrhn commented Sep 4, 2024

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.)

@devoncarew
Copy link
Member

From additional discussion, we think the false positive on generics ([Tween<Offset>] from dart-lang/sdk#59516) 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.

@lrhn
Copy link
Member

lrhn commented Sep 6, 2024

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-dating

where 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.

@devoncarew
Copy link
Member

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.

@lrhn
Copy link
Member

lrhn commented Sep 11, 2024

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&lt;Li&gt;] 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.)

    @devoncarew
    Copy link
    Member

    With the fix in https://dart-review.googlesource.com/c/sdk/+/384840 I think the last blocker for adding this lint has been addressed.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    8 participants