last commit is red in CI with errors I'm not seeing locally. I'll look into it

UPDATE: oh, I think it's because locally the compiler is 2.13.2, whereas on CI we build & use a newer compiler (-Dstarr.version=2.13.3-bin-94412e0-SNAPSHOT) which then produces more warnings. I could bootstrap locally but that would take time; it should be sufficient to use 2.13.head which is 2.13.3-bin-45da54e (-Dstarr.version=2.13.3-bin-45da54e)

some of the warnings are about AnyRefMap and LongMap having multi-arg infix +=; I opened scala/bug#12012 on that

oh, I think it's because locally the compiler is 2.13.2, whereas on CI we build & use a newer compiler

and from here the plot thickens, as follows

going warning-free both bootstrapped and non-bootstrapped is a bit tricky, in two different situations, both of which I hit in this PR:

• if we introduce a new warning category and then need to suppress that warning in some of our own code, we cannot @nowarn(cat="the-new-category") because this will fail non-bootstrapped because the category doesn't exist yet
  • workaround: use a blanket @nowarn, with a comment that the suppression should be made more specific after next re-STARR
• if the reference compiler has a bug that causes a spurious warning, and then we fix that bug in the current compiler, then if some of our own code produces that spurious warning, we cannot @nowarn it because the bootstrapped compile will fail with "@nowarn annotation does not suppress any warnings"
  • workaround: use a blanket @nowarn, but also intentionally add something to the code that is harmless but will cause both compilers to warn, with a comment that it can be removed and the suppression made more specific after next re-STARR

maybe a more elegant solution exists for either or both of these. the workaround I found for the first problem isn't too bad, but the workaround for the second is a bit more gruesome

perhaps this is an argument for only enabling -Werror in the bootstrapped CI build and not during local development — which I think @dwijnand wanted anyway

This is really near the top of my queue again — looking forward to finishing it off.

@SethTisue are you okay with me stealing this?

are you okay with me stealing this?

absolutely

I was just discussing this with Dale recently. we were thinking:

• warnings should never be fatal by default in local development, it's too annoying. but probably provide a convenient way to flip the fatal switch on locally
• on CI, warnings should only be fatal before bootstrap, not after, because the other choice would be too annoying for contributors
  • the downside here is there might be new warnings post-bootstrap that are telling us something important and we wouldn't know about it until we re-STARR after release. we probably want to add some pre-release backstop for this to the release steps. perhaps re-STARR before release and after? maybe that's too big a hammer, maybe it would be fine to just have a release step that says, "re-STARR locally and see what turns up, but don't PR it"

wdyt?

warnings should never be fatal by default in local development, it's too annoying. but probably provide a convenient way to flip the fatal switch on locally

I've done this at work. It's fairly easy to have a fatalWarnings (or Werror) boolean settings key that adds -Werror to scalacOption. You can have it set automatically from env variables (like travis or jenkins ones).

on CI, warnings should only be fatal before bootstrap, not after, because the other choice would be too annoying for contributors

another options might be to configure warnings for @nowarn uses that suppress nothing to info-summary, so that they get logged (and we can clean them up periodically if needed), but we don't run into the issue of needing to double-re-STARR.

I've done this at work. It's fairly easy to have a fatalWarnings (or Werror) boolean settings key that adds -Werror to scalacOption. You can have it set automatically from env variables (like travis or jenkins ones).

There's isCI.value which is set in terms of env vars.

another options might be to configure warnings for @nowarn uses that suppress nothing to info-summary, so that they get logged (and we can clean them up periodically if needed), but we don't run into the issue of needing to double-re-STARR.

IMO: let's not bite off more than we can chew, let's start with just fatal warnings for non-bootstrapped and try the even stricter "and for bootstrapped" variant after a few months.

I don't understand what's the problem with bootstrap. Can someone elaborate?

As you're making changes to warnings it can be annoying for individual commits to fail to bootstrap the compiler because of a slightly wrong warning, which IMO could prove more friction than value.

As an example the changes to REPL.scala in c02a234 (#9140) which is fixed in e8734c1 (#9140).

Hum, I would typically squash such commits, in the same way that I would squash a commit that fixes a bug introduced in the same PR.

And maybe that's a fine compromise/workaround (I see it as arguable IMO, as it's useful to keep some diffs separate). I'm just saying let's take the half step first, as I've found it exhausting to work with very strict warnings-as-errors (e.g. fatal unused imports in the REPL...).

There's isCI.value which is set in terms of env vars.

I did not know that. I will change it to use that. I can't seem to find it/get sbt to load the build with that. does it need to be qualified by something?

let's not bite off more than we can chew, let's start with just fatal warnings for non-bootstrapped and try the even stricter "and for bootstrapped" variant after a few months.

I have no idea how to only enable fatal warnings for non-bootstrapped builds, so doing so is actually significantly harder for me

I did not know that. I will change it to use that. I can't seem to find it/get sbt to load the build with that. does it need to be qualified by something?

Sorry, insideCI.value.

I have no idea how to only enable fatal warnings for non-bootstrapped builds, so doing so is actually significantly harder for me

Yeah, I understand, and if it's better to land a version of this PR even without that - I don't want it to block the progress. I'm still learning myself (part of the reason I built the restarr/restarrFull commands in ScriptCommands).

I think the presence of starr.version in the sys.props is a reasonable approximation for "we're testing bootstrapping", as it's what (AFAICT!) both the Jenkins CI and Travis CI's setups use.
(Ironically it doesn't approximate correctly for my restarrFull but I can adapt that later.)

on CI, warnings should only be fatal before bootstrap, not after, because the other choice would be too annoying for contributors

after my work on cleaning up warnings, I think I actually disagree with this. it's a real pain for errors to suddenly appear much later than they were introduced, simply because they don't fail without bootstrapping

after my work on cleaning up warnings, I think I actually disagree with this. it's a real pain for errors to suddenly appear much later than they were introduced, simply because they don't fail without bootstrapping

how do you suggest we handle the difficulties I described in May?

how do you suggest we handle the difficulties I described in May?

-Wconf:msg=@nowarn annotation does not suppress any warnings:is.

I don't personally think having @nowarn annotations that do nothing for a short period of time is a huge worry, and if we only suppress them when we enable -Werror, then they'll happen locally as a reminder to eventually get rid of them

okay, we have a plan, then!

(and also, we can adjust further, later, based on experience)

@som-snytt is there a category for I can use instead of that message?

A nullifying category would be handy. I've only joked about the idiom to nowarn nowarn. (I'm afraid of notifying someone; I wonder if it's too late to register github user nowarn. Maybe Arnie already has it.)

it's a real pain for errors to suddenly appear much later than they were introduced, simply because they don't fail without bootstrapping

That experience mirrors our users: new compiler (for us: new starr), new warnings. The other side of the coin is the pain for errors to appear while you're making changes in the compiler, like an unused warning or a partial pattern match: should compilation fail (disallowing a test run) because of those, in the non-bootstrapped build?

I'm happy to test out your's and Seth's plan, however. 🎲

Oh, btw, @NthPortal, I'm hoping my exhaustivity checking PR is merged soon, with the idea that with your PRs we'll opt-out of the warnings to avoid them being fatal - so as to not burden you with dealing with them all. If you want to deal with them, go ahead, otherwise I'll eventually rotate back and do it.

it does appear there is unused-nowarn, which is perhaps cleaner than my @nowarn annotation does not suppress any warnings

I forgot that scalac -Wconf:help lists all categories.

I don't think I ever read it all the way to the end; it's sitting on my night table underneath a couple of novels.

superseded by #9235