Change most specified warnings to errors

[eernstg]

Cf. dart-lang/sdk#36127 and dart-lang/sdk#36126, the language specification specifies a few compile-time warnings:

• It is a static warning if the return type of a user-declared operator []= is explicitly declared and not void. (E)
• It is a static warning if a setter declares a return type other than void. (E)
• It is a static warning if a class has a setter named v= with argument type T and a getter named v with return type S, and S may not be assigned to T. (E)
• It is a static warning if an overriding method declaration declares a different default value for a parameter. (W)
• A switch statement may yield static warnings for various non-exhaustive lists of cases. (W)
• It is a static warning to import two different libraries with the same name unless their name is the empty string. (E)

However, the treatment of these warnings has been inconsistent for a long time.

The situations marked '(E)' are currently flagged as errors by the analyzer, and '(W)' is flagged as a warning. Given that the analyzer has flagged these situations as errors for a long time, we have the option to specify them as errors without breaking existing code.

This issue serves to discuss and clarify this situation.

Here is a concrete proposal: Change the specification to specify as errors those situations that are currently flagged as errors, as indicated above.

The treatment of the getter/setter type conflict is specified for null-safety, so it will be an error soon anyway, and similarly for the switch cases.

The examples in dart-lang/sdk#36127 would be adjusted and added as language tests.

@lrhn, @munificent, @leafpetersen, WDYT?

[lrhn]

I agree with making the (E) ones errors.

I still want to make the "different default value" not a warning at all. If it's not an error, then it's not a problem. People who wants the warning can still get it from the analyzer as a lint, if they want to, but then they also have a way avoid it. They can't avoid spec-mandated warnings.

The switch warning on incomplete enums is probably fine as a warning (but I'd prefer to make it non-spec mandated and leave it up to the analyzer, and try to avoid warnings in the spec).

[munificent]

I agree with making the (E) ones errors.

Me too.

[eernstg]

@leafpetersen, @natebosch, @jakemac53, @stereotype441, do you wish to speak up against making the '(E)' cases errors in the spec?

[stereotype441]

Making the '(E)' cases errors SGTM.

[leafpetersen]

LGTM.

[eernstg]

Cf. #1083.

[natebosch]

It is a static warning to import two different libraries with the same name unless their name is the empty string.

I guess this just must not come up in practice, but I don't know if I was aware this was a warning or error. What is the reasoning for this?

[lrhn]

The original intent behind library names was probably that they were intended to be more prominent. You should refer to libraries by their names, which is also what you see in the old part of syntax. I'm sure some of the original designers would have preferred if you imported libraries by name as well, which makes sense if you have a closed-world project structure where all files are known. You can also find libraries by name in the dart:mirrors API.
In that setting, having two different libraries with the same name was really a name conflict at the library scope level.

Turned out that designing Dart for the web made imports by URIs much more practical, and ahead-of-time compiling (for the web, again) made reflection less of a feature and more of a liability.
And then, when you were importing with a URI anyway, and not using parts, most libraries don't actually need a name, so the "unless the name is empty" exception was added. (And the DartDoc tool had to figure out that top-level documentation was actually on the library, even without it being above a library declaration, but that's still better than having to write library let.me.invent.a.name;).

And then we allowed part of to refer back using a URI reference as well, so that removed the last practical non-mirrors use for library names. The only real remaining reason to have library names was to detect multiple different imports of the same library (different versions, or different URIs referring tot he same file).

When the front-end needed to be able to look at individual Dart files rather than just entire programs (because it's also a front-end for the analyzer, and you can analyze a single file), we changed the conflict rules to not disallow library name conflicts "per program", but instead "in the import graph of any file", because it couldn't know whether a file was a program or not.

We then slipped up when putting that into the spec, and didn't say explicitly that a file couldn't transitively import files with the same name, so the implementations only check that they don't directly import files with the same name. Nobody noticed. But basically, it's completely useless, and there is now no reason left for ever adding a library name (other than doing name based lookup in mirrors, which is not available on most of the platforms running Dart anyway).

So, that's the history of library names and naming conflicts (as I understand it, I wasn't actually there for the first part).

[natebosch]

The historical reasoning makes sense, thanks.

Given the current state of the ecosystem I think changing this from a warning to an error today has both very low positive impact, and very low risk. My personal choice might be to skip any of the risk and not bother with changing it since the impact is also so low but I don't feel strongly.

The rest of the (E) LGTM.

[eernstg]

@johnniwinther just found another one:

It is a compile-time error to export two different libraries with the same
name unless their name is the empty string.

Cf. #1096.

[eernstg]

PR #1096 now landed.

[eernstg]

Tests here.

[munificent]

The original intent behind library names was probably that they were intended to be more prominent. You should refer to libraries by their names, which is also what you see in the old part of syntax.

Yes. I think at the time, some even held out hope that libraries would become first class. :)

I'm sure some of the original designers would have preferred if you imported libraries by name as well, which makes sense if you have a closed-world project structure where all files are known.

Yeah, I recall some saying all imports should be prefixed.

Also, at the time I think the original designers thought libraries would be "bigger" than they turned out to be. They thought of each library as being more like, well, a library of reusable code, and not just a single file or module. Hence the name. But it turns out users don't like having a single private namespace that spans many many Dart files so libraries today tend to be the unit of maintainance more than the unit of reuse. Packages (which didn't exist at the beginning) now fill that latter role.

[eernstg]

Closing: Spec change and tests landed, implementation issues closed (dart-lang/sdk#42699), or included in NNBD epics (dart-lang/sdk#42700, dart-lang/sdk#42701, dart-lang/sdk#42702).