Open srawlins opened 1 year ago
cc @bwilkerson @mit-mit @jacob314 @pq @lrhn @leafpetersen
There are others like ParserErrorCode ...
Kind of a minor nit, but I consider ParserErrorCode
s and FfiCode
s to be errors, just like CompileTimeErrorCode
. Both are user-visible and they have the same impact in terms of our tooling.
We didn't always choose the classes based on severity, sometimes it had more to do with where the diagnostics are being generated or who owns the codes. That said, what's important here is not which classes we used in the implementation, but the impact of changing the severities of some of our enabled-by-default diagnostics.
I would consider two things: enablement-by-default, and number of managed diagnostics
. I am under the impression that generally hints
are non-disabled, while lints
are configurable
.
A compile-time error (and similar) is an error, and the user would like to see this as early as possible to be fixed. There is no value to disable it, I guess, since the error will surely come later, either during compilation or at run-time.
Moving the current hints (around 127) to lints will make the user be overwhelmed by the number of diagnostics which are configurable
. Honestly, I think this might not be adding much value, but actually preventing more productivity. Dart is taking the decision to allow combinations and configuration of very tiny details, but does the user really care?
One of the decisions the formatter took, is to almost disable configuration, so whatever the formatter team decides, everyone follows. The only option is to turn on/off the formatter, life is easy and simple.
Let's compare dead code
(currently hint), why should I user configure that? A dead code
will never be executed, it is better to show it while coding. Comparing this to unneeded/unnecessary
(currently lint), the user may want to use the parentheses
or new
, but it is optional, and that's user/development team preferences.
Personally, I believe "less is more", and as the language evolves, I guess there is no need to let the users still use the old ways for long time (using new
keyword, or not migrating to super parameters
). Yes, this is possibly a sensitive area, but I don't think in 10 years the Dart team would still need to carry potentially old deprecated language features, just not to disturb users. Things are made to be changed, and a major change every one/two years wouldn't be that bad. Also, the severity can change, for example, using new
is an info for one year, then a warning
afterwards. Even if the user uses an old-written package, the diagnostics are shown only for the current package, not for the dependencies.
From the high-level, the user chould have three sets of configurable diagnostics (restrictive/all, medium, low/none). The total number of the configurable diagnostics should not be high (I would say somewhere around 100, if not less). Anything which isn't really needed to be configured, should be non-configurable by default. Configuring every diagnostic is ok, but seeing hundreds of non-categorized diagnostics is not very user-friendly.
@asashour I think we agree.
Moving the current hints (around 127) to lints will make the user be overwhelmed by the number of diagnostics which are
configurable
.
I agree. Note that I wrote:
They will mostly be moved to "warnings."
@bwilkerson wrote up a comprehensive list, and I think it is an extreme minority of "hints" which we may rewrite as "lints." Less than 10. Examples are DIVISION_OPTIMIZATION
and PACKAGE_IMPORT_CONTAINS_DOT_DOT
. They're largely obscure (or are to me).
@bwilkerson wrote up a comprehensive list
Can we put that list somewhere (e.g. in a markdown gist) and link it from this issue? That would make it easier to understand the implications of this proposed change.
@mit-mit I believe @bwilkerson shared these with you offline. We could move it into a public gist. The internal doc just has lots of discussion.
Progress: In early December there were 140 HintCodes. Today there are 109 HintCodes, and 35 WarningCodes.
Progress: Today there are 44 HintCodes, and 105 WarningCodes.
Progress: Today there are 25 HintCodes, and 122 WarningCodes.
Progress: Today there are 19 HintCodes, and 135 WarningCodes. This is probably what is shipping in Dart 3.0.
And there are now 165 WarningCodes and 9 HintCodes. 4 are versions of "DEPRECATED_MEMBER_USE"; 2 are deprecated; the remaining are IMPORT_DEFERRED_LIBRARY_WITH_LOAD_FUNCTION
, MACRO_INFO
, and UNNECESSARY_IMPORT
.
@jakemac53 @scheglov
I'd be interested to get your input on MACRO_INFO
. Do we want to treat this as a lint (so that it retains the INFO
severity as the name applies)? Do we want to drop it (and encourage macro authors to write a plugin to produce lints)? Or some other option?
Do we want to treat this as a lint (so that it retains the
INFO
severity as the name applies)?
Yes
Do we want to drop it (and encourage macro authors to write a plugin to produce lints)? Or some other option?
I don't want to force macro authors into writing plugins any more than is necessary. It complicates things unnecessarily imo, both from a user experience perspective and a macro author perspective.
Currently in the analyzer we have (approximately) the following diagnostic categories:
(There are others like ParserErrorCode, TodoCode, FfiCode, etc, but the 4 above are the common, user-visible ones.)
We'd like to remove the HintCode category, and move all "hints" to be either "warnings" or "lints." They will mostly be moved to "warnings."
The categories differ from each other in their default severity (one of
ERROR
,WARNING
, orINFO
), and whether they are reported by default (or must be enabled viaanalysis_options.yaml
):ERROR
WARNING
INFO
INFO
What do these mean, practically? The difference is primarily in the behavior of
dart analyze
(used in presubmit checks, continuous integration, and perhaps by humans):dart analyze
, its exit code is0
.INFO
-severity diagnostic report does not move the exit code away from0
.WARNING
-severity diagnostic report does move the exit code away from0
.ERROR
-severity diagnostic report does move the exit code away from0
.Note also:
WARNING
-severity diagnostics can be changed with the--[no-]fatal-warnings
flag.INFO
-severity diagnostics can be changed with the--[no-]fatal-infos
flag.DEAD_CODE
orUNUSED_IMPORT
or any lint rule) can be changed inanalysis_options.yaml
from its default to any of the others, orIGNORE
.This proposal is therefore a breaking change: "Hints" which are moved to be "warnings" will become fatal (by default) when reported by
dart analyze
. (Behavior will not change when--fatal-infos
or--no-fatal-warnings
is being used.)Another significant change: users will also no longer get the protection from a "hint" which is moved to be an opt-in "lint" unless they explicitly enable it in their
analysis_options.yaml
file (or include another analysis options file which does so).Historical note: We had more "static warnings" defined by the language in previous releases of Dart, but all but a handful of those have been dropped, and the language team has confirmed that it is "fair game" for tools like the analyzer to define their own "warnings."