Skip to content
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

diagnostic for dynamic? #45165

Closed
ghost opened this issue Mar 2, 2021 · 8 comments
Closed

diagnostic for dynamic? #45165

ghost opened this issue Mar 2, 2021 · 8 comments
Assignees
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P3 A lower priority bug or feature request

Comments

@ghost
Copy link

ghost commented Mar 2, 2021

@davidmorgan commented on Mar 2, 2021, 11:47 AM UTC:

I'm surprised to find that dynamic? is allowed and we even see a few uses in google3 ...

Could we lint that you can always switch it to just dynamic? :)

This issue was moved by pq from dart-lang/linter#2493.

@ghost ghost added the NNBD Issues related to NNBD Release label Mar 2, 2021
@ghost
Copy link
Author

ghost commented Mar 2, 2021

@pq commented on Mar 2, 2021, 3:05 PM UTC:

Or perhaps this should be an analyzer diagnostic? (Would anyone ever want to opt-out?)

/fyi @srawlins @bwilkerson

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@srawlins commented on Mar 2, 2021, 3:21 PM UTC:

@leafpetersen @munificent is dynamic? different from dynamic in any way? It'd be awkward to immediately default report against using something specified in the spec...

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@srawlins commented on Mar 2, 2021, 3:23 PM UTC:

Currently the analyzer even allows dynamic dispatch on dynamic?:

void f(dynamic? a) {
  print(a + 2);
}

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@bwilkerson commented on Mar 2, 2021, 4:40 PM UTC:

There's no difference between dynamic and dynamic?, so there's no value in allowing it in code. I doubt that there would be any objections to making it a hint, especially given that hints can be ignored if necessary.

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@leafpetersen commented on Mar 2, 2021, 9:06 PM UTC:

Yeah, dynamic? and void? are completely useless. Never? could be hinted to be equivalent to Null. Null? is also useless. FutureOr<T>? is useless if T is nullable, but you could argue that it's useful documentation.

@ghost
Copy link
Author

ghost commented Mar 2, 2021

@srawlins commented on Mar 2, 2021, 9:08 PM UTC:

Thanks for the responses! I agree that it should be Hinted then; dynamic?, void?, etc. would look to me like I'm supposed to read some sort of signal from that ? and I would carefully tiptoe around it for a few minutes before realizing it is meaningless.

@pq pq added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Mar 2, 2021
@pq pq changed the title Lint for dynamic? diagnostic for dynamic? Mar 2, 2021
@pq pq added the analyzer-warning Issues with the analyzer's Warning codes label Mar 2, 2021
@srawlins
Copy link
Member

srawlins commented Mar 2, 2021

Oof, so many email notifications. Please use "Transfer issue" when transfering an issue from one repo to another, not "Move issue."

@scheglov scheglov added the P3 A lower priority bug or feature request label Mar 3, 2021
@srawlins
Copy link
Member

srawlins commented Mar 9, 2021

Yeah, dynamic? and void? are completely useless. ... Null? is also useless.

Actually, void? appears to be a parse error.

I'm going to submit a diagnostic for dynamic? and Null?.

Never? could be hinted to be equivalent to Null.

I'm going to leave this alone for now. I don't see people writing Never? hardly ever, and I think the guidance is not as strong.

FutureOr<T>? is useless if T is nullable, but you could argue that it's useful documentation.

If that can be argued, then I think we should let this one be. :)

@srawlins srawlins self-assigned this Mar 9, 2021
dart-bot pushed a commit that referenced this issue Mar 11, 2021
Also update the generator:

* Remove same unnecessary '?'
* Update MemberType.name, TypeRef.ref to be non-nullable

Bug: #45165
Change-Id: I438e9ce1e02faac9417a7d2d4ace143f2cf6feb3
TEST=Just the regular trybots.
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/190722
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-warning Issues with the analyzer's Warning codes area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P3 A lower priority bug or feature request
Projects
None yet
Development

No branches or pull requests

3 participants