FED-1569 Prepare for null safety: fix lints and implicit casts #366

greglittlefield-wf · 2023-08-21T21:15:21Z

Motivation

A while back @aaronlademann-wf started a branch to update react-dart's to use workiva_analysis_options and updated lints, and clean things up.

I then based off that branch, made some adjustments, and enabled implicit casts to improve type safety and make things easier when migrating to null safety. I then used that as a base for a null safety branch.

As a result, all of the null safety changes are dependent on these unmerged lint and implicit cast changes.

To get this repo in a better state, and to enable the merge of null safety changes, let's land these lint and implicit cast changes.

Changes

The original commits also ended up getting pretty messy, with some larger commits and quite a few merge commits in between. The changes also were based on workiva_analysis_options v1 instead of v2.

So, I ended up redoing/rebasing most of these changes, splitting them up into very granular and easy-to-review commits, and rebasing the null safety branch on top of those. Shoutout to @aaronlademann-wf for your original work on this! ❤️

For review, I highly recommend reviewing the changes commit-by-commit since there are so many different lint fixes that touch a lot of code. There's a lot of commits, but most of them are either small or dart fix --apply ones that are very easy to review.

Summary of changes:

Upgrade lint configuration to workiva_analysis_options v2, and customize some lints
Fix lints
- The vast majority of these are individual commits using dart fix --apply
Fix implicit casts
- This was done primarily in one commit, along with related adjustments make variables final and remove LHS typing where possible
- The vast majority of these changes occurred in example and test directories; changes to lib were minimal, so the risk of regression here should be pretty low so long as the lib changes are reviewed thoroughly.
- I couldn't disable implicit casts an analysis_options since we'd still get a lot of warnings as a result of ReactComponentFactoryProxy.call returning dynamic as opposed to ReactElement. I'd like to change that typing, but since that's technically a major change, let's save that for another PR.
Stop running CI on Dart 3 until we're null-safe (it was failing on this PR)

Testing

CI passes
Consumer tests in over_react pass

…er_generic_function_type_aliases

@OverRide

The fewer explicit types we have, the fewer need to be updated during the transition to null safety Done via replacement of `(@OverRide\n )(?![gs]et )([^(=\n]+ )([^(=\n]+\()` with `$1/$3`

@OverRide

The fewer explicit types we have, the fewer need to be updated during the transition to null safety Done via replacement of `(@OverRide\n )([^(=\n]+ )(get [^(=\n]+=)` with `$1/$3`

aviary-wf · 2023-08-21T21:16:24Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

greglittlefield-wf · 2023-08-21T21:20:40Z

.github/workflows/dart_ci.yml

Reminder to review commits one-by-one!

This should make review much easier. See PR description for more info

aaronlademann-wf

Mostly some questions...

Other than that, I can plus one / ten this after that...

aaronlademann-wf · 2023-08-22T15:28:51Z

lib/react.dart

-/// Create React DOM `Component`s by calling the specified [creator].
-_createDOMComponents(creator) {
- a = creator('a');
- abbr = creator('abbr');
- address = creator('address');


How long has this been hanging around unused?

Looks like almost 3 years 💀 1ef1196#diff-971a483a502cde928eaed632c11bd797a2a1e6c112fb9152dbe374291d7c8dbaL2804

aaronlademann-wf · 2023-08-22T15:39:14Z

lib/react.dart

 String get displayName {
- var value;
+ String value;


I'm assuming there's no reason to worry about this type being tightened?

Nope; originally it was a dynamic variable that would always be either assigned to a String or uninitialized, then implicitly casted to a String when it was returned.

aaronlademann-wf · 2023-08-22T15:41:17Z

lib/react_client/js_interop_helpers.dart

@@ -7,6 +7,7 @@ import 'dart:collection';
 import 'dart:js_util';

 import 'package:js/js.dart';
+import 'package:react/react_client/react_interop.dart' show forwardRef2;


Note for posterity: this was added for a reference in a doc comment.

lib/src/react_client/dart_interop_statics.dart

aaronlademann-wf · 2023-08-22T15:45:45Z

lib/src/react_client/factory_util.dart

 children = shouldAlwaysBeList ? childrenArgs.map(listifyChildren).toList() : childrenArgs;
- markChildrenValidated(children);
+ markChildrenValidated(children as List);


Should this still get cast as List if shouldAlwaysBeList is false?

On the line above, the value chlidren is reassigned to is always a List, and markChildrenValidated takes in a List, but the static type of children is still dynamic so we need a cast here.

In null safety, with Dart's type inference improvements, children actually has a static type of List, and this becomes an unnecessary cast warning and ends up getting removed.

aaronlademann-wf · 2023-08-22T15:47:52Z

lib/src/react_client/synthetic_data_transfer.dart

+ final files = List<File>.from(rawFiles ?? []);
+ final types = List<String>.from(rawTypes ?? []);


Shouldn't this be

Suggested change

final files = List<File>.from(rawFiles ?? []);

final types = List<String>.from(rawTypes ?? []);

final files = List.from(rawFiles ?? []).cast<File>();

final types = List.from(rawTypes ?? []).cast<String>();

I think in this case, either would be acceptable.

List<File>.from(rawFiles ?? []) creates a new list and casts objects to the target type as they're copied.

List.from(rawFiles ?? []).cast<File>() creates a new List<dynamic> and then returns a view on that list that casts items as they're accessed.

Casting them on copy feels better to me in this case, though, so that any bad typing is revealed always, when the event is dispatched, as opposed to only when these properties are accessed.

aaronlademann-wf

+1

aaronlademann-wf · 2023-08-22T18:42:02Z

QA +1

CI Passing
over_react consumer CI passing

@Workiva/release-management-pp

sydneyjodon-wk

+1 on Aaron's commit

rmconsole-wf

+1 from RM

greglittlefield-wf and others added 28 commits August 9, 2023 14:58

Upgrade lints to workiva_analysis_options v2.recommended

e452d16

dart fix --apply --code=unnecessary_new

1446d0b

dart fix --apply --code=unnecessary_const

1e322c2

dart fix --apply --code=prefer_single_quotes

acc03f1

dart fix --apply --code=deprecated_colon_for_default_value

5d2793d

dart fix --apply --code=annotate_overrides

d19f23d

dart fix --apply --code=prefer_final_in_for_each

bcad3b1

dart fix --apply --code=omit_local_variable_types,prefer_final_locals

8d1a829

Fix missing inferred generics

46a38d2

dart fix --apply --code=use_function_type_syntax_for_parameters,pref…

17b30cc

…er_generic_function_type_aliases

Fix more typedefs

b79d3eb

dart fix --apply --code=unnecessary_this

6490a9c

dart fix --apply --code=unnecessary_brace_in_string_interps

1b5f972

dart fix --apply --code=unnecessary_parenthesis

2948941

Disable some more lints

911c914

Remove some unnecessary function parameter types, link to linter issue

70429ac

Ignore lints that could change behavior or break public API if fixed

ae1914e

dart fix --apply --code=unused_import

8b83f89

dart fix --apply --code=unnecessary_import

ae87897

Fix miscellaneous remaining lints (dart fix --apply)

82c5d9e

Ignore componentZone usage warnings

a7beb88

Remove unused code

1a6d534

Fix warnings by hand

9949639

Fix comment references and links - all credit to @aaronlademann-wf

5344c86

Fix implicit casts, clean up variable types and final

e1e3f7f

Format

35297ff

Remove return types on overrides so they can be inferred

94d3507

The fewer explicit types we have, the fewer need to be updated during the transition to null safety Done via replacement of `(@OverRide\n )(?![gs]et )([^(=\n]+ )([^(=\n]+\()` with `$1/$3`

Remove return types on overridden getters so they can be inferred

cf5e31a

The fewer explicit types we have, the fewer need to be updated during the transition to null safety Done via replacement of `(@OverRide\n )([^(=\n]+ )(get [^(=\n]+=)` with `$1/$3`

rmconsole-wf added the Merge Requirements Unmet label Aug 21, 2023

greglittlefield-wf added 2 commits August 21, 2023 14:17

Format

b778dcd

Disable Dart 3 CI run for now

920f9b1

greglittlefield-wf commented Aug 21, 2023

View reviewed changes

rmconsole6-wk changed the title ~~Prepare for null safety: fix lints and implicit casts~~ FED-1569 Prepare for null safety: fix lints and implicit casts Aug 21, 2023

greglittlefield-wf marked this pull request as ready for review August 21, 2023 21:52

greglittlefield-wf requested review from a team, annawatson-wk and hunterbreathat-wk and removed request for a team, annawatson-wk and hunterbreathat-wk August 21, 2023 23:43

aaronlademann-wf self-assigned this Aug 22, 2023

aaronlademann-wf reviewed Aug 22, 2023

View reviewed changes

Add comment LHS-typed function declaration

60aec37

greglittlefield-wf requested a review from aaronlademann-wf August 22, 2023 18:37

aaronlademann-wf approved these changes Aug 22, 2023

View reviewed changes

rmconsole-wf added the RM Ready label Aug 22, 2023

sydneyjodon-wk approved these changes Aug 22, 2023

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Aug 22, 2023

rmconsole-wf approved these changes Aug 22, 2023

View reviewed changes

rmconsole7-wk merged commit 5b4aef1 into master Aug 22, 2023
1 check passed

rmconsole7-wk deleted the lints-and-implicit-casts-take-2 branch August 22, 2023 18:57

aaronlademann-wf removed their assignment Aug 22, 2023

rmconsole-readonly-wk mentioned this pull request Sep 25, 2023

Consume react-dart 6.2.1 Workiva/over_react#843

Closed

greglittlefield-wf mentioned this pull request Oct 2, 2023

FED-1728 Migrate to null safety #373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-1569 Prepare for null safety: fix lints and implicit casts #366

FED-1569 Prepare for null safety: fix lints and implicit casts #366

greglittlefield-wf commented Aug 21, 2023 •

edited

Loading

aviary-wf commented Aug 21, 2023

greglittlefield-wf Aug 21, 2023

aaronlademann-wf left a comment

aaronlademann-wf Aug 22, 2023

greglittlefield-wf Aug 22, 2023

aaronlademann-wf Aug 22, 2023

greglittlefield-wf Aug 22, 2023

aaronlademann-wf Aug 22, 2023

aaronlademann-wf Aug 22, 2023

greglittlefield-wf Aug 22, 2023

aaronlademann-wf Aug 22, 2023

greglittlefield-wf Aug 22, 2023

aaronlademann-wf left a comment

aaronlademann-wf commented Aug 22, 2023

sydneyjodon-wk left a comment

rmconsole-wf left a comment

		final files = List<File>.from(rawFiles ?? []);
		final types = List<String>.from(rawTypes ?? []);

FED-1569 Prepare for null safety: fix lints and implicit casts #366

FED-1569 Prepare for null safety: fix lints and implicit casts #366

Conversation

greglittlefield-wf commented Aug 21, 2023 • edited Loading

Motivation

Changes

Testing

aviary-wf commented Aug 21, 2023

Security Insights

Action Items

Choose a reason for hiding this comment

Reminder to review commits one-by-one!

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

aaronlademann-wf commented Aug 22, 2023

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

rmconsole-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Aug 21, 2023 •

edited

Loading