inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only] #5728

Philippus · 2017-02-22T20:44:39Z

as can be seen on f.e. this page:
http://www.scala-lang.org/files/archive/api/current/scala/beans/BeanInfoSkip.html
when hovering over "BeanInfoSkip" on the right the title tag contains some unwanted text.
HtmlTag(...) and the ,  should not be inside the title-tag.
This PR should fix that problem.

added missing Inline matches to inlineToStr so it is now exhaustive
scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag

SethTisue · 2017-02-22T22:40:36Z

would it be possible (and not unreasonably hard) to add test coverage...?

lrytz · 2017-02-23T09:03:12Z

I quickly looked at existing scaladoc tests, they test on the model. The PR here doesn't change the model. Maybe @VladUreche can give a hint if it's easy to test this fix?

lrytz · 2017-02-23T09:08:07Z

src/scaladoc/scala/tools/nsc/doc/html/Page.scala

+    case Monospace(in) => inlineToStr(in)
+    case Text(text) => text
+    case Summary(in) => inlineToStr(in)
+    case HtmlTag(tag) => scala.xml.XML.loadString(tag).text


The tag data could be invalid xml, no? For example:

import scala.tools.nsc.doc.model._ import scala.tools.partest.ScaladocModelTest object Test extends ScaladocModelTest { override def code = """ /** Hi there. */ class A """ def scaladocSettings = "" def testModel(root: Package) = { import access._ root._class("A").comment foreach println } }

prints

Body(List(Paragraph(Chain(List(Summary(Chain(List(Chain(List(Text(Hi), HtmlTag( ), Text(there))), Text(.)))))))))

and

scala> scala.xml.XML.loadString(" ") org.xml.sax.SAXParseException: XML document structures must start and end within the same entity.

Indeed, I will look into it a little later. I'd prefer to not use a regex, but maybe that's the way to go.

Philippus · 2017-02-23T09:08:42Z

I would like to add test cases but it will be a little annoying to do so as the method is inside a larger structure.
The initial tests for the PR where the method inlineToStr is introduced are in /scala/test/scaladoc/run/shortDescription-annotation.scalaand those tests repeat the inlineToStr-method inside the test.

lrytz · 2017-02-24T12:43:47Z

/rebuild

lrytz · 2017-02-24T21:01:00Z

/synch

SethTisue · 2017-02-24T21:03:31Z

(in my experience, retroactive application of [ci: last-only] doesn't work as a way to make the "combined" red X disappear, you just have to /nothingtoseehere it)

lrytz · 2017-02-24T21:03:32Z

ok, that "/synch" didn't help, the "combined" status is still red. anyway, it would be a good idea to squash all these commits into one and push -f.

lrytz · 2017-02-24T21:04:52Z

test/scaladoc/run/inlineToStr-strips-unwanted-text.scala

+      case Summary(in) => inlineToStr(in)
+      case HtmlTag(tag) => "<[^>]*>".r.replaceAllIn(tag, "")
+      case EntityLink(Text(text), _) => text
+    }


Copy-pasting is not ideal, obviously.. Given that this function doesn't depend on the class Page in which it is defined, how about pulling it out into the companion object? I don't see a problem with that function being public (vs. protected).

lrytz · 2017-02-24T21:08:29Z

test/scaladoc/run/inlineToStr-strips-unwanted-text.scala

+    /** This comment contains a link [[https://scala.epfl.ch/]] */
+    def baz = ???
+
+    /** This comment contains an <strong>html tag</strong> */


could alo add a test with nested html tags

Philippus · 2017-02-25T12:26:55Z

after the tests complete, I will try squashing the commits.

scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag use a regex to remove html tags inside the tag added some tests for the inlineToStr-method moved inlineToStr to companion object of Page added test for nested html tags

Philippus · 2017-03-02T09:26:50Z

@lrytz when you have time, could you take another look?

lrytz · 2017-03-02T09:42:59Z

Thanks a lot!

SethTisue · 2017-03-03T20:44:07Z

seen in the Scala community build at https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/1301/consoleFull:

[scalatest] scala.MatchError: scala.tools.nsc.doc.base.MemberLookupBase$$anon$1@1ee38638 (of class scala.tools.nsc.doc.base.MemberLookupBase$$anon$1)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.inlineToStr(Page.scala:124)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.$anonfun$inlineToStr$1(Page.scala:113)
[scalatest] 	at scala.tools.nsc.doc.html.Page$.$anonfun$inlineToStr$1$adapted(Page.scala:113)
[scalatest] 	at scala.collection.TraversableLike.$anonfun$flatMap$1(TraversableLike.scala:241)
[scalatest] 	at scala.collection.immutable.List.foreach(List.scala:390)
[scalatest] 	at scala.collection.TraversableLike.flatMap(TraversableLike.scala:241)
[scalatest] 	at scala.collection.TraversableLike.flatMap$(TraversableLike.scala:238)
[scalatest] 	at scala.collection.immutable.List.flatMap(List.scala:353)

looks like a regression as a result of this PR?

lrytz · 2017-03-03T21:22:39Z

Indeed - the pattern here is too narrow: https://github.com/scala/scala/pull/5728/files#diff-53f51aedd9537e3ba6f849e248398165R124, should probably be

    case EntityLink(title, _) => inlineToStr(title)

@Philippus do you have time to take a look?

Philippus · 2017-03-03T22:09:20Z

yes I have time. I've managed to reproduce it with
/** This comment contains a [[link ,,with a subscript title,,]] */
shall I commit a fix (and test) to this branch?

SethTisue · 2017-03-03T22:15:19Z

shall I commit a fix (and test)

yes please

to this branch?

hmm... well, we'll need a new PR. I guess you could reuse the same branch? I don't think it matters

you're in good company, the community build has been catching a lot of regressions lately.

Philippus · 2017-03-03T22:16:15Z

ok, I'll make a new PR.

only trivial merge conflict involving a method renaming * d1d700e - (origin/HEAD, origin/2.12.x) Merge pull request scala#5754 from Philippus/issue/html-tag-in-hover (2 days ago) <Lukas Rytz> |\ | * cf64718 - pattern for entitylink was too narrow, cleaned up the tests (3 days ago) <Philippus Baalman> * | f771395 - Merge pull request scala#5671 from retronym/topic/stubby-2 (3 days ago) <Lukas Rytz> |\ \ | * | ad13063 - Remove non-essential fix for stub symbol failure (4 days ago) <Jason Zaugg> | * | 7f2d1fa - Avoid forcing info transforms of primitive methods (2 weeks ago) <Jason Zaugg> | * | 37a0eb7 - Avoid stub symbol related crash in backend (2 weeks ago) <Jason Zaugg> * | | 96a7617 - Merge pull request scala#5622 from edmundnoble/extra-errs (4 days ago) <Adriaan Moors> |\ \ \ | * | | 466e52b - Match error lengths (4 weeks ago) <Edmund Noble> | * | | d1fc983 - Improved error messages for identically named, differently prefixed types (9 weeks ago) <Edmund Noble> | / / * | | f2e05c2 - Merge pull request scala#5728 from Philippus/issue/html-tag-in-hover (4 days ago) <Lukas Rytz> |\ \ \ | | |/ | |/| | * | e3c5df8 - added missing Inline matches to inlineToStr so it is now exhaustive scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag (9 days ago) <Philippus Baalman> * | | 920bc4e - Merge pull request scala#5743 from som-snytt/issue/10207-bad-update (7 days ago) <Lukas Rytz> |\ \ \ | * | | 094f7f9 - SI-10207 Error before update conversion (8 days ago) <Som Snytt> * | | | 1b4d36f - Merge pull request scala#5746 from paulp/pr/partest (7 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | eac00e1 - Add partest paths to the list of watched sources. (8 days ago) <Paul Phillips> |/ / / * | | 5f1a638 - Merge pull request scala#5732 from retronym/topic/build-info-malarkey (10 days ago) <Adriaan Moors> |\ \ \ | * | | 5e9acac - More predictable performance of SBT build startup, reload (11 days ago) <Jason Zaugg> | / / * | | 759a7b7 - Merge pull request scala#5735 from SethTisue/sd-313 (10 days ago) <Adriaan Moors> |\ \ \ | * | | ed4ddf5 - increase timeouts on some sys.process tests (11 days ago) <Seth Tisue> * | | | e2aaf2c - Merge pull request scala#5723 from dragos/issue/regression-assert-ide (11 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | e5c957e - Fix regression in 5751763 (12 days ago) <Iulian Dragos> * | | | f174bfb - Merge pull request scala#5731 from janekdb/issue/scalaGH-644/fix-spec-latex-rendering (11 days ago) <Seth Tisue> |\ \ \ \ | * | | | ba4c6d4 - scalaGH-644: Remove static html styling of spec code blocks (11 days ago) <Janek Bogucki> |/ / / / * | | | 8e40bef - Merge pull request scala#5729 from scala/revert-5658-topic/hashhash (12 days ago) <Adriaan Moors> |\ \ \ \ | |_|/ / |/| | | | * | | 86cd70f - (origin/revert-5658-topic/hashhash) Revert "Fix erasure of the qualifier of ##" (12 days ago) <Adriaan Moors> |/ / / * | | cbf7daa - Merge pull request scala#5681 from Philippus/issue/9704 (13 days ago) <Lukas Rytz> |\ \ \ | * | | b8a8ac1 - moved Pattern and TagsNotToClose to a HtmlTag companion object (13 days ago) <Philippus Baalman> | * | | a019082 - SI-9704 don't add a closed HtmlTag if it is already closed (4 weeks ago) <Philippus Baalman> | / / * | | effde0c - Merge pull request scala#5726 from scala/revert-5629-issue/10120-quote-err (13 days ago) <Adriaan Moors> |\ \ \ | * | | d60f6e3 - (origin/revert-5629-issue/10120-quote-err) Revert "SI-10133 Require escaped single quote char lit" (13 days ago) <Adriaan Moors> |/ / / * | | a8c4a54 - Merge pull request scala#5663 from gourlaysama/ticket/sd-256-enable-repl-colors-unix-2 (13 days ago) <Adriaan Moors> |\ \ \ | * | | 6411170 - SD-256 enable colored output by default on unix (13 days ago) <Antoine Gourlay> * | | | c96a977 - Merge pull request scala#5658 from retronym/topic/hashhash (13 days ago) <Lukas Rytz> |\ \ \ \ | |/ / / |/| | | | * | | f85c62e - Fix erasure of the qualifier of ## (6 weeks ago) <Jason Zaugg> | / / * | | 76bfb9e - Merge pull request scala#5708 from szeiger/issue/si10194 (13 days ago) <Lukas Rytz> |\ \ \ | * | | 1d22ee4 - SI-10194: Fix abstract type resolution for overloaded HOFs (13 days ago) <Stefan Zeiger> | / / * | | dabec1a - Merge pull request scala#5700 from retronym/ticket/10154-refactor (13 days ago) <Lukas Rytz> |\ \ \ | * | | 06eee79 - Refactor implementation of lookupCompanion (2 weeks ago) <Jason Zaugg> | / / * | | 2f1e0c2 - Merge pull request scala#5704 from som-snytt/issue/10190-elide-string (13 days ago) <Lukas Rytz> |\ \ \ | * | | 6fb3825 - SI-10190 Elide string to empty instead of null (3 weeks ago) <Som Snytt> | / / * | | 13f7b2a - Merge pull request scala#5640 from optimizely/repl-import-handler (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | aa7e335 - Fix ImportHandler's reporting of importedNames and importedSymbols (8 weeks ago) <Hao Xia> | * | | c89d821 - Fix SIOOBE in Name#pos for substrings of length 1 (8 weeks ago) <Jason Zaugg> | / / * | | 023a96a - Merge pull request scala#5629 from som-snytt/issue/10120-quote-err (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | 05cc3e2 - SI-10120 ReplReporter handles message indent (7 weeks ago) <Som Snytt> | * | | 939abf1 - SI-10120 Extra advice on unclosed char literal (8 weeks ago) <Som Snytt> | * | | 855492c - SI-10133 Require escaped single quote char lit (8 weeks ago) <Som Snytt> | / / * | | e21ab42 - Merge pull request scala#5660 from som-snytt/issue/9464-spec (2 weeks ago) <Adriaan Moors> |\ \ \ | * | | a6dcceb - SI-9464 Clarify spec on no final trait (6 weeks ago) <Som Snytt> | / / * | | e87a436 - Merge pull request scala#5659 from retronym/ticket/10026 (2 weeks ago) <Adriaan Moors> |\ \ \ | |/ / |/| | | * | 777a0e5 - SI-10026 Fix endless cycle in runtime reflection (2 weeks ago) <Jason Zaugg> | |/ * | 23e5ed9 - Merge pull request scala#5707 from retronym/topic/java9-prepare (2 weeks ago) <Lukas Rytz> |\ \ | * | 96e8e97 - Workaround bug in Scala runtime reflection on JDK 9 (3 weeks ago) <Jason Zaugg> | * | fe2d9a4 - Avoid ambiguous overload on JDK 9 (3 weeks ago) <Jason Zaugg> | * | 6bba8f7 - Adapt to change in ClassLoader in JDK 9 (3 weeks ago) <Jason Zaugg> | * | 8136057 - Bump scala-asm version (3 weeks ago) <Jason Zaugg> | / * | cad3c3d - Merge pull request scala#5709 from adriaanm/sam_wild_bound (2 weeks ago) <Lukas Rytz> |\ \ | * | c396e96 - Ignore BoundedWildcardType in erasure type map (2 weeks ago) <Adriaan Moors> * | | 3e9df41 - Merge pull request scala#5711 from retronym/ticket/jrt (2 weeks ago) <Lukas Rytz> |\ \ \ | * | | 09c7edc - Faster and simpler Java 9 classpath implementation (2 weeks ago) <Jason Zaugg> | / / * | | 7b9d3cc - Merge pull request scala#5713 from janekdb/issue/scalaGH-644/sync-jekyll-README-to-Gemfile (2 weeks ago) <Lukas Rytz> |\ \ \ | * | | 5e5ec9a - scalaGH-644: Expand note regarding Jekyll versions (2 weeks ago) <Janek Bogucki> | / / * | | 144f7e0 - Merge pull request scala#5714 from dragos/issue/usage-sterr-SI-10178 (2 weeks ago) <Lukas Rytz> |\ \ \ | |/ / |/| | | * | 640c85e - SI-10178 Route reporter.echo to stdout (2 weeks ago) <Iulian Dragos> | / * | 2fec08b - Merge pull request scala#5717 from som-snytt/issue/10148-followup (2 weeks ago) <Adriaan Moors> |\ \ | |/ |/| | * f3d271b - SI-10148 Accept verbose zero (2 weeks ago) <Som Snytt> * 147e5dd - Merge pull request scala#5716 from adriaanm/i296 (2 weeks ago) <Jason Zaugg> * 12437a0 - Ensure ordering for args to `choose` in DurationTest (2 weeks ago) <Adriaan Moors>

scala-jenkins added this to the 2.12.2 milestone Feb 22, 2017

lrytz reviewed Feb 23, 2017

View reviewed changes

adriaanm changed the title ~~inlineToStr is not exhaustive and does not remove html tags inside HtmlTag~~ inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only] Feb 23, 2017

lrytz requested changes Feb 24, 2017

View reviewed changes

lrytz reviewed Feb 24, 2017

View reviewed changes

Philippus force-pushed the issue/html-tag-in-hover branch from 438ae26 to e3c5df8 Compare February 25, 2017 13:45

lrytz approved these changes Mar 2, 2017

View reviewed changes

lrytz merged commit f2e05c2 into scala:2.12.x Mar 2, 2017

Philippus deleted the issue/html-tag-in-hover branch March 2, 2017 10:04

Philippus mentioned this pull request Mar 3, 2017

fix regression in scaladoc #5754

Merged

lrytz mentioned this pull request Mar 7, 2017

[ci: last-only] merge 2.12.x onto 2.13.x (March 6, 2017) #5759

Merged

inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only] #5728

inlineToStr is not exhaustive and does not remove html tags inside HtmlTag [ci: last-only] #5728

Uh oh!

Conversation

Philippus commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SethTisue commented Feb 22, 2017

Uh oh!

lrytz commented Feb 23, 2017

Uh oh!

lrytz Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

Philippus Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

Philippus commented Feb 23, 2017

Uh oh!

lrytz commented Feb 24, 2017

Uh oh!

lrytz commented Feb 24, 2017

Uh oh!

SethTisue commented Feb 24, 2017

Uh oh!

lrytz commented Feb 24, 2017

Uh oh!

lrytz Feb 24, 2017

Choose a reason for hiding this comment

Uh oh!

Philippus Feb 25, 2017

Choose a reason for hiding this comment

Uh oh!

lrytz Feb 24, 2017

Choose a reason for hiding this comment

Uh oh!

Philippus Feb 25, 2017

Choose a reason for hiding this comment

Uh oh!

Philippus commented Feb 25, 2017

Uh oh!

Philippus commented Mar 2, 2017

Uh oh!

lrytz commented Mar 2, 2017

Uh oh!

SethTisue commented Mar 3, 2017

Uh oh!

lrytz commented Mar 3, 2017

Uh oh!

Philippus commented Mar 3, 2017

Uh oh!

SethTisue commented Mar 3, 2017

Uh oh!

Philippus commented Mar 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Philippus commented Feb 22, 2017 •

edited

Loading