Skip to content

strong_mode_implicit_dynamic_method ignores @optionalTypeArgs #32538

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
Hixie opened this issue Mar 14, 2018 · 11 comments
Closed

strong_mode_implicit_dynamic_method ignores @optionalTypeArgs #32538

Hixie opened this issue Mar 14, 2018 · 11 comments
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful type-enhancement A request for a change that isn't a bug

Comments

@Hixie
Copy link
Contributor

Hixie commented Mar 14, 2018

import 'package:meta/meta.dart';

class A {
  @optionalTypeArgs
  void test<T>() { }
}

void main() {
  new A().test();
}

->

  error • Missing type arguments for generic method 'test<T>' at test.dart:10:11 • strong_mode_implicit_dynamic_method
@bwilkerson
Copy link
Member

The error appears to be generated in ErrorVerifier._checkForImplicitDynamicIdentifier, but as far as I can tell, none of the other strong mode checks pay any attention to this annotation, either.

(And the annotation is mis-documented as only being applicable to class declarations, when it really ought to be applicable to any declaration that can have type parameters.)

@bwilkerson bwilkerson added P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug labels Mar 14, 2018
@Hixie
Copy link
Contributor Author

Hixie commented Mar 15, 2018

If there's any chance this could be fixed sooner rather than later that would be awesome. I have a PR where I add some type arguments to some commonly used methods and I am really trying to get the inference to help me at analysis time rather than requiring people to type in the types... right now I have 158 instances of this error in the Flutter repo all of which I'd like to silence by specifying @optionalTypeArgs.

@leafpetersen
Copy link
Member

cc @jmesserly

A workaround is to declare the generic method parameter with a bound of Object (assuming that you are ok with getting Object and not dynamic as the default instantiation).

@Hixie
Copy link
Contributor Author

Hixie commented Mar 15, 2018

That workaround is sufficient, thanks.

@jmesserly
Copy link

@optionalTypeArgs seems to have been added for the linter rule always_specify_types: https://github.com/dart-lang/linter/blob/cd13719c0b2e71c66f12073fecf03951d0ccafcb/lib/src/rules/always_specify_types.dart#L50

As @leafpetersen mentioned, an optional type arg can be declared with no-implicit-dynamic, but it requires using extends Object (or any other type that isn't dynamic):

class A {
  void test<T extends Object>() { }
}
void main() {
  new A().test();
}

The nice thing about that is it prevents dynamic from being implicitly used. Helps for examples like:

class A {
  List<T> test<T>() => <T>[];
}
void main() {
  var list = new A().test(); // implicit List<dynamic>
  list.add('a string!');
  list.first.methodThatDoesNotExist();
}

That produces a runtime error. If changed to T extends Object it produces a compile time error. No-implicit-dynamic is attempting to help catch things like that.

Also FWIW, request for optionalTypeArgs to be recognized was also reported in: #26901

@jmesserly
Copy link

aside: it might be nice if we had an option for instantiate-to-bounds to choose Object over dynamic, without needing to add an extends clause to the declaration. That way implicit instantiation is made safer, without restricting it completely (as no-implicit-dynamic currently does).

@matanlurey
Copy link
Contributor

@jmesserly:

aside: it might be nice if we had an option for instantiate-to-bounds to choose Object over dynamic, without needing to add an extends clause to the declaration. That way implicit instantiation is made safer, without restricting it completely (as no-implicit-dynamic currently does).

YES THIS PLEASE.

@leafpetersen
Copy link
Member

Yeah, I've wondered whether --no-implicit-dynamic shouldn't just do that.

I tested out using Object globally instead of dynamic, and unfortunately it's fairly breaking.

@lrhn
Copy link
Member

lrhn commented Mar 15, 2018

I don't think a linter flag should change program semantics. I don't want multiple incompatible semantics to be implemented and used by different users.
It's fine to reject a program if it doesn't satisfy the stronger linter requirements, that can only hurt whoever explicitly asked for that lint. To actually change program behavior, I'd prefer if a tool actually changed the source code.

If we change the implicit bound to Object, then some code would break.
That code has a way out, it can just write <dynamic> explicitly since A<dynamic> is still valid, if nothing else then as a super-bounded type. I'll take Leaf's word that the breakage is significant, so we probably can't do that.

@jmesserly
Copy link

If we change the implicit bound to Object, then some code would break. That code has a way out, it can just write explicitly since A is still valid, if nothing else then as a super-bounded type. I'll take Leaf's word that the breakage is significant, so we probably can't do that.

Yeah, it would be way too much to all fix at once. If we did want to investigate a change to the default bound, a flag could come in handy. It gives us a way to see if we can move some pieces of code to the new rules, and whether we like the result. If it seems like a good change, then it can be rolled out incrementally across the codebase by opting in, then eventually we can eliminate the flag. (Strong-mode flag was prototyped, and eventually rolled out using a similar approach.)

@srawlins
Copy link
Member

strict-raw-types (replacement for implicit-dynamic: false), takes @optionalTypeArgs into account.

@eernstg eernstg added the strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-warning Issues with the analyzer's Warning codes legacy-area-analyzer Use area-devexp instead. P2 A bug or feature request we're likely to work on strict-analysis-needed Mark issues where strict-raw-types, strict-inference and similar analysis options were helpful type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants