dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.28k stars 1.59k forks source link

Expand @optionalTypeArgs to handle nested type arguments #37568

Open srawlins opened 5 years ago

srawlins commented 5 years ago

The optionalTypeArgs annotation has this doc comment:

Tools such as the analyzer and linter can use this information to suppress warnings that would otherwise require type arguments to be provided for instances of C.

("for instances of C" is not consistent with how analyzer and linter currently use this annotation; for example they may warn that class MyC extends C is missing type argument(s). I'd like to ignore this inconsistency in this issue.)

I'd like to expand the interpretation of this comment to include nested type arguments. For example:

class FooComponent<T> {}

@Component(
    directiveTypes: [Typed<FooComponent>.of(#E)])
class Bar<E> {}

In this Angular example, the Angular compiler will properly type and instantiate a FooComponent<E>, but analysis rules like strict-raw-types will complain that FooComponent should have a type argument. Adding a type argument would be non-functional, and would be very confusing. Attempting to suppress the warning by annotating Typed with @optionalTypeArgs does not currently work, because warnings on "nested" type arguments are not suppressed. It would be inappropriate to annotate FooComponent with @optionalTypeArgs because in all other circumstances, it should have type arguments.

srawlins commented 5 years ago

CC @Hixie @pq @bwilkerson who I think suggested, implemented, and reviewed @optionalTypeArgs in https://github.com/dart-lang/sdk/issues/57293 for any +1s or -1s.

bwilkerson commented 5 years ago

Wouldn't it be just as inappropriate to put @optionalTypeArgs on Typed? That would mean that we wouldn't warning about code like

@Component(
    directiveTypes: [Typed.of(#E)])
class Bar<E> {}

which I assume is invalid.

I think you want something like @OptionalTypeArgsForTypeArgs that would specifically say that, while this type should always have type args, the type args do not themselves need to have type args. But that seems a little too special cased to me.

srawlins commented 5 years ago

That example would be, as you say, invalid. The issue can be caught at runtime.

Any class tagged with optionalTypeArgs has type args that provide some value, such that omitting them could certainly change behavior. But authors that choose to annotate are saying "yeah its <dynamic> so often, or hard to determine statically so often, that it's optional."

I think the name optionalTypeArgs rather than needlessTypeArgs or uselessTypeArgs means that "often" type arguments are not needed, to the point that adding <dynamic> hundreds of times in code, which does not improve understandability, makes the code worse.

bwilkerson commented 5 years ago

... its so often, or hard to determine statically so often, that it's optional.

Right, but is that true of Typed, or only of the nested type args? My guess is that it isn't true, hence my comment that it's equally inappropriate.