Language: Should we continue to support compile errors as warnings in NNBD weak mode?

[leafpetersen]

@jakemac53 and @vsmenon shared some concrete feedback from doing some NNBD experimentation to the effect that they found it surprising and not helpful that NNBD related compile time errors are demoted to warnings in weak mode. They pointed out that in many build systems, compiler warnings are not really surfaced unless the user goes and seeks them out - they instead just to go to a log somewhere. The user only sees a yes/no did it compile or not.

Do we still feel that there is value in allowing code with NNBD compile time errors to run in weak mode?

If we do allow it, should we consider adding a "compile time strong" mode which upgrades these to errors (which might be the default internally to Google)?

cc @lrhn @munificent @stefantsov @johnniwinther

[vsmenon]

I think this is especially true for internal tooling ( @davidmorgan ).

What is the scenario where we think it'll be helpful for users to only have these be warnings? A partially migrated library? A migrated library where deps are unmigrated?

[lrhn]

The errors we are talking about here are errors in NNBD migrated libraries, right?
If so, I am not sure precisely which advantage one would get from converting them to warnings.

It does mean that if you depend on two different packages, and they somehow migrate to NNBD in an incompatible way, you can keep running the code in weak mode anyway.
You could also just downgrade them to the pre-migration versions and keep running anyway, if running is what is important.

If upgrading the code is important, then you will probably still want the packages you depend on to be correct, and since you have NNBD errors, something is not correct.

[davidmorgan]

By default in google3 every error/warning/hint is treated the same and is build breaking.

_But_, there is a second default: developers usually run in a "warnings" mode where warnings/hints do _not_ break the build, but are displayed out of band. They will have to fix warnings/hints before submitting code, but can ignore them while working.

So the error/warning distinction is important to us. We want things to be warnings if it would be useful to keep developing even when they show up--with the understanding that it's not allowed to _commit_ code with warnings. If these are errors, they're fully blocking, i.e. the engineer can't do anything but fix them immediately.

[leafpetersen]

What is the scenario where we think it'll be helpful for users to only have these be warnings? A partially migrated library? A migrated library where deps are unmigrated?

Embarrassingly, I can't really come up with any strong argument here. Thinking back to the when we designed this feature, I think I had brought three tools to the table: suppression of errors in opted out code; errors as warnings within opted in code; and legacy types. We debated about whether to do legacy types, and I think in the absence of legacy types, I think you need the errors as warnings: otherwise you may end up being unable to call legacy code from opted in code.

I think we just decided to do all the things, and so errors as warnings came along for the ride. There was definitely a notion that it could be useful if you're in the process of migrating your code: you can run the code even when it's only half migrated. But given our experience so far, this seems really under-motivated.

[munificent]

I'd be fine with these all being errors. We put all this effort into designing legacy mode so that you can incrementally migrate on a per-library basis. If you want to run your program with only some of the nullability errors fixed, just leave the libraries with them as opted out. I think it's reasonable to expect people to treat migrating a single library as an atomic operation.

[leafpetersen]

Ok, let's just close on this. I'll change this in the spec. @stefantsov @johnniwinther we'll need to change this in the CFE. I think there's no work in the analyzer.

[stefantsov]

I created https://github.com/dart-lang/sdk/issues/40981 to track the progress for the CFE.

[leafpetersen]

This has landed in the implementation, and the spec change is out for review. Closing.