-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should the two new "obvious"/"non-obvious"-concerned rules also target fields and top-level variables? #5101
Comments
The alternatives to not expanding the two new lint rules, are:
|
A benefit of separating out the concern of top-level variables and fields into different lint rules is a consistent set of rules that back Effective Dart. Internal Google might appreciate that, and continue to use the older omit_local_variable_types rule. |
Creating a couple of new lints would make a lot of sense! The fact that local variables and static/top-level variables have different dependency relations to other parts of the given software system already gives a strong hint that they might be treated differently by developers/organizations for good reasons. I think it should be possible to reuse hasObviousType directly. It does give an identifier that denotes a local variable a special treatment, and that case will never be reached when asking whether the declared type of a static/top-level variable is obvious. That shouldn't create any difficulties or wrong outcomes, it will only give rise to a tiny bit of wasted work (when we're testing whether an identifier denotes a local variable, and the outcome is always 'false').
I don't think it needs to be seen as a conflict. In particular, we don't have to interpret 'redundantly' strictly as "the type annotation specifies the same type as the one that inference would have provided if the type annotation had not been there". If the type annotation makes the code more readable for a human reader then I would not consider it to be redundant (and in that case I honestly don't care that those two types are identical, if the human reader needs to know the type, and it is not readily available). Another case is when subtle changes in the typing of other expressions in the same function body cause the code to have inscrutable compile-time errors, or (worse) wrong behavior at run time. A well-placed type annotation on a local variable could make the code more robust against said subtle changes: Perhaps you don't get the wrong behavior in the first place, or you get a compile-time error rather than a silent propagation of subtly different types to many locations in this function body. Such a well-placed type annotation can make the code more robust when subjected to evolution of the software that it depends on (especially if it's in a different package, maintained by a different group of people). In short, the type annotation makes certain assumptions in the code explicit because we'd like to either confirm that they still hold, or we'd like to hear about it if they don't. Again: Not redundant, because it is a non-zero loss for developers who are managing code over time, if the type annotation is removed. The whole point in having |
https://dart-review.googlesource.com/c/sdk/+/387960 implements those lints (they're not very different from the class A {
final num x = 1;
}
class B implements A {
final x = 2.5; // OK, and it has type `num`.
} How would the lint detect this? |
Yeah, tricky. If I understand the problem, we kind of need to know whether This is in particular interesting because of the proposed |
@eernstg I don't know if you have a round-up of folks who's opinions is driving your |
That's actually not necessary, we just need to know that However, it might not be that difficult after all: I do not think we have to retrace the algorithm that produced the given type annotation. That's a rather involved computation, and we don't need to compute that type. It would be sufficient to know that (1) there is no explicit type annotation, and (2) the declaration does not have a compile-time error. If we have that then I believe there is no other possibility than override inference. Also, we'd just ignore the declaration if it does have a compile-time error. |
Right, I implemented this check and added a couple of tests, and it seems to work correctly. |
Yes, indeed! |
See dart-lang/linter#5101 for some background information. Change-Id: I51b6988f08585b7b4863ae8858d44de831615177 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/387960 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Erik Ernst <[email protected]>
Effective Dart has a rule, "DO type annotate fields and top-level variables if the type isn't obvious". We have two new lint rules:
(Effective Dart also has "DON'T redundantly type annotate initialized local variables" which the new rules conflict with; noted.)
Would it be a gain for users, and a gain for maintainability and complexity, to change the two new lint rules to also concern themselves with top-level variables and with fields?
Note that the Effective Dart rule does not distinguish between public and private:
(The Effective Dart rule is sort of backed by an existing lint rule, type_annotate_public_apis, but note that that lint rule only reports on public API.)
CC @munificent @eernstg
The text was updated successfully, but these errors were encountered: