Skip to content

Stricter strong-mode (especially around dynamic) #30397

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

Closed
2 tasks
matanlurey opened this issue Aug 10, 2017 · 8 comments
Closed
2 tasks

Stricter strong-mode (especially around dynamic) #30397

matanlurey opened this issue Aug 10, 2017 · 8 comments
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Aug 10, 2017

Forked from an internal thread.

The strong-mode: true flag to static analysis currently has an optional flag, no_implicit_dynamic, which forces inference of dynamic, to be, well, not implicit 😏. On the other hand, it and no_implicit_casts have some features we'd want to always enforce, and some we'd like to ignore, for now.

I'd like to propose a stricter_dynamic (name to-be-bike-shedded) variant that combines exactly the rules we need to be successful, without rules that don't add anything for our users. For example, no_implicit_dynamic currently flags this:

void doSomething(List<dynamic> list) {}

void main() {
  doSomething([]); // <-- Violates no_implicit_dynamic, but who cares?
}

Potential checks

WORK IN PROGRESS: Not exhaustive.

Priority 1

We'd need these checks immediately.

  • Disallow var or final without a type signature:
class A {
  final a; // <-- Error

  A(this.a);
}
void main() {
  var a; // <-- Error
}

Priority 2

We'd like to see these checks after all P1 checks land.

  • Disallow invoking anything on a dynamic object:
void doSomething(dynamic something) {
  print(something.length); // <-- Error
}

... more TBD ...


I'll be meeting with @jmesserly and others around details of what we enable/what we don't. One important note is that is OK for this new "stricter_dynamic" to not be "strictest_dynamic", it's something we can evolve over time (add more rules as they prove valuable, especially inside Google).

@natebosch
Copy link
Member

Disallow invoking anything on a dynamic object:

In that case what does dynamic do for use that Object wouldn't?

@matanlurey
Copy link
Contributor Author

matanlurey commented Aug 10, 2017

Disallow invoking anything on a dynamic object:

In that case what does dynamic do for use that Object wouldn't?

Nothing, except I can't force every library in existence to never use dynamic:

// some_library.dart
dynamic returnMapOrList() => ...
// google_code.dart
print(returnMapOrList().length); // <-- Error plz.

@jmesserly
Copy link

RE, inferring from dynamic: #26781
Might be worth taking a look at that, it has a lot of examples.

But from your examples @matanlurey, it sounds like you'd like to prohibit dynamic calls/operation altogether?

In that case, there's actually no reason to prohibit var a; ... any usage of that would be required to be safe. You'd have to: 1) use a method on Object, like a.toString(), or 2) cast it, e.g. (a as List).length. Similar with a dynamic List:

List a = [42];
a.removeFirst();
a.add('hi');
print(a[0]); // this is okay
print(a[0].contains('h')); // this is not

Prohibiting all dynamic ops is a strong property. It ensure "rename refactoring safety" for example, which is something I've heard folks request. It's a really interesting idea.

@matanlurey
Copy link
Contributor Author

matanlurey commented Aug 10, 2017

RE, inferring from dynamic: #26781
Might be worth taking a look at that, it has a lot of examples.

Thanks!

But from your examples @matanlurey, it sounds like you'd like to prohibit dynamic calls/operation altogether?

Disallow, or at least require // ignore: stong_strict_dynamic_call to compile.

In that case, there's actually no reason to prohibit var a; ... any usage of that would be required to be safe.

Sure. I'm carefully separating priorities 1 and 2 though, because I think "1" has a lot of consensus and "2" maybe does not, and I'd like to not let perfect be the enemy of good enough here :)

My goal is to see if we can have something called "stricter" strong-mode, that we enable by default in Google, and incrementally add more checks to over time (versus having to write custom lints, etc), instead of needing a breaking change to the default semantics over and over.

@jmesserly
Copy link

Disallow, or at least require // ignore: stong_strict_dynamic_call to compile.

Nice. So there's still a way to opt-in to dynamic if you really want to.

One of our inspirations for no-implicit-dynamic was C# (disclaimer: I worked on that C# feature way back when). In C# they have an explicit dynamic if you want to opt-in to dynamic dispatch. But you never "accidentally" get dynamic, you have to opt-in very explicitly. Also dynamic is only a static type, it is not reified in C# (e.g. List<dynamic> is the same as List<Object> at runtime, dynamic only affects what operations are allowed statically). So explicit dynamic+inference works out really well for them: it's convenient to write dynamic code, but you never accidentally stumble into it.

We couldn't figure out a clean way to do this in Dart, because dynamic is so often the "default" that you get implicitly. Unless we can change things like generics and untyped variables to default to Object, we'll have difficulty with an explicit-dynamic+inference model. Not to mention all the code out there from Dart 1 that may use explicit dynamic unintentionally. With your proposed flag, the dynamic type would become practically equivalent to Object, which solves the issue.

The downside is it makes dynamic a bit useless. But for things like our internal code, which we ideally want to be refactoring-safe, it might be a decent compromise.

Sure. I'm carefully separating priorities 1 and 2 though, because I think "1" has a lot of consensus and "2" maybe does not, and I'd like to not let perfect be the enemy of good enough here :)

Ah, gotcha! That makes sense. Agree, let's get in whatever easy stuff we can first. We can add to the flag over time.

@jmesserly
Copy link

CC @leafpetersen on this discussion

@Hixie
Copy link
Contributor

Hixie commented Aug 10, 2017

FWIW, Flutter (for code we ship, not for user code) does use implicit-dynamic: false in our .analysis_options, and it's worked well for us. We wouldn't want types to sometimes be allowed and sometimes not. (We also enable always_declare_return_types and always_specify_types.)

@matanlurey
Copy link
Contributor Author

I think we have enough of these issues covered elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants