Skip to content

[Preview Only] Update type annotation lints for repository #170435

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eernstg
Copy link
Contributor

@eernstg eernstg commented Jun 11, 2025

This PR was created to do the same things as #169971, but starting from a fresh commit.

This PR

  • Changes analysis_options.yaml to disable always_specify_types and enable type_annotate_public_apis and omit_obvious_local_variable_types.
  • Modifies three occurrences of LinkedHashMap constructor invocations.
  • Fixes violations of omit_obvious_local_variable_types using dart fix --apply --code=....
  • Formats the code.

@eernstg eernstg requested review from a team and matanlurey as code owners June 11, 2025 14:30
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: internationalization Supporting other languages or locales. (aka i18n) f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: routes Navigator, Router, and related APIs. f: gestures flutter/packages/flutter/gestures repository. a: desktop Running on desktop f: focus Focus traversal, gaining or losing focus f: integration_test The flutter/packages/integration_test plugin team-ios Owned by iOS platform team labels Jun 11, 2025
@eernstg eernstg mentioned this pull request Jun 11, 2025
@matanlurey
Copy link
Contributor

matanlurey commented Jun 11, 2025

LGTM but we will land this in a few weeks at a specific time so that we limit disruptions to the tree and don't make cherry picks to/from an upcoming stable release more complicated.

Could you post exactly what you ran to auto-generate this so we can reproduce this PR (we won't be able to land this exact one for the reasons above)

If there are any ahead of time changes we can make to make this smoother (like the LinkedHashMap changes), I'd be happy to see those land first.

@matanlurey matanlurey marked this pull request as draft June 11, 2025 21:15
@matanlurey matanlurey changed the title Enable obvious lints 2 [Preview Only] Update type annotation lints for repository Jun 11, 2025
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@eernstg
Copy link
Contributor Author

eernstg commented Jun 12, 2025

Thanks for the input, @matanlurey!

Could you post exactly what you ran to auto-generate this

Sure, when analysis_options.yaml has been adjusted to disable always_specify_types and enable type_annotate_public_apis, specify_nonobvious_local_variable_types, and omit_obvious_local_variable_types, and the LinkedHashMap constructor invocation issues have been addressed, it goes as follows:

> dart fix --apply --code=omit_obvious_local_variable_types
> bin/cache/dart-sdk/bin/dart --enable-asserts /usr/local/google/home/eernst/devel/dart/flutter/justin_flutter/flutter/dev/tools/bin/format.dart

Note that the dart fix step gives rise to approximately 54 analyzer crashes (see dart-lang/sdk#60856 for details). I suspect that this could cause some libraries to be processed partially or not at all during the dart fix step. Surely it would be nice to fix that before the final PR on this topic is created.

Next, I haven't yet resolved how to update the snippet code. The following command reports a number of testing failures after these steps:

> bin/cache/dart-sdk/bin/dart --enable-asserts /usr/local/google/home/eernst/devel/dart/flutter/justin_flutter/flutter/dev/bots/analyze_snippet_code.dart --verbose

It might also be possible (and it may or may not be appropriate) to disable omit_obvious_local_variable_types for snippets.

so we can reproduce this PR (we won't be able to land this exact one for the reasons above)

Right, I did expect that it would have to be handled like that.

If there are any ahead of time changes we can make to make this smoother (like the LinkedHashMap changes), I'd be happy to see those land first.

Yes, presumably the best way to change those three locations would be to use the type Map and a map literal rather than invoking the LinkedHashMap constructor. This would require at least some other locations to change, too, because they expect the type to be LinkedHashMap. The point is that, as far as I know, no actual benefit is achieved by insisting that it must be precisely a LinkedHashMap, so we might just as well follow the normal rules also in these 3 locations.

Next, we could change analysis_options.yaml: Disable always_specify_types and enable type_annotate_public_apis and specify_nonobvious_local_variable_types. These changes should not give rise to any new lint messages, they will just allow local variable declarations to omit the type when it is obvious, and also allow everything else which was allowed before.

Later, when we perform the actual changes where local variable type annotations are removed, we just use the freedom to do this. This can be done in one big step or in any number of smaller steps.

If it is done in multiple steps then each of them would involve temporarily changing analysis_options.yaml to enable omit_obvious_local_variable_types in order to get the fixes, and then disabling it again.

Finally, as part of the big step or the last small step, omit_obvious_local_variable_types is enabled (as part of the PR rather than temporarily).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: animation Animation APIs a: desktop Running on desktop a: internationalization Supporting other languages or locales. (aka i18n) a: tests "flutter test", flutter_test, or one of our tests a: text input Entering text in a text field or keyboard related problems d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: focus Focus traversal, gaining or losing focus f: gestures flutter/packages/flutter/gestures repository. f: integration_test The flutter/packages/integration_test plugin f: material design flutter/packages/flutter/material repository. f: routes Navigator, Router, and related APIs. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically team-ios Owned by iOS platform team tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants