Enable -Werror for more modules #9304

NthPortal · 2020-11-06T07:40:06Z

No description provided.

NthPortal · 2020-11-06T07:40:26Z

I may add commits for more modules if I have the capacity - we'll see

som-snytt · 2020-11-06T07:56:30Z

src/scaladoc/scala/tools/nsc/doc/Settings.scala

@@ -349,6 +350,7 @@ class Settings(error: String => Unit, val printMsg: String => Unit = println(_),
    /** Dirty, dirty, dirty hack: the value params conversions can all kick in -- and they are disambiguated by priority
     *  but showing priority in scaladoc would make no sense -- so we have to manually remove the conversions that we
     *  know will never get a chance to kick in. Anyway, DIRTY DIRTY DIRTY! */
+    @nowarn("cat=lint-nonlocal-return")


Could also use @nowarn("cat=dirty-dirty-dirty")

SethTisue · 2020-11-22T18:46:16Z

@NthPortal maybe rebase and finish off what you have here, and leave possible future work on other modules for a separate PR?

SethTisue · 2020-11-22T20:26:56Z

and, note that if you worked on a module and got rid of some warnings but didn't get it to where -Werror can be enabled, that work is still mergeable

NthPortal · 2020-11-22T20:33:29Z

@SethTisue all the warnings were gone when I wrote the commits; it seems like other warnings crept in since then. I'll... see if i can clean it up again

SethTisue · 2020-11-22T20:39:36Z

yeah this is an area where the forces of entropy work swiftly

now that 2.13.4 is out and is apparently not catastrophically flawed, the team has more bandwidth again to help you make sure this lands. (if you find yourself without time to work on it, let us know and one of us can finish it.)

Tweak -Wconf options to conform to the help text. Use `@unchecked` instead of `@nowarn` for exhaustiveness warnings.

Enable linting for scalap build and fix all deprecations in the module. Enable -Werror for scalap build.

Enable linting for scaladoc build and fix or suppress all deprecation, linting and feature warnings in the module. Enable -Werror for scaladoc build.

dwijnand · 2020-11-23T10:02:37Z

src/scaladoc/scala/tools/nsc/doc/base/CommentFactoryBase.scala


  /** The end of a Scaladoc code block */
  private val CodeBlockEndRegex =
-    new Regex("""(.*?)((?:\}\}\})|(?:\u000E</pre>\u000E))(.*)""")
+    new Regex("""(.*?)((?:\}\}\})|(?:</pre>))(.*)""")


What's this change about? (Have I asked this already?)

I replaced it with the literal \u000E character, which looks like � (rendering varies). for some reason, github isn't rendering it in diff mode, but if you look at the full file without diff, it's there:

scala/src/scaladoc/scala/tools/nsc/doc/base/CommentFactoryBase.scala

Lines 186 to 190 in 8447c42

new Regex("""(.*?)((?:\{\{\{)|(?:�<pre(?: [^>]*)?>�))(.*)""")

/** The end of a Scaladoc code block */

private val CodeBlockEndRegex =

new Regex("""(.*?)((?:\}\}\})|(?:�</pre>�))(.*)""")

I see, thanks!

Is this because we don't know how "\u" is processed any more? There's also "\x0e".

raw"\x09".r.findAllMatchIn("aaa\tbbb\tccc").map(_.start).toList

I fear that GitHub isn't the only tooling that isn't going to handle an embedded invisible control character well. I think it would really be better to use an escape. No opinion on \x vs \u, but note that since #8282 , \u is no longer weird/special

the reason I replaced it was because of a warning that said to use a literal; I have no particular opinion on it otherwise

I tried it and it is awkward. It's stuck half-way, like Winnie-the-Pooh. I'd recommend \x. I'm going to try \X; there's that other PR about mkstring with unicode points.

scala> "a\u0009b" val res0: String = a b scala> raw"a\u0009b" ^ warning: Unicode escapes in raw interpolations are deprecated. Use literal characters instead. val res1: String = a b

what on earth is this random control character doing in the regexes in the first place? surely Scaladoc doesn't actually use control-N for anything.... one hopes? is it possible somebody meant control-M, which is carriage return? control-N is "SO (SHIFT OUT)", does anybody under the age of 80 even know what that is?

let's see if all the tests still pass if we just take it out: #9359

dwijnand

Basically good to go, IMO. Just one question above a regex change(?)

SethTisue · 2020-11-23T18:25:08Z

I propose a special 🎖️ for "dedication to clean code,"

indeed, 🎖️ to @NthPortal for taking this on, this kind of work has a lasting positive impact on the health of the codebase

surely they can't actually mean anything or be doing anything? (surely?) this came up on scala#9304

I have no idea why we need these control-N characters in these regexes, but it's better not to embed them literally, and Unicode escapes inside triple quotes are deprecated this came up on scala#9304

this came up on scala#9304

NthPortal added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Nov 6, 2020

scala-jenkins added this to the 2.13.5 milestone Nov 6, 2020

NthPortal mentioned this pull request Nov 6, 2020

compile stdlib & compiler with fatal-warnings / deprecations scala/scala-dev#600

Closed

som-snytt reviewed Nov 6, 2020

View reviewed changes

NthPortal force-pushed the topic/werror-4/PR branch from ec891dd to 42f45ee Compare November 23, 2020 01:49

NthPortal added 3 commits November 22, 2020 21:10

Assorted warning conf and annotation cleanup

bfb9ab5

Tweak -Wconf options to conform to the help text. Use `@unchecked` instead of `@nowarn` for exhaustiveness warnings.

Enable linting and -Werror for scalap build

af85ef4

Enable linting for scalap build and fix all deprecations in the module. Enable -Werror for scalap build.

Enable linting and -Werror for scaladoc build

8447c42

Enable linting for scaladoc build and fix or suppress all deprecation, linting and feature warnings in the module. Enable -Werror for scaladoc build.

NthPortal force-pushed the topic/werror-4/PR branch from d4b727f to 8447c42 Compare November 23, 2020 02:57

dwijnand reviewed Nov 23, 2020

View reviewed changes

dwijnand approved these changes Nov 23, 2020

View reviewed changes

dwijnand merged commit 867f63a into scala:2.13.x Nov 23, 2020

NthPortal deleted the topic/werror-4/PR branch November 23, 2020 18:07

SethTisue added a commit to SethTisue/scala that referenced this pull request Dec 4, 2020

remove stray control-N characters

393afce

surely they can't actually mean anything or be doing anything? (surely?) this came up on scala#9304

SethTisue mentioned this pull request Dec 4, 2020

avoid including control characters directly in source code #9359

Merged

SethTisue added a commit to SethTisue/scala that referenced this pull request Dec 4, 2020

don't have literal control characters in source

71b3a1b

this came up on scala#9304

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable -Werror for more modules #9304

Enable -Werror for more modules #9304

NthPortal commented Nov 6, 2020

NthPortal commented Nov 6, 2020

som-snytt Nov 6, 2020

SethTisue commented Nov 22, 2020

SethTisue commented Nov 22, 2020

NthPortal commented Nov 22, 2020

SethTisue commented Nov 22, 2020 •

edited

dwijnand Nov 23, 2020

NthPortal Nov 23, 2020 •

edited

dwijnand Nov 23, 2020

som-snytt Nov 23, 2020

SethTisue Nov 23, 2020

NthPortal Nov 23, 2020

som-snytt Nov 23, 2020

SethTisue Dec 4, 2020 •

edited

SethTisue Dec 4, 2020

dwijnand left a comment

SethTisue commented Nov 23, 2020

	new Regex("""(.?)((?:\{\{\{)\|(?:�<pre(?: [^>])?>�))(.*)""")

	/** The end of a Scaladoc code block */
	private val CodeBlockEndRegex =
	new Regex("""(.?)((?:\}\}\})\|(?:�</pre>�))(.)""")

Enable -Werror for more modules #9304

Enable -Werror for more modules #9304

Conversation

NthPortal commented Nov 6, 2020

NthPortal commented Nov 6, 2020

Choose a reason for hiding this comment

SethTisue commented Nov 22, 2020

SethTisue commented Nov 22, 2020

NthPortal commented Nov 22, 2020

SethTisue commented Nov 22, 2020 • edited

Choose a reason for hiding this comment

NthPortal Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SethTisue Dec 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand left a comment

Choose a reason for hiding this comment

SethTisue commented Nov 23, 2020

SethTisue commented Nov 22, 2020 •

edited

NthPortal Nov 23, 2020 •

edited

SethTisue Dec 4, 2020 •

edited