Parse HTML properly in Scaladoc - #25681
Conversation
| lazy val commonBootstrappedSettings = commonDottySettings ++ Seq( | ||
| // To enable support of scaladoc and language-server projects you need to change this to true | ||
| bspEnabled := false, | ||
| bspEnabled := enableBspAllProjects, |
There was a problem hiding this comment.
no reason not to do this IMHO
There was a problem hiding this comment.
There is a very strong reason: you don't want your IDE to recompile every bootstrapped project from scratch every time you change one line in the compiler!
There was a problem hiding this comment.
I don't follow. If you deliberately enable this off-by-default option, why not recompile whatever's necessary in that case?
If there's an issue we should at the very least put it on the same plan as enableBspAllProjects and make it a variable that's easy to toggle without editing the build file.
There was a problem hiding this comment.
Ah never mind, I saw the removal of false and panicked. Using enableBspAllProjects seems accurate.
| */ | ||
| class CaretTest extends BaseHtmlTest: | ||
|
|
||
| private def docHtml(cls: String, syntax: String = "markdown"): String = |
There was a problem hiding this comment.
moved one level up so the logic can be shared with the new tests
| class ScriptWithSpaces | ||
|
|
||
| /** <script>alert('hello')</script> */ | ||
| class FakeSafeScript |
There was a problem hiding this comment.
the character in the comment that your browser probably doesn't render very well is the same char we internally use to mark "safe" HTML tags (why do we have such a char? because IMHO the entire scaladoc parsing stack is too hacky, and should be rewritten to be a single-pass parser... but that's for another day)
| val CleanCommentLine = | ||
| new Regex("""(?:\s*\*\s?\s?)?(.*)""") | ||
|
|
||
| /** Dangerous HTML tags that should be replaced by something safer, |
There was a problem hiding this comment.
This was a dangerous use of dangerous, pun intended. Those tags were not the kind of tags one should call dangerous.
| /** Safe HTML tags that can be kept. */ | ||
| val SafeTags = | ||
| new Regex("""((&\w+;)|(&#\d+;)|(</?(abbr|acronym|address|area|a|bdo|big|blockquote|br|button|b|caption|cite|code|col|colgroup|dd|del|dfn|em|fieldset|form|hr|img|input|ins|i|kbd|label|legend|link|map|object|optgroup|option|param|pre|q|samp|select|small|span|strong|sub|sup|table|tbody|td|textarea|tfoot|th|thead|tr|tt|var)( [^>]*)?/?>))""") | ||
|
|
There was a problem hiding this comment.
I removed "safe" tags that are related to user input (why would anyone do this?) or considered terminally deprecated (e.g., acronym)
|
@mbovel if you want to propose new behavior for Scaladoc, please go ahead (I'm not sure exactly which process that'd go through), but this PR is continuing the existing behavior while fixing some obviously broken stuff. Let's not let perfect be the enemy of good :) |
| /** | ||
| * Removes HTML tags except simple ones that can be translated or that are definitely harmless, | ||
| * translates Scala/Javadoc tags, and generally cleans the input. | ||
| * Not the fastest code in the world, and should eventually be replaced by a real parser, |
There was a problem hiding this comment.
As discussed orally, I'd be interesest what "Not the fastest code in the world" means.
There was a problem hiding this comment.
@SolalPirelli says the CI time seems similar at least.
| insideLink = false | ||
| } | ||
| index += 1 | ||
| } else if (index <= text.length - 3 && text(index) == '`' && text(index + 1) == '`' && text(index + 2) == '`') { |
There was a problem hiding this comment.
Shouldn't that kind of things be specific to wikidoc or markdown? Are backtricks and [ below interpreted the same in wikidoc and markdown?
| result.append(text.substring(index + 1, subStringEndIndex) match { | ||
| case "" => "<" // not actually a tag | ||
| case "p" | "div" => "\n\n" | ||
| case "h1" => "\n= " |
There was a problem hiding this comment.
I don't understand why we would translate these to wikidoc markup for them to be translated back to HTML after. What's the point? And how does that work with markdown? I realize this was already done like this before this PR.
|
Well, modulo all things that are weird and were already weird before, I guess it somehow LOTM (looks okayish to me) ð |
WojciechMazur
added
the
backport:nominated
WojciechMazur
added
backport:accepted
Backports #25681 to the 3.8.4-RC2. PR submitted by the release tooling. [skip ci]
Avoid basic stuff like letting `<script>` through. Obviously Scaladoc content is in the hands of the user, but we should at least not do HTML with regexes. Obligatory SO post: https://stackoverflow.com/a/1732454 ## How much have you relied on LLM-based tools in this contribution? not ## How was the solution tested? new tests [Cherry-picked 92a4162][modified]
WojciechMazur
added
backport:done
Avoid basic stuff like letting `<script>` through. Obviously Scaladoc content is in the hands of the user, but we should at least not do HTML with regexes. Obligatory SO post: https://stackoverflow.com/a/1732454 ## How much have you relied on LLM-based tools in this contribution? not ## How was the solution tested? new tests
Avoid basic stuff like letting `<script>` through. Obviously Scaladoc content is in the hands of the user, but we should at least not do HTML with regexes. Obligatory SO post: https://stackoverflow.com/a/1732454 ## How much have you relied on LLM-based tools in this contribution? not ## How was the solution tested? new tests [Cherry-picked 92a4162]
5 participants
Avoid basic stuff like letting
<script>through.Obviously Scaladoc content is in the hands of the user, but we should at least not do HTML with regexes.
Obligatory SO post: https://stackoverflow.com/a/1732454
How much have you relied on LLM-based tools in this contribution?
not
How was the solution tested?
new tests