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.27k stars 1.58k forks source link

always_specify_types should respect @optionalTypeArgs w.r.t static extensions #58007

Open srawlins opened 5 years ago

srawlins commented 5 years ago

always_specify types allows unspecified types when a class is annotated with @optionalTypeArgs. This should be updated for extension methods. Some of this work might just be adding tests for verification.

(This has been spun off of https://github.com/dart-lang/sdk/issues/38105)

eernstg commented 5 years ago

The part about Baz("ok") is a little bit confusing. If we have

class C<T> {...}
extension Baz<T> on C<T> {
  void baz() {}
}

then Baz(C<int>()).baz() would be an example of an explicit extension method invocation where some type arguments could be given, but were omitted. So that's a situation where always_specify_types could flag the missing type arguments (to Baz), and it could then silently accept the same thing when Baz has an @optionalTypeArgs in its metadata.

But isn't that an overly strict thing to do? I'd expect extension method invocations to be implicit almost all the time, and I would certainly expect a lint that forces developers to change myC.baz() to Baz<int>(myC).baz() to be too strict in practice, because extension method invocations should be implicit whenever possible.

Another point in the design space would be to accept myC.baz() and Baz<int>(myC).baz(), but reject Baz(myC).baz(), but it seems odd to be really strict with the explicit invocation if there's no lint on myC.baz() (where exactly the same type arguments are omitted and inferred).

srawlins commented 5 years ago

This is an extremely strict lint rule. See the tests: https://github.com/dart-lang/linter/blob/master/test/rules/always_specify_types.dart

Map<String, String> map = {}; //LINT

But I have no skin in this game.

AlexV525 commented 2 years ago

I'm facing the false positive issue with @optionalTypeArgs recently with the CodeFactor, and they've suggested this issue.

The problem is https://www.codefactor.io/repository/github/fluttercandies/flutter_wechat_assets_picker/issues, where the ExamplePageMixin is annotated with @optionalTypeArgs.

Source code: https://github.com/fluttercandies/flutter_wechat_assets_picker/blob/0629b80aac30e3386889a65aaee2d07b8acf3213/example/lib/constants/page_mixin.dart#L16

pq commented 2 years ago

Thanks @AlexV525. I'm trying to come up with a targeted repro.

I'd expect this to fail based on what you're seeing but it doesn't.

class C<T> { }
mixin M0 { }

@optionalTypeArgs
mixin M<T> on C<T> { }
class MixedIn extends C<String> with M0, M { } // OK

On a quick glance, do you see what I might be missing?

AlexV525 commented 2 years ago

Sorry I missed some info. The annotation didn't fail locally. However the CodeFactor reports that I need to import meta explicitly. And after the import, the analyzer complains that it's already included in the import of flutter/material.dart.

I'll paste the conversation later.

AlexV525 commented 2 years ago

So to solve the issue in the CodeFactor, I have to import meta/meta.dart. However:

image

By the time I thought it was included, they told me that the static analysis was not able to recognize the import:

image

Not sure if the above contents would help.

pq commented 2 years ago

So to solve the issue in the CodeFactor, I have to import meta/meta.dart.

Strange. On the surface this seems like a CodeFactor issue and might have something to do with how they're passing resolved packages to (what I assume) is a wrappered linter that they're consuming as a library (and not through the analyzer). If you open an issue w/ them could you cc me and I'm happy to chime in?

Thanks!

AlexV525 commented 2 years ago

If you open an issue w/ them could you cc me and I'm happy to chime in?

I can open a new one instead. Which address should I cc you?

AlexV525 commented 2 years ago

Oh, in the meantime, they've sent me the feedback that:

It turned out to be Flutter issue on our side since we did not officially support it before

So I guess my case should be out of the scope of this issue, and thanks for keeping up! @pq