-
Notifications
You must be signed in to change notification settings - Fork 30
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
Consider avoid_dynamic_calls
as a lint rule.
#44
Comments
Not approved: dynamic is fully supported in the language, and something a developer is free to use if they desire. |
@mit-mit How does a lint prevent a developer from using dynamic calls if they desire? 🤔 My understanding is that the lint would be used (optionally) by developers who do not desire to have dynamic calls in their codebase, or at least, in rare situations where they do need one, they want it flagged and worked around using an I was looking forward to this lint to use in the Flutter Web engine. We don't need dynamic invocations in the engine, but our codebase is big, so we don't know if we have some dynamic calls that sneaked in by accident, potentially hurting performance and/or code size. Also, my understanding is that the lint doesn't lint against dynamic json = parseJson(jsonString);
if (json is Map) {
dynamic someField = json['some_field'];
if (someField is num) {
print(someField.toStringAsFixed(2));
} else if (someField is String) {
print(someField);
} else {
// ... etc ...
} What would be flagged is this: dynamic json = parseJson(jsonString);
json['some_field'].toStringAsFixed(3);
// ^ ^
// | | dynamic invocation of toStringAsFixed
// dynamic invocation of operator[] Those invocations can crash, and they can cause retention of methods on unrelated classes. In codebases, such as the Flutter Engine, flagging such situations would be super useful. (Edit: minor fixes; clarification; examples) |
This issue doesn't track the availability of this lint; it tracks whether the lint is enable by default in |
Oh, but maybe the confusion is that @srawlins logged this as a request to have this as a supported lint? Then we can certainly reactivate and transfer to https://github.com/dart-lang/linter/issues... |
The rule exists. This was a request to add the rule to the lints:core or lints:recommended rule set. |
Ah, cool. If this can be optionally added to analysis options, I have no further concerns. |
I hear there were some recent discussion around /cc @munificent @eernstg you were part of these discussions. Curious to hear what you think. |
I guess it is possible to disable a recommended lint in a package and certainly using |
@mit-mit Can you reopen this issue for reconsideration during our next lint triage? |
It came up in a recent discussion that a surprisingly high number of the top 200 runtime errors reported by our flutter analytics are related to dynamic. There is some hope that enforcing a lint like this (and/or #125 could help in reducing that number). |
From a discussion, I believe another way that was proposed to disable this lint was via an explicit cast; so: (value as dynamic).fooBar(); |
Together with If we're not willing to actually remove dynamic from the language, I don't think we should enable these restrictions by default, which is what adding to |
A big "if"! Surely not a settled decision, but an open possibility. |
To me, the lints we recommend are about best practices - and from our crash reporting we can see that a lot of people are struggling with |
When I've written an app that's talking to a JSON API I don't know well, sometimes I just guess at the API by casting stuff and then let the crashes incrementally teach me the API's schema. I wonder if some fraction of these crashes are coming from users that are just doing "let me just try it and see what happens" and don't indicate a real source of pain in the language. |
Are we able to pivot the crash reporting on release mode (debug vs release)? I'd suspect such "let me just try it" development would be all debug mode? |
The fundamental question here is: Why does Dart support the type One answer could be: Because it is an irreplaceable extension of the expressive power of the language in situations where very generic code is assumed type safe based on an ad-hoc proof which is beyond the capabilities of the built-in type system. In other words, it's a specialist's tool, and it is no problem at all if dynamically typed code requires some extra syntax. (Like an explicit A different answer could be: Because dynamically typed code is easier to create for inexperienced developers. Static typing is an option that developers may adopt over time when they are ready to make the extra investment into the correctness of their code. If this is the reason for having I'd very much prefer to consider dynamically typed coding as a specialist's tool, that is: I prefer the first answer. I think the second answer is much less convincing today than it might have been years ago. In particular, Dart supports a very substantial level of static analysis (type inference, variable promotion, flow analysis) that enables statically typed code with few explicit type annotations. Admittedly, this kind of coding can appear to be more complex than dynamically typed coding, but that's because the statically typed code, even with type inference etc, does more than the dynamically typed code: It includes a proof of certain kinds of correctness. I believe the Dart community as a whole is willing to do the extra work here, in return for less buggy programs. Based on this perspective on dynamically typed coding, I'd have no problems recommending that we include |
If we actually want to take I'd just remove the type, and add an "operator"/special-syntax of It might be hard on JSON access, but I hope pattern matching will help with that. The global migration would add |
I strongly agree with everything @eernstg said. With Dart 2.0, we pivoted Dart away from the "gradual typing" user experience where a use uses dynamic because they just don't want to deal with types (or don't want to deal with them yet). Once we made var a = [];
var b = [1, 2, 3];
b.addAll(a); I'm happy to give up this use case (though I believe it does have value!) because losing that means much better user experience for users who do want the full benefit of types. I think the trade-off is a very clear net win. But it does mean that, like Erik suggests, |
I think we would be considering adding something that allows dynamically invoking a method without knowing the type declaring it. The implicit downcast, probably not. I do think that using |
I am completely in favor of considering Even if we don't remove |
What about an
If we come up with what we think are the total set of best practices around I do think that what a lot of our customers want is effectively Dart without We can align the analysis towards removing |
This one is very hard to comply with today, without making a lot of exceptions. There are gobs and gobs of generic types (with
These examples can be alleviated, but it requires some work:
I would 100% support a lint rule that required you to change bounds from
Available to day as
Available today as
We could release an analysis options file that enables different bits of config. |
I'm starting to come around to re-considering if https://dart.dev/tools/linter-rules/avoid_dynamic_calls should indeed be added to the recommended lint set. Do we think this would trigger a bunch of prevalent code being written today? |
There aren't that many In Dart 2.12, the default bound of a generic type variable with no specified bound was changed to In hindsight, that phrasing was problematic, because there was no concept of "default bound" before, so we failed to enumerate the precise places where one should now use I'd love to change instantiate-to-bounds to not insert spurious TL;DR: The issue with About:
The problem with taking that literally is that the result will have type var name = (((json as dynamic)["context"] as dynamic)["project"] as dynamic)["name"] as String; Sure, you can use If you can, then (Example valid only until someone creates an extension type wrapper for JSON. I know I will. 😉) |
Yes, this is what I meant, sorry for the imprecision. Instead of, "There are gobs and gobs of generic types (with Instead of "Alternatively, change the default bound to
Nope, that's the goal :D
Do you mean something like these? https://github.com/flutter/devtools/pulls?q=is%3Apr+extension+is%3Aclosed+author%3Asrawlins Or more generic? |
Would it help if the language allowed a type parameter bound to be |
@munificent wrote:
Interesting! That's actually a kind of "don't use this member when the receiver type is raw" mechanism: typedef Void = void;
class C<X extends Void> {
final X x;
C(this.x);
void foo() {}
}
void main() {
C c = C(1);
c.foo(); // No problem.
print(c.x); // Error!
} |
Yeah, it's basically allowing bounds that let you pick which top type you want if the type parameter is unconstrained. We could also treat the default instantiate-to-bound as |
FWIW I would personally be fine with this verbosity, but I agree that many users would find this annoying. There are also alternative ways to write this today, both approaches that give more opportunity to custom handle inputs with unexpected shape: var name = switch(json) {
{'context': {'project': {'name':String name}}} => name,
_ => throw FormatException('Expected a Map with <format>'),
}; Or a terse approach: var {'context': {'project': {'name': String name}}} = json; |
Also folks could just disable the lint for a particular file. Outside of toy CLI scripts, everyone uses some sort of JSON code generation solution at this point, or a wrapper/helper. |
I think
avoid_dynamic_calls
would be a great addition to the lint rule sets. Either in core or in recommended.https://dart-lang.github.io/linter/lints/avoid_dynamic_calls.html
It is currently marked
experimental
. See dart-lang/sdk#58409The text was updated successfully, but these errors were encountered: